Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.7k
[Routing] remove deprecations#31792
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
53afdc0 toc1a8344Compare| /** | ||
| * @internal since Symfony 4.3, will be removed in Symfony 5 as the class won't implement Serializable anymore | ||
| */ | ||
| publicfunctionserialize() |
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 removing this but keeping the new magic serialization methods means that the deserialization breaks when upgrading php to 7.4. so persisted routes will not work anymore when upgrading php. I think that was one of the main reasons why we implemented Serializable in the first place.
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.
So, we can keep them for a smoother transition, and keep only@internal then, would that be ok to you?
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.
Oh, let's make them reallyfinal to close this a bit more?
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.
If we make them final Drupal can't overwrite them anymore which is what they do inhttps://api.drupal.org/api/drupal/core%21lib%21Drupal%21Core%21Routing%21CompiledRoute.php/class/CompiledRoute/8.2.x
I don't see what else they could do then. It would make it impossible to adjust serialization in a subclass.
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.
@nikic Inhttps://wiki.php.net/rfc/custom_object_serialization you say
For this reason, code using parent::serialize() is generally broken
But from what I see the implementation in Drupal calling parent::serialize does not seem broken
publicfunctionserialize() {$data =unserialize(parent::serialize());$data['fit'] =$this->fit;$data['patternOutline'] =$this->patternOutline;$data['numParts'] =$this->numParts;returnserialize($data); }publicfunctionunserialize($serialized) {parent::unserialize($serialized);$data =unserialize($serialized);$this->fit =$data['fit'];$this->patternOutline =$data['patternOutline'];$this->numParts =$data['numParts']; }
It's not using backreferences but if it was, this approach usingunserialize(parent::serialize() seems fine.
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.
As long as we need to support php version < 7.4, don't don't see a solution without \Serializable. So I would propose to revert#30286 for the routing component.
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 provide__serialize and__unserialize as extension points. Note that yes, routes are exposed to the issue. The reason is that options/defaults can be anything, like objects with these references that break. Drupal is doing that (objects there).
This means to me we should keep implementing Serializable, but make the corresponding method@internal - and tell ppl to use__serialize and__unserialize as extension points instead.
This will provide FC/BC.
c1a8344 toa1ce27cCompare…(Tobion)This PR was merged into the 4.3 branch.Discussion----------[Routing] revert deprecation of Serializable in routing| Q | A| ------------- | ---| Branch? | 4.3| Bug fix? | no| New feature? | no <!-- please update src/**/CHANGELOG.md files -->| BC breaks? | no <!-- seehttps://symfony.com/bc -->| Deprecations? | no <!-- please update UPGRADE-*.md and src/**/CHANGELOG.md files -->| Tests pass? | yes <!-- please add some, will be required by reviewers -->| Fixed tickets || License | MIT| Doc PR |we still need to implement Serializable as long as we support PHP < 7.4. otherwise serialized data in php 7.2 would not work anymore when people upgrade to php 7.4. see discussion in#31792 (comment)partly reverts#30286Commits-------d32a295 [Routing] revert deprecation of Serializable in routing
a1ce27c toc7dc368Comparec7dc368 to5beb5f8CompareTobion commentedJun 5, 2019
Rebased and ready |
nicolas-grekas commentedJun 5, 2019
Thank you@Tobion. |
This PR was merged into the 5.0-dev branch.Discussion----------[Routing] remove deprecations| Q | A| ------------- | ---| Branch? | master| Bug fix? | no| New feature? | no <!-- please update src/**/CHANGELOG.md files -->| BC breaks? | yes <!-- seehttps://symfony.com/bc -->| Deprecations? | no <!-- please update UPGRADE-*.md and src/**/CHANGELOG.md files -->| Tests pass? | yes <!-- please add some, will be required by reviewers -->| Fixed tickets || License | MIT| Doc PR |Ref.#30249,#30286,#24894Commits-------5beb5f8 [Routing] remove deprecations
This PR was merged into the 5.0-dev branch.Discussion----------[Routing] remove dead fixtures| Q | A| ------------- | ---| Branch? | master| Bug fix? | no| New feature? | no| BC breaks? | no| Deprecations? | no| Tests pass? | yes| Fixed tickets | -| License | MIT| Doc PR | -Follows#31792Commits-------181aa02 [Routing] remove dead fixtures
Ref.#30249,#30286,#24894