Recipe CMS 1.2.X breaks semantic versioning

[silbinarywolf]

The problem
I had failing tests in Travis because I was pulling down Recipe-CMS 1.2.
https://github.com/silbinarywolf/silverstripe-phpstan/blob/dev-ss4/.travis.yml#L25

I had to update my PHPStan tests to allow for two different types, SiteTree / SiteTreeLink:
https://github.com/silbinarywolf/silverstripe-phpstan/blob/dev-ss4/tests/Reflection/SiteTreeMethodClassReflectionExtensionTest.php#L79

This was caused by this:
silverstripe/silverstripe-versioned#75 (comment)

The solution
Since the public return types of a function have been changed, I think Recipe-CMS should be bumped to the next MAJOR version so it adheres to semantic versioning. (And anything else that uses the future version of silverstripe-versioned will need a MAJOR bump)

[robbieaverill]

I think this is a pretty indepth way to look at the SilverStripe API.

SiteTree::LinkTracking() returns a DataList before and after that change, but it's the source class that has changed here and that your tests are picking up which isn't immediately part of that API, although arguably it is in a way... 😆

Generally we say that the semver applicable API is that which is included in PHPDocs - I doubt that there would be something like @return SiteTree[] attached to this method. I think that as SilverStripe progresses further into SS5 which will allow us to use strict typing in PHP7, things will naturally become much cleaner and tidier.

Personally I'm inclined to say in this case that it's a "won't fix" - however I am impressed that there are tools like yours scrutinising the API so closely!

Does this actually affect any project functionality for you, or was it a unit test failure that highlighted this change to you?

Side note: we would probably rather revert parts of a change to make them semver compatible than bump a major version to cover that - this change is not released yet which gives us more flexibility in this area too.

What does the rest of the @silverstripe/core-team think?

[sminnee]

Change of return type is a public API. Code that breaks a module is a breaking change. So I think this is an API breakage.

However, rather than do a major version bump I would probably fix the "bug" that is the API breakage.

[dhensby]

So the list should be returning the pages that track the current page instead of the link object? I guess we could work that in.

I think this probably is breaking as people would be expecting a list of SiteTree objects not just DataObjects...

[sminnee]

Yeah the main thing is that we shouldn't have changed this between 4.1 and 4.2. So let's change it back before tagging 1.2 / 4.2.

If we want to provide access to the SiteTreeLink object for some purpose, we should have a 2nd method.

Related: the recipe should have the same version numbers as framework, or more to the point, if someone says "I've installed SilverStripe version X.Y" that number should refer to the recipe version number. We should retag 1.x as 4.x.

[sminnee]

PS: thanks @silbinarywolf for pointing this out before we released a stable tag ;-)

[silbinarywolf]

Thanks for taking backwards compatibility seriously guys.
It's really appreciated!

[chillu]

Related: the recipe should have the same version numbers as framework, or more to the point, if someone says "I've installed SilverStripe version X.Y" that number should refer to the recipe version number. We should retag 1.x as 4.x.

Created a ticket: silverstripe/recipe-core#24

[tractorcow]

So let's change it back before tagging 1.2 / 4.2

Reverting the change would break non sitetree link tracking which was important for elemental, by the way.

I would tag versioned 2.0 instead and keep recipe at 1.2. The module has the api not the recipe per se. That's the same solution we decided on earlier for graphql 2.0

I also considering suggesting a wontfix with upgrading more but that's less desirable if we want to be better at semver.

Cc @unclecheese to confirm my thinking

[chillu]

The original reporter said:

I think Recipe-CMS should be bumped to the next MAJOR version so it adheres to semantic versioning. (And anything else that uses the future version of silverstripe-versioned will need a MAJOR bump)

That's unrealistic, due to our LTS commitments. This is a real world fix solving a real world problem (link/image tracking, e.g. in elemental). Moving this effectively to SilverStripe 5.x isn't an option.

If we want to provide access to the SiteTreeLink object for some purpose, we should have a 2nd method.

So we're happy to create a confusing API on the long run (method name different from relationship name), just to avoid a theoretical API breakage? From what I understand, the new return signature is DataObject[] rather than SiteTree[]? So if you use link tracking in your own code (already a bit of a stretch to imagine a use case), this would only affect you if you made specific assumptions about using SiteTree features?

I would tag versioned 2.0 instead and keep recipe at 1.2. The module has the api not the recipe per se. That's the same solution we decided on earlier for graphql 2.0

Have we ever discussed/decided if recipes are semver themselves? I think they need to be, otherwise we shouldn't advocate their use via installer etc. Bumping silverstripe/graphql from 1.x to 2.x in a minor core release was an exception rather than a precedent (we should've never tagged 1.0 in the first place).

[silbinarywolf]

@chillu The problem is that LinkTracking() returns SiteTreeLink[] in Recipe-CMS 1.2.X rather than SiteTree[].

This means any user code reading or writing from this collection will need to be updated.

[sminnee]

This is a bug in versioned, not the receipe, so I've re-logged it there. ^

[tractorcow]

Sorry I'm finally back from china and able to investigate this properly.

@silbinarywolf

@chillu The problem is that LinkTracking() returns SiteTreeLink[] in Recipe-CMS 1.2.X rather than SiteTree[].

No that's not the change. It returns a ManyManyThroughList instead of a ManyManyList, but the dataClass() of each list remains SiteTree, and both inherit the same "base" class "RelationList", and still share the common "Relation" interface.

[tractorcow]

@silbinarywolf was it actually this line that failed?

https://github.com/silbinarywolf/silverstripe-phpstan/blob/dev-ss4/tests/Reflection/SiteTreeMethodClassReflectionExtensionTest.php#L87

[silbinarywolf]

Nah it was this one!
https://github.com/silbinarywolf/silverstripe-phpstan/blob/dev-ss4/tests/Reflection/SiteTreeMethodClassReflectionExtensionTest.php#L98

That's why there is a check to see if the value is SiteTree or SiteTreeLink now. To make sure my tests pass on Recipe 1.2.X

[tractorcow]

I'm not sure exactly how those tests work, but the iterator of SiteTree::LinkTracking is still SiteTree.

I have a feeling you MAY have a bug in your phpstan library for manymanythroughlist. Can you double check please?

[tractorcow]

Ah yes, here's the bug.

https://github.com/silbinarywolf/silverstripe-phpstan/blob/7fadf204193dcfa313666509a3e5d20098ab970a/src/Reflection/MethodClassReflectionExtension.php#L129-L138

The type of the list is not the 'through' array value. That's the 'internal type' only; You'll need to use both 'through' and 'to' to get the actual list class type.

See DataObjectSchema for how we do this internally.

// Normal many-many
            if ($manyManySpec === $parentClass) {
                return $inverseComponentName;
            }
            // many-many through, inspect 'to' for the many_many
            if (is_array($manyManySpec)) {
                $toClass = $this->hasOneComponent($manyManySpec['through'], $manyManySpec['to']);
                if ($toClass === $parentClass) {
                    return $inverseComponentName;
                }
            }

Where you have $type = $type['through']; you'll actually need to do deep inspection on the through class to pull the type of the has_one property specified by $type['to'].

Hope that helps. :)

[tractorcow]

I raised a ticket at https://github.com/silbinarywolf/silverstripe-phpstan/issues/13

[silbinarywolf]

@tractorcow Thanks! I'll investigate this weekend and confirm if the issue is on my end.

[silbinarywolf]

@tractorcow Bug was confirmed to be my issue, look at the issue you raised for more information on my testing steps / etc.

@sminnee You can close this.