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

Allow using readonly as function name #7468

Closed
wants to merge 1 commit into from

Conversation

nikic
Copy link
Member

@nikic nikic commented Sep 6, 2021

Don't treat "readonly" as a keyword if followed by "(". This
allows using it as a global function name. In particular, this
function has been used by WordPress.

This does not allow other uses of "readonly", in particular it
cannot be used as a class name, unlike "enum". The reason is that
we'd still have to recognize it as a keyword when using in a type
position:

class Test {
    public ReadOnly $foo;
}

This should now be interpreted as a readonly property, not as a
read-write property with type ReadOnly. As such, a class with
name ReadOnly, while unambiguous in most other circumstances,
would not be usable as property or parameter type. For that
reason, we do not support it at all.

Based on the recent Twitter discussion, cc @jrfnl @ramsey @derickr. Unfortunately this is not such a clean case as enum, because we can only fully support the function case.

Don't treat "readonly" as a keyword if followed by "(". This
allows using it as a global function name. In particular, this
function has been used by WordPress.

This does not allow other uses of "readonly", in particular it
cannot be used as a class name, unlike "enum". The reason is that
we'd still have to recognize it as a keyword when using in a type
position:

    class Test {
        public ReadOnly $foo;
    }

This should now be interpreted as a readonly property, not as a
read-write property with type `ReadOnly`. As such, a class with
name `ReadOnly`, while unambiguous in most other circumstances,
would not be usable as property or parameter type. For that
reason, we do not support it at all.
@jrfnl
Copy link
Contributor

jrfnl commented Sep 6, 2021

@nikic I, for one, very much appreciate this PR. This will significantly lessen the risk of "white screens of death" for end-users of WordPress, so would be great if this PR gets accepted.

@mvorisek
Copy link
Contributor

mvorisek commented Sep 6, 2021

This should target PHP-8.1 otherwise it is quite useless.

@ramsey
Copy link
Member

ramsey commented Sep 6, 2021

This should target PHP-8.1 otherwise it is quite useless.

I'll discuss with the other 8.1 RMs.

@patrickallaert
Copy link
Contributor

We can't declare global functions:

public function try() {}
public function throw() {}
public function goto() {}
public function public() {}
public function protected() {}
public function private() {}
public function goto() {}
public function global() {}
public function var() {}
...

While we actually could as none of those keywords are used with a following (.

This would make the keyword readonly somewhat apart, also as being the only one allowed for a function in the global namespace but disallowed as a class in the same global namespace‽

This and being past RC1 doesn't make me super enthusiast about it.

  1. Should it be allowed for now and in the future (PHP 8.2+)?
  2. If allowed, shouldn't this exception generate a deprecation notice so that we can have something more consistent across keywords as of PHP 8.2 (meaning: disallowing it)?
  3. Doesn't it make it more complex for IDE, static analyzers, parsers,... to have multiple notions of being a reserved keyword?

@Girgias
Copy link
Member

Girgias commented Sep 9, 2021

We can't declare global functions:

public function try() {}
public function throw() {}
public function goto() {}
public function public() {}
public function protected() {}
public function private() {}
public function goto() {}
public function global() {}
public function var() {}
...

While we actually could as none of those keywords are used with a following (.

This would make the keyword readonly somewhat apart, also as being the only one allowed for a function in the global namespace but disallowed as a class in the same global namespace‽

This and being past RC1 doesn't make me super enthusiast about it.

1. Should it be allowed for now **and** in the future (PHP 8.2+)?

2. If allowed, shouldn't this exception generate a deprecation notice so that we can have something more consistent across keywords as of PHP 8.2 (meaning: disallowing it)?

3. Doesn't it make it more complex for IDE, static analyzers, parsers,... to have multiple notions of being a `reserved keyword`?

Counter-point the enum keyword is already apart and a function can be called enum in PHP 8.1.

I think allowing readonly is pretty much mandatory except if we want to leave the whole of WP behind, which ain't a good idea. Now if we should emit a deprecation notice in the future for these cases, this is up to debate and should follow the standard RFC process.

@patrickallaert
Copy link
Contributor

Counter-point the enum keyword is already apart and a function can be called enum in PHP 8.1.

I think allowing readonly is pretty much mandatory except if we want to leave the whole of WP behind, which ain't a good idea. Now if we should emit a deprecation notice in the future for these cases, this is up to debate and should follow the standard RFC process.

Ok, I regret the WP community haven't raised concerns while in alpha/beta, but I feel we haven't much choice and this is not the most critical thing in terms of decision.

Unless I misunderstood something, we (RMs) are OK with this for PHP 8.1. (/cc @krakjoe / @ramsey)

@kamil-tekiela
Copy link
Member

Could we allow readonly only temporarily and with a deprecation message? I don't feel like this is the right solution in the long term.

@ramsey
Copy link
Member

ramsey commented Sep 9, 2021

As @patrickallaert said, this is approved to target PHP-8.1.

@jrfnl
Copy link
Contributor

jrfnl commented Sep 9, 2021

I can't speak for WordPress, only for myself, however:

  1. Should it be allowed for now and in the future (PHP 8.2+)?
  2. If allowed, shouldn't this exception generate a deprecation notice so that we can have something more consistent across keywords as of PHP 8.2 (meaning: disallowing it)?

I fully support this being a temporary exception and for a deprecation warning to be added together with this change.

Removing the exception in PHP 8.2 may be a bit soon. Would it be acceptable to remove it in PHP 9.0, similar to other deprecated functionality from the 8.x series?

For context:
I know WP has created an alternative function name (in the global namespace, but prefixed with wp_) and will actively encourage plugin/theme authors to update their code.

Having said that, not all plugins/themes are still actively maintained, so having some more time will allow a) enough actively maintained plugins/themes to be fixed up and b) enough end-users to find alternative plugins/themes if the plugin/themes is no longer maintained.

  1. Doesn't it make it more complex for IDE, static analyzers, parsers,... to have multiple notions of being a reserved keyword?

As an active maintainer of various PHPCS based coding standards as well as contributor to PHPCS itself, I agree, yes, this will make it more difficult.
Then again, there are already multiple different sets of rules in place for different types of keywords, so this is just one more to add to that list.

Ok, I regret the WP community haven't raised concerns while in alpha/beta, but I feel we haven't much choice and this is not the most critical thing in terms of decision.

Actually I did raise this same concern before: #7089 (comment)

@krakjoe
Copy link
Member

krakjoe commented Sep 13, 2021

Merged as 76348f3

Thanks.

@krakjoe krakjoe closed this Sep 13, 2021
@jrfnl
Copy link
Contributor

jrfnl commented Sep 13, 2021

Thank you all for still making this change this late in the cycle 🙏🏻 💞

jrfnl added a commit to PHPCompatibility/PHPCompatibility that referenced this pull request Dec 4, 2022
… names

PHP 8.0 introduced `match` as a new reserved keyword and `mixed` as a new "other" reserved keyword.
PHP 8.1 introduced `readonly` as a new reserved keyword, `never` as an "other" reserved keyword and `enum` as a soft reserved keyword.

Note: `readonly` has an exception for when it is used as a function declaration name.

Includes regenerated test case files.

Refs:
* https://wiki.php.net/rfc/match_expression_v2
* https://wiki.php.net/rfc/mixed_type_v2#backward_incompatible_changes
* https://wiki.php.net/rfc/readonly_properties_v2
* https://wiki.php.net/rfc/enumerations
* https://wiki.php.net/rfc/noreturn_type#backwards_incompatible_changes
* php/php-src#7468 (readonly exception in PHP 8.1)
* php/php-src#9512 (readonly exception PHP 8.2 follow-up)
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.

8 participants