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

Fix compatibility with PHP 8.1 #101

Merged
merged 2 commits into from
Oct 6, 2021
Merged

Fix compatibility with PHP 8.1 #101

merged 2 commits into from
Oct 6, 2021

Conversation

W0rma
Copy link
Contributor

@W0rma W0rma commented Oct 6, 2021

Q A
Bug fix? no
New feature? no
Doc updated no
BC breaks? no
Deprecations? no
Tests pass? yes
Fixed tickets
License MIT

Run tests against PHP 8.1 and fix deprecation warning because of deprecated Serializeable interface (https://wiki.php.net/rfc/phase_out_serializable)

@W0rma
Copy link
Contributor Author

W0rma commented Oct 6, 2021

The failing "tests-coverage" pipeline seems to be unrelated to this PR

@W0rma W0rma marked this pull request as ready for review October 6, 2021 07:19
@W0rma W0rma changed the title fix deprecation notice regarding Serializable interface in PHP 8.1 Fix compatibility with PHP 8.1 Oct 6, 2021
@goetas
Copy link
Collaborator

goetas commented Oct 6, 2021

thanks!

@goetas goetas merged commit 3e1dc95 into schmittjoh:master Oct 6, 2021
@W0rma W0rma deleted the php81 branch October 25, 2021 06:32
@alexander-schranz
Copy link
Contributor

alexander-schranz commented Nov 1, 2021

Hi,

I'm currently testing @sulu against current dev dependencies. In this case I did find out that the change of the jms/metadata between the current master and 2.5.1 does break serialization of our Localization object.

On 2.5.1 it runs correctly and on 3e1dc95 with this change it fails:

Bildschirmfoto 2021-11-01 um 16 19 32

Does somebody understand why this is now happening. I do not see any error here in the code provided by @W0rma.

/cc @goetas do you have any idea how this change could affect this?

@goetas
Copy link
Collaborator

goetas commented Nov 2, 2021

@alexander-schranz can it be becase this https://github.com/schmittjoh/serializer/blob/master/src/Metadata/PropertyMetadata.php#L213 does not call the parent serialization method?

@alexander-schranz
Copy link
Contributor

@goetas Think we need also there add a __serialize and __unserialize method or what do you think?

@goetas
Copy link
Collaborator

goetas commented Nov 2, 2021

this seems a very subtle change that will require a major release of jms/metadata. Since /src/Metadata/PropertyMetadata.php extends the base PropertyMetadata class, php will call __serialize ignoring the serialize method present in the child class.

@W0rma
Copy link
Contributor Author

W0rma commented Nov 7, 2021

@goetas Should we bump the min PHP version to 7.4 in the new major release? This way we could already stop implementing the deprecated Serializable interface because the magic methods __serialize() and __unserialize() will be recognized automatically (https://wiki.php.net/rfc/custom_object_serialization).

Downside of this approach might be that we need a new major release for jms/serializer as well.

WDYT?

@goetas
Copy link
Collaborator

goetas commented Nov 8, 2021

I think that we can bump the minimum version to 7.4.

Downside of this approach might be that we need a new major release for jms/serializer as well.

Why so? i think that we can still implement Serializable in the jms/serializer library and there still support php 7.2.

The main advantage I see of doing so is that other projects using jms/metadata can drop old php versions, for jms/serializer still want to keep 7.2 as minimum version requirement.

@W0rma W0rma mentioned this pull request Nov 13, 2021
@goetas
Copy link
Collaborator

goetas commented Nov 13, 2021

#105 implements a backward compatible version of this change

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants