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

Implement __serialize to avoid deprecation warning on php 8.2 #80

Merged
merged 2 commits into from
Jan 22, 2024
Merged

Implement __serialize to avoid deprecation warning on php 8.2 #80

merged 2 commits into from
Jan 22, 2024

Conversation

nicosp
Copy link
Contributor

@nicosp nicosp commented Jan 12, 2024

Fixes the following on PHP 8.2:

AuditStash\Event\AuditUpdateEvent implements the Serializable interface, which is deprecated. Implement __serialize() and __unserialize() instead (or in addition, if support for old PHP versions is necessary)

@cnizzardini cnizzardini added this to the 3.2.1 milestone Jan 12, 2024
@dereuromark
Copy link
Collaborator

This does seem to miss a test
But fair enough

@nicosp
Copy link
Contributor Author

nicosp commented Jan 22, 2024

This does seem to miss a test But fair enough

I thought this was covered as part of testing with php 8.2.

@dereuromark
Copy link
Collaborator

dereuromark commented Jan 22, 2024

dont think so, it only says so far the class is valid (no syntax error). no assert on the method returns yet.

@nicosp
Copy link
Contributor Author

nicosp commented Jan 22, 2024

dont think so, it only says so far the class is valid (no syntax error). no assert on the method returns yet.

I might be missing something but the tests are:

$serialized = serialize($event);
$this->assertEquals($event, unserialize($serialized));

My understanding is that php 8.2 will automarically use the __ methods while previous versions will use the non-underscored versions.

@dereuromark
Copy link
Collaborator

OK, in that case you seem to be right.

@dereuromark dereuromark merged commit 54b5135 into lorenzo:3.x Jan 22, 2024
13 checks passed
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