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() on DefaultTestResultCache #4662

Conversation

derrabus
Copy link
Contributor

@derrabus derrabus commented May 3, 2021

The Serializable interface is being phased out as described in https://wiki.php.net/rfc/phase_out_serializable

The consequence is that PHP 8.1 will trigger a deprecation notice if a class implements Serializable unless it also implements the magic methods __serialize() and __unserialize(). Those have been introduced with PHP 7.4 and are meant to replace the old Serializable mechanism.

PHPUnit's DefaultTestResultCache is affected by this change. This PR proposes to implement __serialize() and __unserialize() to avoid the deprecation.

Because this changes the output of the serialization slightly, I had to adjust a couple of tests.

@codecov
Copy link

codecov bot commented May 3, 2021

Codecov Report

Merging #4662 (d71643f) into 8.5 (2c47a3e) will increase coverage by 0.00%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff            @@
##                8.5    #4662   +/-   ##
=========================================
  Coverage     84.21%   84.21%           
- Complexity     3981     3983    +2     
=========================================
  Files           154      154           
  Lines         10185    10187    +2     
=========================================
+ Hits           8577     8579    +2     
  Misses         1608     1608           
Impacted Files Coverage Δ Complexity Δ
src/Runner/DefaultTestResultCache.php 93.75% <100.00%> (+0.20%) 31.00 <7.00> (+2.00)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2c47a3e...d71643f. Read the comment docs.

@sebastianbergmann
Copy link
Owner

Thank you for working on this, Alexander.

However, it may not be as simple as "just" updating the tests. Is the new code able to load from a cache file created by the old code? It is is: great! If it's not, we need to adapt the loader to not crash when loading a file it does not understand and treat that situation as a cache miss.

@derrabus
Copy link
Contributor Author

derrabus commented May 3, 2021

Is the new code able to load from a cache file created by the old code?

It should be. There is a test in this PR that performs an unserialize() with the old format.

@sebastianbergmann
Copy link
Owner

It should be. There is a test in this PR that performs an unserialize() with the old format.

Thank you for pointing that out, I did not have time yet to (closely) look at the proposed changes.

@sebastianbergmann sebastianbergmann self-assigned this May 3, 2021
@sebastianbergmann sebastianbergmann added the type/change-in-php-requires-adaptation A change in PHP requires a change in PHPUnit label May 3, 2021
@derrabus
Copy link
Contributor Author

derrabus commented May 3, 2021

However, it does not work the other way round: PHP 7.3 won't be able to unserialize the format generated by PHP 7.4.

@sebastianbergmann
Copy link
Owner

Good to know, thanks. Only goes to show that using serialize() was not a great choice to begin with.

@sebastianbergmann
Copy link
Owner

Thank you for your contribution. I appreciate the time you invested in preparing this pull request. However, I have decided not to merge it. Instead, I have changed the implementation of DefaultTestResultCache to not use serialize() / unserialize() at all.

@derrabus derrabus deleted the improvement/serializable-deprecation branch May 3, 2021 13:16
@derrabus
Copy link
Contributor Author

derrabus commented May 3, 2021

No worries. Your way is certainly the better solution in the long run!

ruudk added a commit to LongRunning/LongRunning that referenced this pull request Nov 25, 2021
ruudk added a commit to LongRunning/LongRunning that referenced this pull request Nov 25, 2021
ruudk added a commit to LongRunning/LongRunning that referenced this pull request Nov 25, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type/change-in-php-requires-adaptation A change in PHP requires a change in PHPUnit
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants