8000 [Validator] Breaking change caused by return types upgrade from 4.3 to 4.4 · Issue #34478 · symfony/symfony · GitHub
[go: up one dir, main page]
Skip to content

[Validator] Breaking change caused by return types upgrade from 4.3 to 4.4 #34478

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Closed
s-duval opened this issue Nov 21, 2019 · 6 comments
Closed

[Validator] Breaking change caused by return types upgrade from 4.3 to 4.4 #34478

s-duval opened this issue Nov 21, 2019 · 6 comments
Labels
BC Break Bug Status: Needs Review Validator

Comments

@s-duval
Copy link
s-duval commented Nov 21, 2019

Symfony version(s) affected: 4.4.0

Description
Upgrading from 4.3 to 4.4 today causing cache clearing and the bundle : sonata admin bundle to throw errors.

From the commit "Add return types to internal|final|private methods" 3211618

Return type was forced even on magic methods which can cause breaking changes on third party libraries overriding them without specifying the return type

How to reproduce
To reproduce the bug simply create a new Constraint class overriding one of the method where the return type was forced in 4.4, this class will work in 4.3 but not when upgrading in 4.4

class MyConstraint extends Constraint { public function __sleep(){ ...} }

Possible Solution
Maybe don't force return types on 4.4 version to keep retro compatibility
and do those breaking changes in 5 ?
@s-duval s-duval changed the title [Validator] Breaking change caused by return types [Validator] Breaking change caused by return types upgrade from 4.3 to 4.4 Nov 21, 2019
@xabbuh
Copy link
Member
xabbuh commented Nov 21, 2019

The __sleep() method is marked as internal. For those the BC promise does not apply. If you override internal methods, you will need to be prepared to update your code if we change them. Thus, I am going to close here as explained.

@xabbuh xabbuh closed this as completed Nov 21, 2019
@nicolas-grekas
Copy link
Member

Actually we might want to reconsider this one: __sleep is the only way to serialize an objet so there is no other choice than overriding it. It doesn't totally "belong to us".

@dmaicher
Copy link
Contributor

I believe this particular issue was fixed already though: sonata-project/SonataCoreBundle#716

@core23 core23 mentioned this issue Nov 22, 2019
Closed
@jaikdean
Copy link

I believe this particular issue was fixed already though: sonata-project/SonataCoreBundle#716

That fix isn't released, and is on a bundle that is now deprecated. The Sonata bundles typically have quite a slow release cycle, so it'll almost certainly be faster to get this fixed in Symfony.

@er1z
Copy link
Contributor
er1z commented Nov 22, 2019

@nicolas-grekas — what should we do? Make a bugfix release to Symfony? This issue is pretty important because prevents our projects from upgrading to 4.4 LTS.

@xabbuh xabbuh mentioned this issue Nov 22, 2019
Merged
@xabbuh
Copy link
Member
xabbuh commented Nov 22, 2019

see #34513 for the fix

@fabpot fabpot closed this as completed Nov 24, 2019
fabpot added a commit that referenced this issue Nov 24, 2019
@fabpot
bug #34513 [Validator] remove return type declaration from __sleep() …
e757512 
…(xabbuh)

This PR was merged into the 4.4 branch.

Discussion
----------

[Validator] remove return type declaration from __sleep()

| Q             | A
| ------------- | ---
| Branch?       | 4.4
| Bug fix?      | yes
| New feature?  | no
| Deprecations? | no
| Tickets       | Fix #34478
| License       | MIT
| Doc PR        |

Commits
-------

bedad35 remove return type declaration from __sleep()
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
BC Break Bug Status: Needs Review Validator
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
8 participants
@fabpot @nicolas-grekas @jaikdean @dmaicher @er1z @xabbuh @s-duval @carsonbot
0