Skip to content

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

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

Closed

Conversation

fancyweb
Copy link
Contributor

@fancyweb 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
Copy link
Member

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
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
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
Copy link
Member

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
Copy link
Member

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 discourage-using-serializable- branch from 76591e0 to e3ed9fc Compare April 8, 2019 15:38
@fancyweb
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 discourage-using-serializable- branch from e3ed9fc to 7b10f7c Compare April 8, 2019 15:52
@@ -313,6 +316,17 @@ public function checkAnnotations(\ReflectionClass $refl, $class)
return $deprecations;
}

if (
isset($parentAndOwnInterfaces['Serializable']) &&
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@fancyweb fancyweb force-pushed the discourage-using-serializable- branch 2 times, most recently from e1caa30 to 0f6ada1 Compare April 11, 2019 20:35
`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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

for classes and interfaces that implement or extend

@nicolas-grekas nicolas-grekas force-pushed the discourage-using-serializable- branch from 0f6ada1 to f0e400d Compare April 15, 2019 13:28
@nicolas-grekas
Copy link
Member

@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
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?

@fancyweb
Copy link
Contributor Author

@nicolas-grekas I'm doing a tour of my open PR.

Is this one going somewhere or should we just close? The amount of time we both spent on this might not be worth it in the end 😄

@nicolas-grekas
Copy link
Member

Agreed :)
Thanks for giving it a try!

@fancyweb fancyweb deleted the discourage-using-serializable- branch June 28, 2019 14:34
@nicolas-grekas nicolas-grekas modified the milestones: next, 4.4 Oct 27, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants