Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.7k
[Serializer] Remove support for deprecated signatures#22743
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
dunglas commentedMay 18, 2017
| Q | A |
|---|---|
| Branch? | master |
| Bug fix? | no |
| New feature? | no |
| BC breaks? | no |
| Deprecations? | no |
| Tests pass? | yes |
| Fixed tickets | n/a |
| License | MIT |
| Doc PR | n/a |
nicolas-grekas commentedMay 18, 2017
rebase needed to see tests green |
| * @throws RuntimeException | ||
| */ | ||
| protectedfunctioninstantiateObject(array &$data,$class,array &$context,\ReflectionClass$reflectionClass,$allowedAttributes/*,$format = null*/) | ||
| protectedfunctioninstantiateObject(array &$data,$class,array &$context,\ReflectionClass$reflectionClass,$allowedAttributes, ?string$format =null) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I think adding the?string typehint should be avoided here, as someone might actually have overridden this method without it (as it was not part of the commented argument, nor there is any check on it), right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
IMO it's better to force the user to upgrade his code here, now that PHP 7.1 is required (it wasn't the case when this comment was introduced)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
we have to settle a policy here.
right now, the policy is: no BC break without first a notice in the last minor (3.4)
doing this change does not pass this requirement
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I can add the typehint in the comments of the 3.4 branch. But we should take this opportunity to enforces types.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
let's update the type hint in 3.2 in fact, that should do it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
It does not change the semanticat all, sorry if you see one, but it's a pure matter of coding style.
javiereguiluzMay 18, 2017 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Nico, is true that these two look identical:
publicfunction foo(string$foo =null);publicfunction foo(?string$foo =null);
But I agree with Maxime that they are different because only the second one lets you change the default value without breaking the apps:
// will break for apps who previously used null for the foo argumentpublicfunction foo(string$foo ='bar');// will keep working regardless of the value of the foo argumentpublicfunction foo(?string$foo ='bar');
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
@nicolas-grekas I agree with you on that one that there's no semantic difference here, but that's only the case as long as the default value isnull. Even if redundant, the ability to change the default value without BC break is compelling.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
changing the default valueis a BC break :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
For reference, this thread on PHP internals contains some insights on the topic:
https://externals.io/message/96045
| if (__CLASS__ !==get_class($this)) { | ||
| $r =new \ReflectionMethod($this,__FUNCTION__); | ||
| if (__CLASS__ !==$r->getDeclaringClass()->getName()) { | ||
| @trigger_error(sprintf('Method %s::%s() will have a 6th `$format = null` argument in version 4.0. Not defining it is deprecated since 3.2.',get_class($this),__FUNCTION__),E_USER_DEPRECATED); |
nicolas-grekasMay 18, 2017 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
update in 3.2: > will have a 6thstring $format = null
…n preparation for 4.0 (ogizanagi)This PR was merged into the 3.3 branch.Discussion----------[Security][Serializer][DI] Add new arguments typehints in preparation for 4.0| Q | A| ------------- | ---| Branch? | 3.3| Bug fix? | no| New feature? | no| BC breaks? | no| Deprecations? | no| Tests pass? | yes| Fixed tickets |#22743 (review)| License | MIT| Doc PR | N/ASee#22743 (review) discussion for the motivations.Commits-------b973b30 [Security][Serializer][DI] Add new arguments typehints in preparation for 4.0
…n preparation for 4.0 (ogizanagi)This PR was merged into the 3.3 branch.Discussion----------[Security][Serializer][DI] Add new arguments typehints in preparation for 4.0| Q | A| ------------- | ---| Branch? | 3.3| Bug fix? | no| New feature? | no| BC breaks? | no| Deprecations? | no| Tests pass? | yes| Fixed tickets |symfony/symfony#22743 (review)| License | MIT| Doc PR | N/ASeesymfony/symfony#22743 (review) discussion for the motivations.Commits-------b973b30 [Security][Serializer][DI] Add new arguments typehints in preparation for 4.0
nicolas-grekas commentedMay 22, 2017
rebase needed |
nicolas-grekas commentedMay 22, 2017
CHANGELOG entry missing! |
nicolas-grekas commentedMay 29, 2017
Thank you@dunglas. |
…(dunglas)This PR was merged into the 4.0-dev branch.Discussion----------[Serializer] Remove support for deprecated signatures| Q | A| ------------- | ---| Branch? | master| Bug fix? | no| New feature? | no <!-- don't forget updating src/**/CHANGELOG.md files -->| BC breaks? | no| Deprecations? | no <!-- don't forget updating UPGRADE-*.md files -->| Tests pass? | yes| Fixed tickets | n/a| License | MIT| Doc PR | n/a<!--- Bug fixes must be submitted against the lowest branch where they apply (lowest branches are regularly merged to upper ones so they get the fixes too).- Features and deprecations must be submitted against the master branch.- Please fill in this template according to the PR you're about to submit.- Replace this comment by a description of what your PR is solving.-->Commits-------894f99b [Serializer] Remove support for deprecated signatures
…(chalasr)This PR was merged into the 4.0-dev branch.Discussion----------[Serializer] Implement missing context aware interfaces| Q | A| ------------- | ---| Branch? | 4.0| Bug fix? | no| New feature? | no| BC breaks? | no| Deprecations? | no| Tests pass? | yes| Fixed tickets |#22743| License | MIT| Doc PR | n/aForgot in#22743Commits-------61d796a [Serializer] Implement missing context aware interfaces