Make WordPress Core

Opened 4 years ago

Closed 3 years ago

Last modified 2 years ago

#53299 closed enhancement (fixed)

[PHP 8.1] Update `is_serialized` function to accept Enums

Reported by: ayeshrajans's profile ayeshrajans Owned by: audrasjb's profile audrasjb
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)

#1 @SergeyBiryukov
4 years ago

  • Milestone changed from Awaiting Review to 5.9

#2 @hellofromTonya
3 years ago

  • Milestone changed from 5.9 to Future Release

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).

#3 @hellofromTonya
3 years ago

  • Milestone changed from Future Release to 6.0

Moving into 6.0.

This ticket was mentioned in PR #2272 on WordPress/wordpress-develop by konradyoast.


3 years ago
#4

  • Keywords has-patch added

#5 @peterwilsoncc
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 @costdev
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 @dennisatyoast
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 @costdev
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 @dennisatyoast
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 @costdev
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

#18 @mukesh27
3 years ago

The PR looks good to me. approved the PR.

This ticket was mentioned in Slack in #core by chaion07. View the logs.


3 years ago

#20 @costdev
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 @audrasjb
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

#24 @audrasjb
3 years ago

  • Resolution set to fixed
  • Status changed from reviewing to closed

In 53886:

Formatting: Add support for Enums in is_serialized().

This changeset adds support for Enums in is_serialized(). It also adds new unit tests for this function.

Props ayeshrajans, konradyoast, peterwilsoncc, costdev, dennisatyoast, mukesh27.
Fixes #53299.

#25 @audrasjb
3 years ago

  • Keywords early commit removed

#27 @milana_cap
2 years ago

  • Keywords add-to-field-guide added

This ticket was mentioned in Slack in #hosting-community by javier. View the logs.


2 years ago

This ticket was mentioned in Slack in #hosting-community by jadonn. View the logs.


2 years ago

Note: See TracTickets for help on using tickets.