Yii2: #14456 breaks backward compatibility

[devantoine]

Regarding https://github.com/yiisoft/yii2/pull/14456#issuecomment-327409143 I think the PR #14456 breaks backward compatibility.

You can totally have some classes extending BaseActiveRecord (which implements ActiveRecordInterface which extends StaticInstanceInterface) and having an instance() method.

See for instance Humhub: https://github.com/humhub/humhub/blob/master/protected/humhub/modules/user/models/GroupPermission.php#L26

[klimov-paul]

That is why it is mentioned in UPGRADE.

[samdark]

That's possible scenario when adding any public/protected method. Probably we need to revise releases and versioning overall (will be discussed) but current versioning policy allows new features and enhancements in minor releases therefore allows public API expansion.

[samdark]

See https://github.com/yiisoft/yii2/issues/13773

[cebe]

how about renaming the method to modelInstance() to make it less likely to conflict?

[samdark]

I'm OK with it. @klimov-paul ?

[klimov-paul]

how about renaming the method to modelInstance() to make it less likely to conflict?

And what about following comment then:
https://github.com/yiisoft/yii2/pull/14456#issuecomment-323966799
?

We have already decided the naming, besides who can tell someone have not declared modelInstance() method inside his ActiveRecord.

We have already placed the note to UPGRAGE.md, I can not see why there should be anything else to be done.

[samdark]

Need more opinions. @yiisoft/core-developers? @yiisoft/reviewers ?

[rob006]

I would rather move these changes to 2.1 milestone than introduce some weird names like modelInstance().

The main issue here is lack of LTS line which contains only BC bugfixes. Right now big projects based on Yii (or extensions) doesn't have any way to safely use Yii without locking framework version to specific release. Just look at https://github.com/yiisoft/yii2/blob/master/framework/UPGRADE.md - changing method signature or introducing new methods in interfaces are regular BC breakers in last releases. Yii simply does not have BC line - new versions always always bring a risk of serious BC break.

[samdark]

Well, introducing new methods is allowed in minor versions both according to our versioning policy and to SemVer.

[schmunk42]

Without knowing the deeper internals for this change, I have some general questions:

• isn't every public API expansion possibly backward-incompatible? There could always be a project who is already using the new method in an extended class - or does this relate to interfaces specifically?
• while the change from Object to BaseObject has a detailed description what a developer needs to do if he wants to upgrade (also to PHP 7.2) - there's no advice about instance(). Would a developer needs to rename every method call in his project for example or could he work around it?

---

[addon] Java example, but related https://stackoverflow.com/q/35187899/291573 - since this change has been made to an interface it would be BC-breaking, but on the other hand all classes without an interface would need to be final, which looks way too strict to me.

Here's another article about Semantic Versioning and Public Interfaces stating:

Thus, interfaces are more susceptible to BC breaks than concrete classes. Once an interface is published as “stable”, I don’t see how it can be changed at all per the rules of SemVer (that is, unless you bump the major version). The only thing you can do to maintain BC is add an entirely new interface to the package while leaving the old one in place, perhaps even extending the old one if needed.

Last but not least the Symfony BC policy - linked in above article.

[samdark]

isn't every public API expansion possibly backward-incompatible? There could always be a project who is already using the new method in an extended class - or does this relate to interfaces specifically?

It's general. Doesn't matter if it's a class or interface. In both SemVer and YiiVer it's allowed to be released in minor version which 2.0.13 is.

while the change from Object to BaseObject has a detailed description what a developer needs to do if he wants to upgrade (also to PHP 7.2) - there's no advice about instance(). Would a developer needs to rename every method call in his project for example or could he work around it?

No workaround is possible. It's common for minor versions and I've never seen any library is describing every introduced non-private method or property in this manner.

[SilverFire]

Each update we create tons of new method and we can not predict cases like this. There's always a possibility that new public or protected method will break users' code just because of incompatible declaration. I think we have nothing to do it.

[tom--]

Adding a method is ok. But there's a possible fatal error about the new abstract method not being implemented. Either we consider this extremely unlikely or UPGRADE should have guidance on how to fix it. Can we reasonably assume that using the trait will work?

[klimov-paul]

It is already mentioned at UPGRADE.md:
https://github.com/yiisoft/yii2/blame/master/framework/UPGRADE.md#L111-L113

We already have many such changes in the past, check following:
https://github.com/yiisoft/yii2/blame/master/framework/UPGRADE.md#L124_L125
https://github.com/yiisoft/yii2/blame/master/framework/UPGRADE.md#L127-L128
https://github.com/yiisoft/yii2/blame/master/framework/UPGRADE.md#L130-L132

Why should we suddenly be so concern about this now?

[devantoine]

@klimov-paul because people won't use your framework if it breaks their code at each minor update.

[klimov-paul]

There is a versioning policy, which explains what changes may appear at particualr release.
For the 2.0.x there is an explanation:
https://github.com/yiisoft/yii2/blob/master/docs/internals/versions.md#2xy-minor-releases

It is not some secret: you have been warned about it.

Yes, it is a sad thing that we are unable to follow strict versioning rules, but we simply do not have enough resources for that. If we use more strict rules for the releases this framework will not move anywhere as it happens for 2.1 version at the present state.

[cebe]

Adding new methods to a class is usually never considered a breaking change. Usually method names are quite unique to the case they are added for and naming conflicts with methods in overwritten classes occur rarely. In this case we had the (bad) luck to match a common name that has been implemented by a popular project, otherwise we would never had noticed.

Keeping 100% backwards compatibility is not possible (only without changing anything).

I do not think we can make a general rule from this, or change a policy to avoid things in the future, it will limit the possibility to change anything too much.
Imo the only question is what we want to do in this case, as we got to know the issue before release.

PS: one of my projects also had an instance() method in AR classes, conflicting with this change.

[cebe]

Imo the only question is what we want to do in this case, as we got to know the issue before release.

Resolving the case with humhub would likely be discussed here: https://github.com/humhub/humhub/issues/2745#issuecomment-328115443 as far as I see it is not hard to solve it.

From the current information available I do not see anything that needs to be changed in Yii code.
We might describe the UPDATE instructions a bit more clearly as we now know of this case.

[cebe]

Last but not least the Symfony BC policy - linked in above article.

We could create such a policy for Yii to make it more clear what kind of changes to expect. This is something we could consider for 2.1, when the framework is split up into more small parts we can allow fewer changes and still be able to move forward.

[tom--]

My concern is if the UPGRADE text is sufficient or should be expanded and that's the motive for my question.

[cebe]

I have mentioned this case in 5ccbe85ffa18cd438e8f8008b2f736e41693fb7a

[SilverFire]

The same story here: https://github.com/yiisoft/yii2/pull/14441#issuecomment-315127527