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

Drop \Serializable implementations #30051

Merged
merged 1 commit into from Feb 17, 2019

Conversation

Projects
None yet
6 participants
@renanbr
Copy link

renanbr commented Jan 31, 2019

Q A
Branch? master
Bug fix? no
New feature? no
BC breaks? yes, it removes \Serializable interface from many classes
Deprecations? no
Tests pass? yes
Fixed tickets n/a
License MIT
Doc PR n/a

This PR replaces Serializable implementations with __sleep() and __wakeup().

Changes touch these components:

  • Config
  • DependencyInjection
  • Form
  • HttpKernel
  • Validator
@renanbr

This comment has been minimized.

Copy link
Author

renanbr commented Jan 31, 2019

This is a work in progress.

There are some issues to be solved.

KernelInterface API

I'm not able to mesure the impact of droping \Serializable from KernelInterface, not sure if we should do that in a minor release.

Tests

Traces into objects

This new serialization strategy injects a trace into the serialized objects.
To make some tests pass I had to clone these objects to keep their original instances clean.
Here the list of tests changed:

  • Symfony\Component\Config\Tests\Resource\ComposerResourceTest:testSerializeUnserialize()
  • Symfony\Component\DependencyInjection\Tests\Config\ContainerParametersResourceTest:testSerializeUnserialize()
  • Symfony\Component\Routing\Tests\RouteTest:testSerialize()
  • Symfony\Component\Routing\Tests\RouteTest:testSerializeWhenCompiled()
    • This one is interesting because there is a internal object with the same behavior as its parent

Tests realying on serialized strings

Symfony\Component\HttpKernel\Tests\DataCollector\DumpDataCollectorTest relied on internal serialization engine. I've updated it, but it still depends on the internal implementation. I created a ExposedDumpDataCollector class that expose some internal variables for test purposes.

Tests failing

  • Symfony\Component\Routing\Tests\RouteTest:testSerializedRepresentationKeepsWorking()
  • Symfony\Component\Config\Tests\ResourceCheckerConfigCacheTest:testCacheIsNotFreshWhenUnserializeFails()
    • I guess it's related to Symfony\Component\Config\ResourceCheckerConfigCache:safelyUnserialize()

@nicolas-grekas nicolas-grekas added this to the next milestone Feb 1, 2019

@nicolas-grekas

This comment has been minimized.

Copy link
Member

nicolas-grekas commented Feb 1, 2019

I would consider implementing Serializable an internal implementation detail, which means I'm fine removing it on these classes on master. No BC break here :)

@renanbr renanbr force-pushed the renanbr:drop-serializable branch from cbec8ea to 4915643 Feb 1, 2019

@renanbr

This comment has been minimized.

Copy link
Author

renanbr commented Feb 1, 2019

@stof, using __sleep without transient attribute will break serialization for child classes (if parent class has private attributes in the bulk).

In other words: Serializing extended classes won't work anymore.

Real example: I ketp the trasient attribute in the Symfony\Component\Routing\CompiledRoute in order to keep Symfony\Component\Routing\Tests\RouteTest:testSerializeWhenCompiledWithClass() green.

Here is a list of classes that mention private attributes in __sleep():

  • Symfony\Component\Config\Resource\ClassExistenceResource
  • Symfony\Component\Config\Resource\ComposerResource
  • Symfony\Component\Config\Resource\DirectoryResource
  • Symfony\Component\Config\Resource\FileExistenceResource
  • Symfony\Component\Config\Resource\FileResource
  • Symfony\Component\Config\Resource\GlobResource
  • Symfony\Component\Config\Resource\ReflectionClassResource
  • Symfony\Component\DependencyInjection\Config\ContainerParametersResource
  • Symfony\Component\Form\FormError
  • Symfony\Component\HttpKernel\Debug\FileLinkFormatter
  • Symfony\Component\Routing\Route

Please, check if I should revert it for some of them.

Here is the list of classes mentioning only protected attributes in __sleep():

  • Symfony\Component\HttpKernel\DataCollector\DataCollector
  • Symfony\Component\HttpKernel\Kernel

Here is the list of classes I did changed the transient attribute approach:

  • Symfony\Component\Form\Extension\DataCollector\FormDataCollector
  • Symfony\Component\HttpKernel\DataCollector\DumpDataCollector

@nicolas-grekas s/list(...)/[...] done


Tests still failing:

  • Symfony\Component\Config\Test\ResourceCheckerConfigCacheTest::testCacheIsNotFreshWhenUnserializeFails
  • Symfony\Component\Routing\Tests\RouteTest::testSerializedRepresentationKeepsWorking

@renanbr renanbr force-pushed the renanbr:drop-serializable branch 4 times, most recently from 3773e66 to 41855e4 Feb 1, 2019

@nicolas-grekas
Copy link
Member

nicolas-grekas left a comment

LGTM, with a minor comment.
Let's make tests pass and good to go.
We could and should IMHO make all these classes final, in another PR if you're up to.

@nicolas-grekas

This comment has been minimized.

Copy link
Member

nicolas-grekas commented Feb 5, 2019

Actually, we cannot do that for DataCollector as that breaks classes like e.g.
https://github.com/intbizth/toro-admin-bundle/blob/fc85a454fa8c22daffdd6e4ac713b8ea95d23990/DataCollector/ToroDataCollector.php

Maybe split the data-collector related changes to a separate PR to move the rest forward?

@renanbr

This comment has been minimized.

Copy link
Author

renanbr commented Feb 5, 2019

I think we can drop \Serialize nowhere, because related methods become @internal one week ago. We cannot be sure no one is overwriting these methods in other classes 😒

@nicolas-grekas

This comment has been minimized.

Copy link
Member

nicolas-grekas commented Feb 5, 2019

And we still need to move forward, so we need plans :)
Mine would be to drop Serializable from resources and mark them @final because that looks safe enough,
and remove @internal from the base data collector btw, then consider data collectors in a separate PR.

@renanbr renanbr force-pushed the renanbr:drop-serializable branch 2 times, most recently from 069ea63 to b8c6d6d Feb 8, 2019

@renanbr

This comment has been minimized.

Copy link
Author

renanbr commented Feb 8, 2019

Updates:

  • Reverted changes in DataCollector
  • Some classes become @final

❗️ Test failing ResourceCheckerConfigCacheTest::testCacheIsNotFreshWhenUnserializeFails

It seems the strategy applyed in ResourceCheckerConfigCache:safelyUnserialize() does not work with __sleep() payload.

Here is an example where native unserialize() works, and safelyUnserialize() doesn't: https://3v4l.org/0Chn9

@nicolas-grekas nicolas-grekas force-pushed the renanbr:drop-serializable branch from b8c6d6d to aa137e9 Feb 8, 2019

@chalasr

This comment has been minimized.

Copy link
Member

chalasr commented Feb 11, 2019

OK for dropping \Serializable on my side but still, it is a BC break that should be mentioned in CHANGELOG/UPGRADE files IMHO. Classes made @final may also be documented as anything that triggers notices.

@nicolas-grekas nicolas-grekas force-pushed the renanbr:drop-serializable branch from aa137e9 to 0fb8d88 Feb 17, 2019

@renanbr renanbr requested a review from xabbuh as a code owner Feb 17, 2019

@nicolas-grekas nicolas-grekas force-pushed the renanbr:drop-serializable branch from 0fb8d88 to f8bf973 Feb 17, 2019

@nicolas-grekas

This comment has been minimized.

Copy link
Member

nicolas-grekas commented Feb 17, 2019

Changelogs updated. Not sure about upgrade files, so I didn't update them.

@nicolas-grekas

This comment has been minimized.

Copy link
Member

nicolas-grekas commented Feb 17, 2019

Thank you @renanbr.

@nicolas-grekas nicolas-grekas merged commit f8bf973 into symfony:master Feb 17, 2019

3 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
fabbot.io Your code looks good.
Details

nicolas-grekas added a commit that referenced this pull request Feb 17, 2019

feature #30051 Drop \Serializable implementations (renanbr)
This PR was merged into the 4.3-dev branch.

Discussion
----------

Drop \Serializable implementations

| Q             | A
| ------------- | ---
| Branch?       | master
| Bug fix?      | no
| New feature?  | no
| BC breaks?    | yes, it removes `\Serializable` interface from many classes
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | n/a
| License       | MIT
| Doc PR        | n/a

This PR replaces [Serializable](https://secure.php.net/serializable) implementations with [__sleep() and __wakeup()](http://php.net/manual/en/language.oop5.magic.php#language.oop5.magic.sleep).

Changes touch these components:

- Config
- DependencyInjection
- Form
- HttpKernel
- Validator

Commits
-------

f8bf973 Drop \Serializable
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment