A critical interface method was just removed in 5.3.0

[dereuromark]

Symfony version(s) affected: 5.3.0

Description
Regression in minor release.
symfony/security-core@v5.2.8...v5.3.0
A critical interface method was just removed instead of normal deprecation workflow and removal in next major.

How to reproduce
Implement UserInterface on vendor or project level, code against interface and now find yourself missing this contract (to be sure that method is available and implemented properly.

Possible Solution
Keep it in with deprecation only or add this method as soft annotation at least (as implementing code cannot do that post-factum now for the vendor interface).

Additional context

 ------ ------------------------------------------------------------------------
  Line   SprykerShop/Yves/CustomerPage/Plugin/Provider/CustomerUserProvider.php
 ------ ------------------------------------------------------------------------
  58     Call to an undefined method
         Symfony\Component\Security\Core\User\UserInterface::getUsername().
  115    Call to an undefined method
         Symfony\Component\Security\Core\User\UserInterface::getUsername().
 ------ ------------------------------------------------------------------------

The same seems to be true for Authentication/RememberMe/PersistentTokenInterface.php class.

[stof]

/cc @wouterj

I think the method has been removed from the interface with the intent of allowing new code to implement only the new method. But that's indeed wrong as that would make such implementations incompatible with code expecting the 5.x UserProviderInterface.

[derrabus]

Is this causing an actual problem or is this just about your static code analyzer complaining?

[stof]

Well, if you implement new code without the loadUserByUsername, you could get a fatal error when passing it to a code written against the 5.2 interface

[stof]

and the symfony core is not the only caller of that interface. Bundles providing custom auth methods are likely to rely on a UserProvider as well (they are even encouraged to do so)

[JKapitein]

getUsername was renamed, I think this "break" was intentional

#40403

It's a simple fix and one that I feel should be allowed in a minor, but it might break authentication bundles supporting ^5.0 that implement and encourage use of their own UserInterface implementations

[dereuromark]

As a software vendor we support and build upon e.g. Symfony 4+5 of all minors and we can only control so much of what customers are using (what combination of previously working packages get installed through composer).
As such, there is a high risk of failure now introduced that is not helpful. Having it deprecated would not halt the "rename" mentioned, and how to move forward for future code and implementations.

[stof]

@JKapitein the renaming is indeed intentional. But the way is has been done does not respect our BC policy. The old method should not have been removed from the interface: https://symfony.com/doc/current/contributing/code/bc.html#changing-interfaces

[wouterj]

Code supporting both <5.3 and >=5.3 will have to use a method_exists check anyways for either method name. So there is no "risk" here.

For reference, I didn't do this at first, but was explained that this is how we have done such method renames in the past. See https://github.com/symfony/symfony/pull/30388/files#diff-4412ea6a8e82d6b291607b14e89869f152887f0cf57efdf97b5908f82ebc8e21 for instance.

to be clear: I'm not against doing it this way, but I'm not sure if the pros (keeping a deprecated method) are heavier than the cons (breaking tests while upgrading a minor).

I understand and share your concerns, but going with @deprecated is a no-go as it'd mean triggering a deprecation notice that is impossible to fix until 6.0 for all (existing and new) implementations, while a broken mock can be fixed.
We already accepted this tradeoff in #31814 (comment)

[stof]

@wouterj the issue is code written for 5.2 that gets incompatible with 5.3+. That's a BC break in a minor version (for code consuming the UserProviderInterface).

[JKapitein]

@wouterj in the release blog it's described as such

That’s why in Symfony 5.3 we’ve decided to avoid this confusion and we’ve renamed “username” to “user identifier”. This might require some changes in your application code (in 5.3 the old names still work but they are deprecated and in Symfony 6.0 they will be removed)

As far as I can tell (and I could very well be wrong about this) this is not the case. The old getUsername still works if it was implemented, but the "new" interface requires getUserIdentifier. Further, static code analysis will fail on interface hints and the old getUsername method calls.

With the prevalence of UserProviders and other Security related classes in vendor bundles, it seems like something that would fit better in Symfony 6. I like the change, and I realize that it's not easy to add deprecations to widely used interfaces in PHP, but I feel like this should not have been in a minor.

[wouterj]

For reference: I'm fine either way. As long as we can remove the old method in 6.0 and users are warned via the usual Symfony methodology about this before 6.0.

[JKapitein]

That's the best case we could go for. Currently code written against (non experimental) ^5.2 could be incompatible with 5.3 which should not be possible

[wouterj]

Neither 3 suggestions I can come up with (2 of them implemented in the PR at some point) fulfill these requirements:

1. Removing the old method: issues described here
2. Keeping the old method with @deprecated: no application will be deprecation free in 5.4
3. Keeping the old method without @deprecated: we cannot tell any callers of the method that the method is deprecated and might not be available anymore in 6.0+ code. Effectively, this forbids us to remove the method.

Of all these, it was decided in the PR (and previous interface method renaming procedures) that (1) is the solution with the least practical issues. (note that static analysis is never part of our BC policy, as that forbids us to make any change to anything)

[nicolas-grekas]

There is no BC break here, since code that used to work continues to behave the same.
Does our BC layer trigger a deprecation if both getUsername and getUserIdentifier are defined? If yes, I suppose we should remove the deprecation in this specific case.
About static analyzers, we could add an @method annotation, with a proper deprecation notice in it? We would need to teach DebugClassLoader(and potentially other stans) about the meaning of this annotation so that it doesn't trigger a deprecation when the method is not implemented.
@method string getUsername() @deprecated since Symfony 5.3 for example?

[JKapitein]

The static analysis isn't an issue imo since it would be a normal task to fix those during the upgrading of the framework.

The renaming of the interface method from getUsername to getUserIdentifier will break <5.3 implementations of UserInterface though, which is widely used, and I think this violates the BC promise. The updated Security components do ship with the BC layer of method_exists checks and deprecation warnings, but considering many projects make their own User(Provider) implementation (and like @stof said, are in fact encouraged to do), it is a fairly impactful change.

Take https://github.com/lexik/LexikJWTAuthenticationBundle for example, a popular bundle that claims to work against symfony/security-bundle ^5.1. If you currently pull the master and run composer install, it will install the 5.3 UserInterface and therefore be incompatible with the JwtUser class.

[nicolas-grekas]

The BC promise is about ensuring that code that is already written will keep working despite the changes.
That's the case here. If newly written code works only with 5.3, that's just normal, since using new features always requires bumping something. And if newly written code wants to work with both 5.2 and 5.3, it's also possible.

[stof]

That's the case here.

@nicolas-grekas no. that's not the case. If I have written a bundle consuming a UserProviderInterface against 5.2, that bundle will suddenly break (with a fatal error) when a project using the bundle implements the UserProviderInterface of 5.3.

Currently, we are backward compatible for cases implementing a UserProviderInterface in the old or new way and passing it to the core Symfony callers. But we are not backward compatible for other callers.

[stof]

About static analyzers, we could add an @method annotation, with a proper deprecation notice in it? We would need to teach DebugClassLoader(and potentially other stans) about the meaning of this annotation so that it doesn't trigger a deprecation when the method is not implemented.
@method string getUsername() @deprecated since Symfony 5.3 for example?

Making the UserProviderInterface backward compatible for caller does not allow omitting the method. There's a reason why our BC promise says that removing a method from an interface is forbidden. If we allow it, it is impossible to consume the interface safely.

[stof]

2\. Keeping the old method with `@deprecated`: no application will be deprecation free in 5.4

that's not true. If you implement the method of the interface, that does not trigger a deprecation warning AFAICT. The deprecation warning is triggered if you miss implementing the new one.

[nicolas-grekas]

that bundle will suddenly break (with a fatal error) when a project using the bundle implements the UserProviderInterface of 5.3.

I get that, but that's still in line with what I wrote: no already written code will break because of the change we're talking about here.

So, should we force ppl to implement both the new and the old methods in 5.4 to cover the scenario you're describing @stof?

On my side, I think it could be enough to add two @method annotations, with better explanations if needed, eg:
@method string getUsername() @deprecated since Symfony 5.3, should still be implemented when compatibility with 5.2 is desired.

[dereuromark]

We are fine with the method annotation, while I still think that a proper way of deprecation would keep the method in place in code.
Especially since code usually has to work together across different minors of (Symfony) packages.
And with this, there is still a time bomb in place now for all code, both old and new, which is super hard to detect. Given that all consumers of this Symfony code expect SemVer rules in place, and that includes interface contracting as per SemVer of both Symfony docs as well as official one. After all, that's how you declare package compatibility (^ instead of ~ locking).

[stof]

I get that, but that's still in line with what I wrote: no already written code will break because of the change we're talking about here.

Already written bundles declaring compatibility with symfony/core ^5.2 thanks to semver will break.
Your argument would be that we only provide semver to projects, not to other packages.

[pamil]

Same is true for Symfony\Component\Security\Core\Authentication\Token\TokenInterface::getUsername and Symfony\Component\Security\Core\User\UserProviderInterface::loadUserByUsername. In my opinion, Symfony should keep the deprecated method until 6.0, right now it's hard to figure out what happened to keep compatibility with Symfony 4.4 and 5.3 at the same time in Sylius.

[stof]

Code supporting both <5.3 and >=5.3 will have to use a method_exists check anyways for either method name. So there is no "risk" here.

@wouterj The method_exists check should be necessary to avoid a deprecation warning, not to avoid a fatal error. Code that get a deprecation warning is still working, and that's the basis of the BC promise.

[wouterj]

See #41493

[dereuromark]

I close this discussing then pending the PR merge.
Thank you @wouterj for the quick reaction.

[derrabus]

Thank you everyone for this healthy discussion. I must admit, I have underestimated the consequences of changing the interfaces the way we did. And I agree that we should add the methods to the interfaces again.

That being said, if I may make a request:

As a software vendor we support and build upon e.g. Symfony 4+5

Please test beta versions and RCs! This change has been in beta testing for quite some time. If you are building a software product that heavily relies on Symfony, please make sure that you test against pre-releases. If an issue like this pops up during the stabilization phase, it is way easier to fix than now that a stable release is already out there.