Skip to content
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

[Debug][DebugClassLoader] Discourage using the \Serializable interface #30987

Open
wants to merge 2 commits into
base: master
from

Conversation

Projects
None yet
5 participants
@fancyweb
Copy link
Contributor

fancyweb commented Apr 7, 2019

Q A
Branch? master
Bug fix? no
New feature? yes
BC breaks? no
Deprecations? no
Tests pass? yes
Fixed tickets #30304
License MIT
Doc PR -

#eufossa

The deprecation is triggered only if the class implementing \Serializable does not already implement the future serialization mechanism (with __serialize and __unserialize).

The case we don't handle (yet) is if your class implements an interface that extends \Serializable.

Needs #30965 and twigphp/Twig#2927 for tests to pass.

@derrabus

This comment has been minimized.

Copy link
Contributor

derrabus commented Apr 7, 2019

I'm unsure if DebugClassLoader should really do that. If the interface gets deprecated (it hasn't been yet, afaik), php will trigger its own deprecation notice. Also, this change will potentially product quite a few deprecation notices for third-party code that an application developer can't do much about.

@fancyweb

This comment has been minimized.

Copy link
Contributor Author

fancyweb commented Apr 7, 2019

The DebugClassLoader already produces a lot of deprecations for third-party code. That's ok because that's not controllable and that can push people to contribute to fix issues.

Also the \Serializable interface is definitely going to be removed (according to the content of the RFC).

So I think the question here is more something like : do we want the DebugClassLoader to report deprecations before they really happen to help users to be "future proof" ? Or in this case, do we want the DebugClassLoader to try to force a good practice to avoid potential problems ?

@chalasr

This comment has been minimized.

Copy link
Member

chalasr commented Apr 7, 2019

The DebugClassLoader already produces a lot of deprecations for third-party code

The fact it is third-party or userland code makes no difference as it triggers based on specific markers such as @deprecated only, meaning it's a feature you can opt-in via some phpdoc annotations.
This makes it discourage the use of a native PHP interface based on our own opinion/experience/usage, it could do the same for e.g. eval() in userland: that's not Symfony's scope to me, it's PHP one.

@derrabus

This comment has been minimized.

Copy link
Contributor

derrabus commented Apr 7, 2019

The RFC …

is written with a subsequent deprecation and removal of the severely broken Serializable interface in mind.

https://wiki.php.net/rfc/custom_object_serialization

In other words, there is the intention of deprecating the interface some time in the future. There is (to my knowledge) no RFC yet to actually deprecate it. So even if we would see it as the responsibility of DebugClassLoader to issue deprecation notices for this interface, implementing it now would be too early imho.

@nicolas-grekas

This comment has been minimized.

Copy link
Member

nicolas-grekas commented Apr 7, 2019

So let's add a check to enforce this in the Symfony namespace for now then.

@nicolas-grekas nicolas-grekas added this to the next milestone Apr 7, 2019

@fancyweb fancyweb force-pushed the fancyweb:discourage-using-serializable- branch from 76591e0 to e3ed9fc Apr 8, 2019

@fancyweb

This comment has been minimized.

Copy link
Contributor Author

fancyweb commented Apr 8, 2019

@nicolas-grekas I restricted it to the Symfony namespace + I applied your idea to handle interfaces (but will need #31011 for tests to pass).

@fancyweb fancyweb force-pushed the fancyweb:discourage-using-serializable- branch from e3ed9fc to 7b10f7c Apr 8, 2019

Show resolved Hide resolved src/Symfony/Component/Debug/CHANGELOG.md Outdated
Show resolved Hide resolved src/Symfony/Component/Debug/DebugClassLoader.php Outdated
Show resolved Hide resolved src/Symfony/Component/Debug/DebugClassLoader.php Outdated
@@ -313,6 +316,17 @@ public function checkAnnotations(\ReflectionClass $refl, $class)
return $deprecations;
}
if (
isset($parentAndOwnInterfaces['Serializable']) &&

This comment has been minimized.

Copy link
@nicolas-grekas

nicolas-grekas Apr 8, 2019

Member

this should be an "is_a" check, so that this is triggered for classes whose parent interfaces extends Serializable

This comment has been minimized.

Copy link
@fancyweb

fancyweb Apr 8, 2019

Author Contributor

The problem with a is_a check is that it will trigger for classes extending \ArrayIterator for example. And we're not gonna stop extending this core class in the core I guess.

This comment has been minimized.

Copy link
@fancyweb

fancyweb Apr 11, 2019

Author Contributor

@nicolas-grekas Thanks for your review. I have addressed all comments. This is the last one 😄

Show resolved Hide resolved src/Symfony/Component/Debug/DebugClassLoader.php Outdated
Show resolved Hide resolved src/Symfony/Component/Debug/DebugClassLoader.php Outdated

@fancyweb fancyweb force-pushed the fancyweb:discourage-using-serializable- branch 2 times, most recently from e1caa30 to 0f6ada1 Apr 11, 2019

`Exception\FlattenException::getTraceAsString` to increase compatibility to php
exception objects
`Exception\FlattenException::getTraceAsString` to increase compatibility to php exception objects
* added a deprecation for classes and interface that implements or extends the `\Serializable` interface

This comment has been minimized.

Copy link
@nicolas-grekas

nicolas-grekas Apr 15, 2019

Member

for classes and interfaces that implement or extend

@nicolas-grekas nicolas-grekas force-pushed the fancyweb:discourage-using-serializable- branch from 0f6ada1 to f0e400d Apr 15, 2019

@nicolas-grekas

This comment has been minimized.

Copy link
Member

nicolas-grekas commented Apr 15, 2019

@fancyweb I rebased + added a 2nd commit on this PR. I did not update tests so that you can inspect the effect of the changes. Please have a look and squash+fix tests if you're OK. Let's discuss it if not.

@fancyweb

This comment has been minimized.

Copy link
Contributor Author

fancyweb commented Apr 16, 2019

@nicolas-grekas I have just checked your implementation. Three initial tests are failing. The failing tests are on "interfaces" cases.

With the initial implementation, deprecations are reported on interfaces that extends \Serializable, not on the classes that actually implement them. Example :

interface ExtendsSerializable extends \Serializable
{
}

class ImplementsAnInterfaceThatExtendsSerializable implements ExtendsSerializable
{
    public function serialize() {}
    public function unserialize($data) {}
}

With the initial implementation, deprecations are reported on the ExtendsSerializable interface : it should have the __serialize and __unserialize methods (to force their implementations).
With this implementation, deprecations are reported on the ImplementsAnInterfaceThatExtendsSerializable class.

Also, this implementation does fix the \ArrayIterator question but partially. If the final class overrides one of the serialize or unserialize methods, the deprecations are reported. IMHO, that shows an issue of this implementation. Example :

interface ExtendsSerializable extends \Serializable
{
}

class ImplementsAnInterfaceThatExtendsSerializable implements ExtendsSerializable
{
    public function __serialize(): array {}
    public function serialize() {}
    public function __unserialize(array $data): void {}
    public function unserialize($data) {}
}

class AnotherClass extends ImplementsAnInterfaceThatExtendsSerializable
{
    public function serialize() {}
}

Technically, AnotherClass is fine : it does implement the new mechanism (through inheritance). However, with the new implementation, the deprecations are reported for this class. With the initial implementation, deprecations are reported only on the ExtendsSerializable interface. I didn't take the time to see if it's easily fixable.

WDYT of those two points?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.