#53299 closed enhancement (fixed)
[PHP 8.1] Update `is_serialized` function to accept Enums
Reported by: |
|
Owned by: |
|
---|---|---|---|
Milestone: | 6.1 | Priority: | normal |
Severity: | normal | Version: | |
Component: | General | Keywords: | php81 has-patch has-unit-tests add-to-field-guide |
Focuses: | Cc: |
Description
PHP 8.1 finally brings Enums! It is not backwards compatible with older PHP versions, so it will be a long time before we can make a meaningful use of it with WordPress.
While reading #53295 , I had a concern with the WordPress's `is_serialized` function. It tries to meticulously validate a serialized string. At the same time, Enums can be serialized, and uses its own Serialized text representation for Enums. I think we should update the function to account for the new E
identifier.
Ticket #53295 suggests to not make a precise validation in the first place if it deems safer, but this ticket is more about updating the existing rules to accommodate the new symbol.
Thank you.
Change History (29)
This ticket was mentioned in PR #2272 on WordPress/wordpress-develop by konradyoast.
3 years ago
#4
- Keywords has-patch added
Trac ticket: https://core.trac.wordpress.org/ticket/53299
#5
@
3 years ago
@hellofromTonya @jrf do either of you have any bandwidth to review this? Some notes about suitable unit tests would be lovely too.
As serialization has a somewhat exciting history with regards to security, it would be ideal to get this in quite soon if it is suitable for core.
#6
@
3 years ago
- Keywords early dev-feedback needs-unit-tests added
- Milestone changed from 6.0 to 6.1
As 6.0 Beta 1 was released yesterday and this still needs review and unit tests, I'm moving this ticket to the 6.1 milestone, marking for attention early
in the next cycle as indicated in this comment.
Also adding the dev-feedback
and needs-unit-tests
keyword to accurately reflect the ticket's current state.
This ticket was mentioned in Slack in #core by costdev. View the logs.
3 years ago
d-claassen commented on PR #2272:
3 years ago
#8
I've added several tests to confirm existing behaviour of the is_serialize()
function. I've based the input for these tests on the existing tests for `is_serialized_string`.
To test the Enum serialization, I've created and serialized an Enum on 3v4l.org ((link)https://3v4l.org/J6h8o). The given input and result there are shown below for PHP 8.1, which confirms the case for the E
character that's added in the is_serialize()
function.
<?php enum Foo { case bar; } echo serialize( Foo::bar ); // E:7:"Foo:bar";
#9
@
3 years ago
Just to improve on the code block in above comment, the input and result on PHP 8.1 when serializing an Enum looks like this:
<?php enum Foo { case bar; } echo serialize( Foo::bar ); // E:7:"Foo:bar";
This ticket was mentioned in Slack in #core by costdev. View the logs.
3 years ago
#11
@
3 years ago
- Keywords has-unit-tests changes-requested added; needs-unit-tests removed
Thanks @dennisatyoast!
I've left a review on the PR with some suggestions for consistency in the test suite and some additional datasets for more thorough testing to protect BC.
#12
@
3 years ago
Thanks for the feedback, @costdev! I’ve pushed additional commits to the PR to apply the provided suggestions.
This ticket was mentioned in Slack in #core by audrasjb. View the logs.
3 years ago
This ticket was mentioned in Slack in #core by audrasjb. View the logs.
3 years ago
This ticket was mentioned in Slack in #core by chaion07. View the logs.
3 years ago
#16
@
3 years ago
- Keywords changes-requested removed
This ticket was discussed in the recent bug scrub.
@dennisatyoast Sorry for the delay in responding to the updated PR. I've re-reviewed and approved the PR. Removing changes-requested
.
Is there anything else needed for this ticket before adding the commit
keyword? Another reviewer perhaps?
Props to @markparnell, @cu121 and @chaion07.
This ticket was mentioned in Slack in #core by chaion07. View the logs.
3 years ago
This ticket was mentioned in Slack in #core by chaion07. View the logs.
3 years ago
#20
@
3 years ago
- Keywords commit added; dev-feedback removed
This ticket was discussed in the recent bug scrub. The PR looks good and we think this is now a commit
candidate.
Additional props: @chaion07
This ticket was mentioned in Slack in #core by audrasjb. View the logs.
3 years ago
#22
@
3 years ago
- Owner set to audrasjb
- Status changed from new to reviewing
As per today's bugscrub, I'm self assigning this ticket for a final review before commit
.
This ticket was mentioned in Slack in #core by audrasjb. View the logs.
3 years ago
3 years ago
#26
committed in https://core.trac.wordpress.org/changeset/53886
With 5.9 feature freeze being tomorrow and no progress on this ticket, ran out of time to introduce Enums. Moving to
Future Release
as the next milestone is not yet available. But once it is, let's explore this feature in the next cycle (which starts in a little over a week).