Skip to content

Remove a BC break introduced by #2243 #2640

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.

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

Merged
merged 1 commit into from
Mar 26, 2019

Conversation

meyerbaptiste
Copy link
Member

Q A
Bug fix? yes
New feature? no
BC breaks? no
Deprecations? yes
Tests pass? yes
Fixed tickets n/a
License MIT
Doc PR n/a

@meyerbaptiste
Copy link
Member Author

Huum, there is also a BC break in the constructor and more difficult to handle because of the last argument $properties. I think the easiest way is to use a setter instead of the constructor for the $existsParameterName argument. WDYT @api-platform/core-team?
Not so easy to deal with a non-final class associated to an abstract service definition when we want to inject a new argument in the constructor...

@dunglas
Copy link
Member

dunglas commented Mar 23, 2019

What’s the problem with the constructor? We should mark these class @Final.

@teohhanhui
Copy link
Contributor

teohhanhui commented Mar 25, 2019

We cannot change a non-final class to final. That would be a BC break.

Edit: Uhh, you mean the annotation.

@teohhanhui
Copy link
Contributor

We can add the @final annotation, but we should also fix the BC break in the constructor.

@teohhanhui
Copy link
Contributor

This should target 2.4 as it's a bug fix.

@antograssiot
Copy link
Contributor

antograssiot commented Mar 25, 2019

The regression is only in master @teohhanhui

@meyerbaptiste meyerbaptiste force-pushed the fix_bc_break branch 2 times, most recently from 7f761a6 to ce7dfd4 Compare March 25, 2019 14:12
@meyerbaptiste
Copy link
Member Author

I just added the BC for the constructor @api-platform/core-team.

Copy link
Contributor

@antograssiot antograssiot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just need a PHPStan false positive exclusion

@soyuka soyuka merged commit 6980627 into api-platform:master Mar 26, 2019
@meyerbaptiste meyerbaptiste deleted the fix_bc_break branch March 26, 2019 09:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants