[Serializer] Remove support for deprecated signatures #22743
Conversation
dunglas
commented
May 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 |
|
rebase needed to see tests green |
| @@ -302,21 +302,8 @@ protected function getConstructor(array &$data, $class, array &$context, \Reflec | |||
| * | |||
| * @throws RuntimeException | |||
| */ | |||
| protected function instantiateObject(array &$data, $class, array &$context, \ReflectionClass $reflectionClass, $allowedAttributes/*, $format = null*/) | |||
| protected function instantiateObject(array &$data, $class, array &$context, \ReflectionClass $reflectionClass, $allowedAttributes, ?string $format = null) | |||
There was a problem hiding this comment.
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.
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.
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.
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.
let's update the type hint in 3.2 in fact, that should do it
There was a problem hiding this comment.
It does not change the semantic at all, sorry if you see one, but it's a pure matter of coding style.
There was a problem hiding this comment.
Nico, is true that these two look identical:
public function foo(string $foo = null);
public function 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 argument
public function foo(string $foo ='bar');
// will keep working regardless of the value of the foo argument
public function foo(?string $foo = 'bar');There was a problem hiding this comment.
@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 is null. Even if redundant, the ability to change the default value without BC break is compelling.
There was a problem hiding this comment.
changing the default value is a BC break :)
There was a problem hiding this comment.
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); |
There was a problem hiding this comment.
update in 3.2: > will have a 6th string $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/A See #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/A See symfony/symfony#22743 (review) discussion for the motivations. Commits ------- b973b30 [Security][Serializer][DI] Add new arguments typehints in preparation for 4.0
|
rebase needed |
|
CHANGELOG entry missing! |
|
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/a Forgot in #22743 Commits ------- 61d796a [Serializer] Implement missing context aware interfaces