PHP 8.1: syntax error due to new 'readonly' property

[haosun]

New 'readonly' property modifier is introduced to PHP 8.1 after commit 6780aaa5[1]. That means, 'readonly' becomes one keyword.

However, there is a function named 'readonly' in wordpress, which would lead to a syntax error, as shown in the following error message:

Parse error: syntax error, unexpected token "readonly", expecting "(" in /var/www/html/php-bench/wordpress/wp-includes/general-template.php on line 4823

[1] ​https://github.com/php/php-src/commit/6780aaa5

[knutsp]

Hello @haosun and welcome to Trac!

Nice find. Nasty thing.

Would this work?

1. Rename readonly() to wp_readonly()
2. If PHP<8.1 : function readonly() calls wp_readonly()

This function has "friends", like checked(). Also alias them for some consistency?

[haosun]

Replying to knutsp:

Hello @haosun and welcome to Trac!

Nice find. Nasty thing.

Would this work?

1. Rename readonly() to wp_readonly()
2. If PHP<8.1 : function readonly() calls wp_readonly()

This function has "friends", like checked(). Also alias them for some consistency?

Hi @knutsp Thanks for your reply.
I'm afraid I didn't fully get your point.

As shown in the error msg, 'readonly' function is defined in wp-includes/general-template.php on line 4823

function readonly( $readonly, $current = true, $echo = true ) {
	return __checked_selected_helper( $readonly, $current, $echo, 'readonly' );
}

Function readonly calls __checked_selected_helper, instead of wp_readonly().

I suppose one straightforward fix is to rename function 'readonly' at definition site and all the use sites. However, as I grepped by "readonly(", I didn't find any call site of this function. I'm not sure if I missed something.

My workaround:
I simply renamed 'function readonly(' to 'function readonly_workaround(', and yes this syntax error is gone. But I suspect it's incomplete because I guess there must exist use sites of function 'readonly' but I missed.

[knutsp]

Function 'readonly' calls 'checked_selected_helper', instead of 'wp_readonly()'.

I know. I meant (by 'calls') that the readonly() alias just should call the new wp_readonly(), and only be defined in case PHP < 8.1.

[swissspidy]

Interestingly I couldn't find any use of this function in WordPress nor any WordPress plugin or theme (via ​https://wpdirectory.net/). Looks like it was just added out of convenience/consistency in #16886.

Renaming to wp_readonly() and making readonly() an alias of that on PHP < 8.1 sounds reasonable as long as it doesn't cause syntax errors anymore. It should be marked as deprecated though.

[haosun]

Thanks for your replies. @swissspidy and @knutsp

Here is some background. I'm working on developing PHP JIT 8.1, and I used ngix+mysql+wrk+wordpress to test the performance of PHP JIT. And this error is exposed just after new 'readonly' property is introduced.

Interestingly I couldn't find any use of this function in WordPress nor any WordPress plugin or theme (via ​https://wpdirectory.net/). Looks like it was just added out of convenience/consistency in #16886.

If so, I think my workaround would work, i.e. simply renaming readonly() to another name.
However, based on my testing, this workaround would affect the performance.

Note that I used the PHP 8.1 version just before 'readonly' property is introduced, and run the performance testing two times. In the first time, I didn't change wordpress, and in the second time, I renamed 'readonly' function to 'readonly_workaround' (in the same way as you said). But I found the performance data is downgraded from 174.51 requests/s to 154.31 requests/s.

That's why I suspected I might miss some use sites of function 'readonly'. But I'm not sure anyway.
Do you have any hints for this performance regression? Thanks.

---

Renaming to wp_readonly() and making readonly() an alias of that on PHP < 8.1 sounds reasonable as long as it doesn't cause syntax errors anymore. It should be marked as deprecated though.

May I ask one simple question? How can I define an alias? Could you show me one example? I'm not familiar with PHP actually. Thanks in advance.

[knutsp]

How can I define an alias? Could you show me one example?

<?php
function canonical_name( $argslist ) {
    // whetever
    return $something;
}

function alias_of_above( $argslist ){
    return canonical_name( $argslist );
}

[haosun]

Replying to knutsp:

How can I define an alias? Could you show me one example?

<?php
function canonical_name( $argslist ) {
    // whetever
    return $something;
}

function alias_of_above( $argslist ){
    return canonical_name( $argslist );
}

Thanks.

On PHP < 8.1, I suppose the reason to add this alias is to keep consistent with other code, right?

[ayeshrajans]

Thanks for opening this ticket, @haosun.
readonly indeed is a reserved token in PHP 8.1, so we have no choice but to rename it.

I think simply renaming the function to wp_readonly (or something more meaningful TBH) would do; this will be a breaking change no matter how we like to see it. It will be a less severe one though, because thanks to @swissspidy, we know there are at least no such calls in public repositories exposed to wpdirectory.

[swissspidy]

There is still a possibility that this function is used somewhere, so simply removing it would be a breaking change. Hence the advocacy for an alias.

Basically something like this:

<?php

// In wp-includes/general-template.php:

/**
 * Outputs the HTML readonly attribute.
 *
 * Compares the first two arguments and if identical marks as readonly
 *
 * @since 5.9.0
 *
 * @param mixed $readonly One of the values to compare
 * @param mixed $current  (true) The other value to compare if not just true
 * @param bool  $echo     Whether to echo or just return the string
 * @return string HTML attribute or empty string
 */
function wp_readonly( $readonly, $current = true, $echo = true ) {
        return __checked_selected_helper( $readonly, $current, $echo, 'readonly' );
}

// In wp-includes/deprecated.php:

if (PHP_VERSION_ID <= 80100) {
        /**
         * Outputs the HTML readonly attribute.
         *
         * Compares the first two arguments and if identical marks as readonly
         *
         * @since 4.9.0
         * @deprecated 5.9.0
         *
         * @param mixed $readonly One of the values to compare
         * @param mixed $current  (true) The other value to compare if not just true
         * @param bool  $echo     Whether to echo or just return the string
         * @return string HTML attribute or empty string
         */
        function readonly( $readonly, $current = true, $echo = true ) {
                _deprecated_function( __FUNCTION__, '5.9.0', 'wp_readonly()' );
                return wp_readonly( $readonly, $current, $echo );
        }
}

@haosun Could you perhaps test this on your setup to see if it works?

---

Do you have any hints for this performance regression? Thanks.

I don't think it's related. You would need to run these tests multiple times anyway to get a more meaningful result.

[knutsp]

On PHP < 8.1, I suppose the reason to add this alias is to keep consistent with other code, right?

Because some theme, plugin or site specific code in the wild might use the (old) function when running PHP 5.6 - 8.0, it's needed not to cause a fatal error. We can't expect those devs to write PHP 8.1 compatible code in many, many years.

Btw: I wonder if just enclosing the function in an if statement is enough. Maybe we have to include a file conditionally?

[jrf]

I know. I meant (by 'calls') that the readonly() alias just should call the new wp_readonly(), and only be defined in case PHP < 8.1.

Renaming to wp_readonly() and making readonly() an alias of that on PHP < 8.1 sounds reasonable as long as it doesn't cause syntax errors anymore. It should be marked as deprecated though.

I've read through the discussion so far and would like to add some observations:

1. This function is used - at least in userland code -, though luckily not very often. Here is a ​WP Directory plugin search showing this. Lots of false positives for JS code, but also some real uses and even polyfill declarations for when WP didn't have the function yet. A ​Theme Search for the same did not yield any valid results.
2. While this will be even more difficult to search for, the function may also be used as a callback via add_filter(). To be honest, I wouldn't be surprised if that type of use exists in the WP Core codebase, though I haven't done a search for it (yet). In that case, in PHP 8.1, I expect this will result in a run-time fatal error when readonly gets called via apply_filters().
3. A plain renaming of the function is a BC-break which is unacceptable in the WP landscape as it will break integrations.
4. Aliasing the function in the manner @swissspidy suggest in wp-includes/deprecated.php will not work as readonly will now be full keyword in PHP, which means that loading the file with the original deprecated function would yield a parse error in PHP 8.1, i.e. result in a white screen of death.

Btw: I wonder if just enclosing the function in an if statement is enough. Maybe we have to include a file conditionally?

So yes, to move forward, I'd like to suggest the following:

1. Rename the function in place.
2. Add a new file which only contains the old, deprecated readonly function and - this is the important part - only load this file conditionally when PHP < 8.1 is detected.

Plugins/themes which use the function and want to support PHP 8.1 will need to switch to the new function name ASAP, as calls to the readonly() function in their code on PHP 8.1 will be a parse error just the same.

This should get a prominent mention in the WP 5.9 dev-note about PHP 8.1.

[jrf]

Some example code which could be used for the dev-note for how plugins can make their code WP cross-version compatible as well as PHP cross-version compatible:

<?php
// Option 1: polyfill the new function (preferred solution).
if ( ! function_exists( 'wp_readonly' ) ) {
    function wp_readonly( $readonly, $current = true, $echo = true ) {
        return __checked_selected_helper( $readonly, $current, $echo, 'readonly' );
    }
}

// Option 2: conditionally call the function.
if ( function_exists( 'wp_readonly' ) ) {
    $code = wp_readonly( $readonly, $current, $echo );
} elseif ( PHP_VERSION_ID < 80100 ) {
    // Using `call_user_func()` prevents this code from being a parse error.
    $code = call_user_func( 'readonly', $readonly, $current, $echo );
} else {
    $code = __checked_selected_helper( $readonly, $current, $echo, 'readonly' );
}

These code samples should work for WP 2.8 (version in which __checked_selected_helper() was introduced) to current.

[swissspidy]

Thanks @jrf! I feared that was the case re: the syntax error. Putting the function into its own file makes sense and there's precedence for that already in core as well 👍

[ayeshrajans]

Suggesting this patch. It renames readonly to wp_readonly, and conditionally includes a ./PHP_Compat/readonly.php file on PHP < 8.1, which declares a readonly func that calls wp_readonly.

[ayeshrajans]

Last patch errornously said @subpackage oEmbed. Fixed in this one.

[haosun]

Replying to knutsp:

On PHP < 8.1, I suppose the reason to add this alias is to keep consistent with other code, right?

Because some theme, plugin or site specific code in the wild might use the (old) function when running PHP 5.6 - 8.0, it's needed not to cause a fatal error. We can't expect those devs to write PHP 8.1 compatible code in many, many years.

Thanks a lot for your explanation.

[haosun]

@haosun Could you perhaps test this on your setup to see if it works?

@swissspidy Yes. The syntax error is gone if function readonly is renamed or we use the code you provided.

---

Do you have any hints for this performance regression? Thanks.

I don't think it's related. You would need to run these tests multiple times anyway to get a more meaningful result.

I'm afraid the performance regression is still there. I did run the test for multiple times and get the performance data by computing the average value.

As pointed by @jrf, there might exist the use sites of readonly. However we didn't use the new wp_readonly at these use sites. Do you think this might be the root cause of performance regression? Thanks.

[jrf]

@ayeshrajans Thanks for the patch(es).

In general looks good. A few small remarks:

1. I can imagine it might help if the @since tag would still say 4.9.0 with a second @since tag stating that the function was renamed in 5.9.0 and mentioning the old function name. I'm not 100% sure what the convention is for this (if there is one) as renaming functions is so rare in WP due to BC considerations.
2. The PHP_Compat subdirectory name does not comply with the WP Coding Standards. This should be php-compat (lowercase and dash separated).

I'll leave further opinions on the filename/subdirectory name to others.

Are there any tests covering this method which need updating ? And if no: would now be a good time to add them ?

@haosun Regarding the performance impact: fatal errors trump performance considerations any time. Including an extra file will always have a small performance impact, but can't be avoided in this case as we do need to mitigate the BC-break as much as possible.

[SergeyBiryukov]

Replying to jrf:

2. While this will be even more difficult to search for, the function may also be used as a callback via add_filter(). To be honest, I wouldn't be surprised if that type of use exists in the WP Core codebase, though I haven't done a search for it (yet). In that case, in PHP 8.1, I expect this will result in a run-time fatal error when readonly gets called via apply_filters().

Just noting that I have not found any instances of using the function in core.

Replying to jrf:

Are there any tests covering this method which need updating ? And if no: would now be a good time to add them ?

There are some tests for selected() and checked(), see [48907] / #51166.

disabled() and readonly() don't seem to have any tests. Would be a great time to add them :)

[jrf]

Couple more thoughts I've had about this issue and the patch:

1. I think a call to ​`_deprecated_function()` needs to be added to the now deprecated readonly() function.
2. The @deprecated 5.9.0 tag in the docblock should mention the new function name as a replacement.
3. I would like to advocate for this patch to be backported to (at least) a few older WP versions so that themes/plugins which support the latest 3 to 5 WP versions don't have to polyfill the function.

[haosun]

Replying to jrf:

@haosun Regarding the performance impact: fatal errors trump performance considerations any time. Including an extra file will always have a small performance impact, but can't be avoided in this case as we do need to mitigate the BC-break as much as possible.

Thanks for your explanation. I further tested under more machines with different CPUs and found that there is only negligible performance impact.

@ayeshrajans Thanks a lot for your previous patches.
Could you help update the patch with the suggestions from @jrf and @SergeyBiryukov?

[jrf]

This patch is a prerequisite before CI testing against PHP 8.1 can be enabled as the CI build fails in the "Install WordPress" step while running the tools/local-env/scripts/install.js script due to this issue.
​Failing build where this can be seen

[ayeshrajans]

Thanks a lot for the pointers @jrf. I uploaded a new patch with the points addressed.

wp_readonly now has two @since tags:

* @since 4.9.0
* @since 5.9.0 This function was renamed from `readonly`.

[SergeyBiryukov]

Thanks for the updated patch!

I think just @since 5.9.0 would be enough for the new function, otherwise it gives an impression that wp_readonly() was already available in WordPress 4.9 with that exact name, which is not the case.

[jrf]

@ayeshrajans Thanks for that! The only thing I'm missing is 'wp_readonly()' as the $replacement parameter in the call to _deprecated_function().

I'm creating some proper tests now for this function set. Will upload those in a little while.

This PR contains:

• @Ayesh's 53858-3.patch patch with those last two remarks addressed.
• Moves the tests related to the __checked_selected_helper() function and all it's helper functions to its own file.
• Improves those tests to make them feature complete.

Trac ticket: https://core.trac.wordpress.org/ticket/53858

[jrf]

@ayeshrajans @SergeyBiryukov I've opened a PR on GitHub which incorporates @ayeshrajans's patch and improves the tests for the complete set of __checked_selected_helper() related methods.

I've also ran the tests on PHP 8.1 and they pass, so as far as I'm concerned, this should now be good to go, though a critical look is, of course, welcome.

For anyone interested: you may find it educational to look at what I've done with the tests.

[SergeyBiryukov]

In 51586:

Code Modernization: Rename the readonly() function to wp_readonly().

Since PHP 8.1, readonly is a reserved keyword and cannot be used as a function name.

In order to avoid PHP parser errors, the readonly() function was extracted to a separate file and is now only included conditionally on PHP < 8.1.

This commit also:

• Moves the tests for the __checked_selected_helper() function and all the related functions to their own file.
• Switches to named data providers. This makes the output when using the --testdox option more descriptive and is helpful when trying to debug which data set from a data provider failed the test.
• Improves the tests in question to make them feature-complete and expand test coverage.

Props jrf, ayeshrajans, haosun, knutsp, swissspidy, SergeyBiryukov.
Fixes #53858.

[SergeyBiryukov]

Reopening to consider backporting [51586] to the latest stable branch and a few older branches, per comment:22.

[jrf]

Thanks @SergeyBiryukov !

For the backport, I'd like to suggest just adding the wp_readonly() function declaration to the general-template.php file. That way plugins/themes which support multiple WP versions don't all need to polyfill it.
The rest of the patch does not need backporting.

So, basically just this:

<?php
function wp_readonly( $readonly, $current = true, $echo = true ) {
    return readonly( $readonly, $current, $echo );
}

Not sure what the @since tag should read in the docblock though...

Suggestion: backport for WP 5.2 - 5.8.

[prbot]

​jrfnl commented on ​PR #1557:

Closing as merged via 51586

Since PHP 8.1, readonly is a reserved keyword and cannot be used as a function name.

In order to avoid PHP parser errors, the readonly() function was extracted to a separate file and is now only included conditionally on PHP < 8.1 in WP 5.9.

To prevent plugins/themes using this function each having to polyfill it, the new wp_readonly() function is being backported.

Trac ticket: https://core.trac.wordpress.org/ticket/53858

[jrf]

FYI: I've opened ​GitHub PR #1590 with a backport proposal.

[peterwilsoncc]

I have mixed feelings about backporting to 5.8 as this is a PHP 8.1 compatibility fix going in to a version that doesn't support the version of PHP.

I certainly don't want to backport to WP 5.7 or earlier as they're unsupported versions of WordPress. The last time Core backported a fix to unsupported versions was #49956 (at my suggestion) was a much more serious issue in my view.

[jrf]

@peterwilsoncc Thanks for pitching in. I agree that this backport is optional, at the same time, I kind of feel that a site potentially getting a white screen of dead due to a PHP parse error is kind of serious.

Similar to the patch applied in this ticket for WP 5.9, plugins/themes cannot use the keyword readonly as a plain function call anymore when the plugin is used on PHP 8.1 as it would be a parse error.
As most plugins and themes support more than just the latest and greatest of WP, any plugin using this function would need to jump through some hoops to make their code PHP and WP cross-version compatible.

There are various solutions plugins/themes can use if we don't backport:

• Stop using the WP Core function altogether and write their own logic to add the HTML attribute.
• Conditionally, depending on the WP version and PHP version, use call_user_func('readonly', $args...).
• Polyfill the new function when it doesn't exist.

This backport proposal means that plugins/themes can switch to using the new function name outright and that we greatly diminish the risk of burdening end-users with sites going down.

If we don't backport, we really need to make it extremely clear in the dev-note how serious the problem is they will cause if plugins/themes do not adjust their code for this issue.

[desrosj]

Replying to jrf:

This backport proposal means that plugins/themes can switch to using the new function name outright and that we greatly diminish the risk of burdening end-users with sites going down.

I don't know that we can definitively say this. Because plugins and themes often support many versions of WordPress, we would need to backport this pretty far to be able to say this with any confidence.

I think this situation is a parallel to when other new functions are added to Core. Plugin and theme developers need to use function_exists() checks to defensively confirm something exists before attempting to utilize it, or they must raise the minimum required version of WP. This is only different because this change is also related to PHP 8.1.

While it would be nice to allow developers to start updating their code now, I think it's perfectly reasonable to call this out as a "preparing for WP 5.9 & PHP 8.1 support" item.

[desrosj]

Since 5.8.1 RC is in less than 48 hours and there's no consensus on whether backporting should occur here, I'm going to push this to 5.8.2 for now.

[jrf]

Just FYI: I'm okay with not backporting this, as long as we all understand the risk sufficiently.

Unlike other new Core functions, using a function_exists() and calling the old function if it doesn't exist, is not an option in this case, as just calling the old function alone will be a parse error in PHP 8.1.

So without a backport, I'd strongly recommend for the code sample(s) I posted in https://core.trac.wordpress.org/ticket/53858#comment:13 to be used in the dev-note.

[jrf]

FYI: some not all too subtle lobbying from my end, may be yielding some results.

There is a PR open against PHP core now to reduce readonly from "full keyword" to (limited) "contextual keyword", which would hopefully avoid any white screens of death: ​https://github.com/php/php-src/pull/7468

I still think we need to strongly encourage plugins/themes to move to using the wp_readonly() function as having a function name which clashes with a PHP reserved keyword is never a good idea, but the impact on end-users for slow adopters amongst plugins/themes should be a whole lot less if the upstream change gets accepted.

[prbot]

​jrfnl commented on ​PR #1590:

I'm closing this PR as a change has been made (committed) to PHP 8.1, which mitigates the impact of the new readonly keyword, meaning that a backport should not be necessary anymore.

For more information, see: ​https://github.com/php/php-src/pull/7468

[jrf]

As the PR to PHP Core has now been committed to PHP 8.1, which mitigates the breaking impact of the new keyword for the WP usecase, the backport is no longer needed.

The change to PHP Core is expected to be a temporary exception to the keyword rules (probably until PHP 9.0, but the discussion about this has only just started), so the original patch for WP 5.9 is still needed and devs should still migrate to using the new function name.

And even if the exception made in PHP would not be temporary, I still think this change should stay in WP as using a PHP reserved keyword as a function name is not only confusing, but also sets a bad example and precedent.

Having said all that, @desrosj @SergeyBiryukov can this ticket now be closed ? Or should it stay open for the dev-note (which is still needed) ?

[desrosj]

We can close it out. The needs-dev-note keyword will flag flag the ticket and can be changed to has-dev-note after it's published.