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

switch to new serialization mechanism #114

Closed
wants to merge 1 commit into from
Closed

Conversation

remicollet
Copy link
Collaborator

@remicollet remicollet commented Jun 11, 2021

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

So this is supported since 7.4, and mandatory in 8.1

Drop Serializable interface usage, and implement new magic methods

NOTICE: this mean incompatibility with previous version and serialized data.

BTW.... serialized data should only be used for local temporary data... should be...

Test suite passes with both 8.0.7 and 8.1.0alpha1

P.S. old implementation is kept where used for toString.

@remicollet remicollet requested a review from m6w6 June 11, 2021 15:09
@codecov
Copy link

codecov bot commented Jun 11, 2021

Codecov Report

Merging #114 (88ef341) into master (28a1848) will decrease coverage by 0.07%.
The diff coverage is 75.40%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #114      +/-   ##
==========================================
- Coverage   85.73%   85.66%   -0.08%     
==========================================
  Files          42       42              
  Lines        9193     9210      +17     
==========================================
+ Hits         7882     7890       +8     
- Misses       1311     1320       +9     
Impacted Files Coverage Δ
src/php_http_message_body.c 84.89% <61.53%> (-1.74%) ⬇️
src/php_http_message.c 87.28% <66.66%> (-0.05%) ⬇️
src/php_http_querystring.c 91.94% <83.33%> (+0.17%) ⬆️
src/php_http_header.c 83.54% <88.88%> (-0.21%) ⬇️
src/php_http_header_parser.c 94.62% <0.00%> (-0.03%) ⬇️

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 28a1848...88ef341. Read the comment docs.

@remicollet
Copy link
Collaborator Author

Notice: I kept old behavior to raise a warning for bad data during unserialization, but probably make sense to raise an exception, like in php-src (ex in Spl)

@m6w6
Copy link
Owner

m6w6 commented Jun 25, 2021

Thanks!

Looks like this requires a new major version, due to dropping Serializable.

@m6w6
Copy link
Owner

m6w6 commented Jun 30, 2021

Quote:

  • In PHP 8.1, declaring an “only Serializable” class will throw a deprecation warning. Other implementations of Serializable will be accepted without a deprecation warning, because libraries supporting PHP < 7.4 will generally need to implement both the old and new mechanisms.
  • In PHP 9.0 the Serializable interface will be removed and unserialize() will reject payloads using the C serialization format. Code needing to support both PHP < 7.4 and PHP >= 9.0 may polyfill the Serializable interface, though it will have no effect on serialization.

So it looks like it would be best to keep both, Serializable and the new magic methods until PHP-9.

@Jan-E
Copy link
Collaborator

Jan-E commented Jul 19, 2021

So it looks like it would be best to keep both, Serializable and the new magic methods until PHP-9.

n00b question: does this imply that supporting ZEND_ACC_NOT_SERIALIZABLE is also recommended? See a comparable issue for the msgpack extension: msgpack/msgpack-php#161

@m6w6 m6w6 closed this Aug 3, 2021
@Jan-E
Copy link
Collaborator

Jan-E commented Aug 3, 2021

FWIW: I am running into new deprecations with PHP 8.1.0 beta 1:

Deprecated: Return type of http\Client::attach(SplObserver $observer) should either be compatible with SplSubject::attach(SplObserver $observer): void, or the #[ReturnTypeWillChange] attribute should be used to temporarily suppress the notice
Deprecated: Return type of http\Client::detach(SplObserver $observer) should either be compatible with SplSubject::detach(SplObserver $observer): void, or the #[ReturnTypeWillChange] attribute should be used to temporarily suppress the notice
Deprecated: Return type of http\Client::notify(?http\Client\Request $request = , $progress = ) should either be compatible with SplSubject::notify(): void, or the #[ReturnTypeWillChange] attribute should be used to temporarily suppress the notice

@m6w6
Copy link
Owner

m6w6 commented Aug 3, 2021

FWIW: I am running into new deprecations with PHP 8.1.0 beta 1:

Deprecated: Return type of http\Client::attach(SplObserver $observer) should either be compatible with SplSubject::attach(SplObserver $observer): void, or the #[ReturnTypeWillChange] attribute should be used to temporarily suppress the notice
Deprecated: Return type of http\Client::detach(SplObserver $observer) should either be compatible with SplSubject::detach(SplObserver $observer): void, or the #[ReturnTypeWillChange] attribute should be used to temporarily suppress the notice
Deprecated: Return type of http\Client::notify(?http\Client\Request $request = , $progress = ) should either be compatible with SplSubject::notify(): void, or the #[ReturnTypeWillChange] attribute should be used to temporarily suppress the notice

Yes, I'm not sure how to handle this yet; it probably needs a new major version fixing the method signatures.

m6w6 added a commit that referenced this pull request Aug 30, 2021
* Fixed PHP-8.1 compatibility (see gh issues #114, #115 and #118)
* Fixed cookies failing with libcurl >= 7.77 (see gh issue #116)
* Fixed tests using $_ENV instead of getenv() to find executables in PATH (see gh issue #113)
* Added http\Env::reset(): resets internal HTTP request cache (see gh issue #90)
m6w6 added a commit that referenced this pull request Aug 30, 2021
* Fixed PHP-8.1 compatibility (see gh issues #114, #115 and #118)
* Fixed cookies failing with libcurl >= 7.77 (see gh issue #116)
* Fixed tests using $_ENV instead of getenv() to find executables in PATH (see gh issue #113)
* Added http\Env::reset(): resets internal HTTP request cache (see gh issue #90)
@m6w6 m6w6 deleted the issue-php81 branch June 8, 2022 17:53
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