Make WordPress Core

Opened 16 months ago

Last modified 4 months ago

#58874 accepted enhancement

Code Modernization: Consider using the null coalescing operator.

Reported by: costdev's profile costdev Owned by: costdev's profile costdev
Milestone: Future Release Priority: normal
Severity: normal Version:
Component: General Keywords: has-patch awaiting-php7-make-post-first 2nd-opinion
Focuses: Cc:

Description

Overview

There are currently around 500 instances of isset( $var ) ? $var : 'default' in Core.

In Updating the Coding Standards for modern PHP, the proposal states:

The spaceship <=>, null coalesce ?? and null coalesce equals ??= operators can not be used in WordPress Core until the minimum PHP version has been raised to PHP 7.0 (spaceship and null coalesce) or PHP 7.4 (null coalesce equals).


With the dropping of PHP 5 support in WordPress 6.3, we can now make use of the null coalescing operator.

The null coalescing operator (??) has been added as syntactic sugar for the common case of needing to use a ternary in conjunction with isset(). It returns its first operand if it exists and is not null; otherwise it returns its second operand.
PHP.net


This allows changes from:

<?php

$new_var = isset( $var ) ? $var : 'default';
$new_var = isset( $arr['key']['subkey'][0] ) ? $arr['key']['subkey'][0] : 'default';
$new_var = isset( $obj->prop ) ? $obj->prop : 'default';

to:

<?php

$new_var = $var ?? 'default';
$new_var = $arr['key']['subkey'][0] ?? 'default';
$new_var = $obj->prop ?? 'default';

Proposal

This ticket proposes that we update these as the above instances often result in very long lines or very cumbersome conditions. This follows on from similar changes to use str_starts_with(), str_ends_with() and str_contains(), helps usher in the bump to a PHP 7 minimum for WordPress in a safe way compared to other PHP 7+ features, and promotes increased contribution as prospective contributors see WordPress not just enforcing a minimum PHP version or using new features in new code, but modernising its existing codebase.

As we're very early in 6.4-alpha, making this change now is as "perfect" as we could hope to be considering this will invalidate some patches. However, given the verbosity of isset() ternaries, these usually occur on their own line with very little extra code, so the number of invalidated patches should be relatively low.

Considerations

  1. Backporting: This may add extra work if backports involve changing lines containing the affected isset() ternaries. This applies to security backports as well as WordPress 6.3 minor releases. However, our earlier changes to str_starts_with(), str_ends_with() and str_contains() had a greater risk of creating extra work.
  2. Invalidated patches: Patches that change lines containing the affected isset() ternaries will need a refresh. This is a negative, but it's also likely to be relatively straightforward to resolve for each patch. Our earlier changes to str_starts_with(), str_ends_with() and str_contains() risked invalidating many more patches compared to this proposal.
  3. Readability: While objectively this is a benefit for brevity, readability is subjective.
  4. Code refactoring should not be done just because we can: This page details several things needed for proposals such as this:
  • Unit tests, even if the code was not previously covered. We can’t afford regressions, and this will improve our test coverage.
    • The behaviour of the null coalescing operator is the same as isset( $var ) ? $var : 'default'. This proposal does not suggest changing any other instances at this time.
  • Performance benchmarks, before and after. We can’t afford regressions.
    • The behaviour and performance of the null coalescing operator is the same as isset( $var ) ? $var : 'default'. This proposal does not suggest changing any other instances at this time.
  • Proper justification and clear rationale of changes are both necessary. Too often it is impossible to determine the purpose, objective, or focus of these patches. Code should not be rewritten under the cloaks of readability, narrow personal opinion, or general subjectiveness.
    • Much like the changes to str_starts_with(), str_ends_with() and str_contains(), this provides brevity in the codebase, and per-instance has a much greater reduction in code.
    • It reduces our time reading and writing code - I appreciate there will be an adjustment period for some.
    • It provides a clear signal to prospective contributors and to extenders that WordPress is moving forward, encouraging participation and observation for future changes.

Change History (23)

This ticket was mentioned in PR #4886 on WordPress/wordpress-develop by @costdev.


16 months ago
#1

This PR contains commits separated by component for easier reviewing/possible commit.

Changes can be reviewed altogether or per PR commit.
If moved forward for merging to WordPress Core, this PR's commits can be merged individually, grouped into smaller chunks, or squashed into a single commit.

#2 @costdev
16 months ago

  • Milestone changed from Awaiting Review to 6.4

Pinging some folks who also worked on/discussed the str_starts_with(), str_ends_with() and str_contains() changes.

@azaozz @SergeyBiryukov @peterwilsoncc @audrasjb @spacedmonkey @mukesh27

Props to @afragen for reviewing the ticket summary after he'd just arrived to work.

Moving this to the 6.4 milestone to get some early attention on this.

#3 @swissspidy
16 months ago

I think this should be a blog post on make/core first.
Introducing new syntax is different from using new functions. Also means updates to WPCS are required. Hence I would post this oroposal on make/core first.

#4 follow-up: @jrf
16 months ago

I agree with @swissspidy that this should be discussed on Make first, probably as part of a larger Make post about the PHP 7.0 syntaxes now available.

I also wonder why this should be changed for existing code (as it makes backports harder). What about just allowing it for new code and for code which is being patched anyway ?
That way, the impact on backports will be much more limited, while we can still use the new shiny in code actually being worked on. 😁

Other considerations for this ticket:

  • Should this also be changed/applied when the original ternary is used as a parameter for a function call ?
  • Should this also be changed/applied when the original ternary is used as part of a condition in a control structure ?

For the first I imagine it may impact readability more in a negative way, for the second I see a risk of issues with operator precedence.

And while we are discussing this, WP, more often than not, uses a slightly different pattern for the same:

<?php
$var = 'default';
if ( isset( $bar ) ) {
    $var = $bar;
}

...which could be re-written to:

<?php
$var = $bar ?? 'default';

I don't see any mention of that pattern in this ticket, while I expect that to be far more common.

Changing the code "naturally" (as in update code which is being worked on when it is being worked on) would be more likely to also catch that pattern and update it.

Note: I'm not against this change or against using the null coalesce operator, I just think we need to think this through a bit more carefully.

Also means updates to WPCS are required

@swissspidy Good point. As far as I can see, WPCS (3.0.0) should be fine as the null coalesce operator is already taken into account in various places and where it wasn't, we've made some updates already for this to be included in WPCS 3.0.0.
We may, of course, have overlooked some situations in which it should be taken into account, in which case, please submit a bug report.

Last edited 14 months ago by jrf (previous) (diff)

#5 @jrf
16 months ago

Oh and performance-wise I expect no significant impact of changing the patterns as IIRC PHP desugars the syntax anyway (i.e. at compile time brings the code back to the old pattern). (could do with verification)

Last edited 16 months ago by jrf (previous) (diff)

#6 @costdev
16 months ago

@swissspidy @jrf I agree this should be discussed in a Make post.

I also wonder why this should be changed for existing code (as it make backports harder). What about just allowing it for new code and for code which is being patched anyway ?
That way, the impact on backports will be much more limited, while we can still use the new shiny in code actually being worked on. 😁

I compared this to the str_starts_with() etc. changes a bit too much, but I recognise that a syntax change differs from using new functions/polyfills. I thought that updating the existing codebase would help get contributors more used to both seeing and using the operator for the use cases mentioned in the ticket summary. As Committers would feel an impact that I wouldn't, I was very much looking for the exact kind of feedback that's come so far 🙂

Other considerations for this ticket:

  • Should this also be changed/applied when the original ternary is used as a parameter for a function call ?
  • Should this also be changed/applied when the original ternary is used as part of a condition in a control structure ?

For the first I imagine it may impact readability more in a negative way, for the second I see a risk of issues with operator precedence.

I agree that both of these may have a more negative impact that a positive one.

And while we are discussing this, WP, more often than not, uses a slightly different pattern for the same
I don't see any mention of that pattern in this ticket, while I expect that to be far more common.

True, that case is very likely to be more common, although harder to detect and may require closer inspection to determine whether to make the change. I figured that for an initial change, the more straightforward use case mentioned in the summary with ~450 instances in Core was a common enough case that was particularly verbose.

I'm happy to move the discussion to a Make post about the PHP 7 syntaxes when it's ready and keep this ticket open if there is anything actionable as a result of the proposal. 🙂

Last edited 16 months ago by costdev (previous) (diff)

#7 in reply to: ↑ 4 @SergeyBiryukov
16 months ago

Replying to jrf:

I also wonder why this should be changed for existing code (as it make backports harder). What about just allowing it for new code and for code which is being patched anyway ?
That way, the impact on backports will be much more limited, while we can still use the new shiny in code actually being worked on. 😁

I would very much prefer consistency in the code, i.e. using the same pattern for the same thing across all codebase, to avoid confusion, or adding a comment if that's not feasible in a particular place for some reason.

In this case, using the null coalescing operator (??) anywhere in the file appears to cause a parse error on PHP 5.6, so it would be trivial to catch if it's accidentally backported to an older branch.

I think only allowing it for new code does not automatically make backports easier, as some parts of new code can occasionally be backported too as part of bug fixes or security fixes. So I believe the required PHP version bump in 6.3 is something we'll have to keep in mind anyway when doing backports, pretty much like with the previous version bump to PHP 5.6.20 in WP 5.2.

#8 follow-up: @hellofromTonya
16 months ago

Adopting this syntax change will also impact code being merged from Gutenberg. How? The Gutenberg plugin supports the last 2 WP releases, which will be 6.3 (PHP 7.0 minimum) and 6.2 (PHP 5.6 minimum).

There's a discussion in Gutenberg for when it can raise its minimum supported PHP version https://github.com/WordPress/gutenberg/issues/52344.

#9 in reply to: ↑ 8 @hellofromTonya
16 months ago

Replying to hellofromTonya:

Adopting this syntax change will also impact code being merged from Gutenberg. How? The Gutenberg plugin supports the last 2 WP releases, which will be 6.3 (PHP 7.0 minimum) and 6.2 (PHP 5.6 minimum).

There's a discussion in Gutenberg for when it can raise its minimum supported PHP version https://github.com/WordPress/gutenberg/issues/52344.

This concern is now resolved. Gutenberg's minimum supported PHP version is now 7.0.

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


14 months ago

#11 @marybaum
14 months ago

I am a huge fan of new syntax in PHP and JS. And, I'm not interested in signing anyone up for endless backports, per Sergey. So greenlighting the operators in new code and patches seems a good compromise.

But our process has been to run it by the community with a post on the p2. Thoughts on that?

#12 @oglekler
14 months ago

Can we proceed with this? If so, the patch needs to be rebased.

@costdev commented on PR #4886:


14 months ago
#13

@mukeshpanchal27 This PR is on hold until a Make post has been made to establish what PHP 7 syntaxes will be supported/allowed in Core.

#14 @costdev
14 months ago

  • Keywords awaiting-php7-make-post-first added; 2nd-opinion removed
  • Milestone changed from 6.4 to 6.5
  • Owner set to costdev
  • Status changed from new to accepted

@marybaum A Make post regarding PHP 7 is being prepared which will include issues such as the one raised in this ticket. See comment:4.

@oglekler As the Make post hasn't been posted yet, and now that there is a 6.5 milestone, I'll move this to 6.5. It can be pulled back into the 6.4 milestone if the Make post and feedback arrive in time, and if this is considered a task rather than an enhancement, but we'll cross that bridge if we come to it. 🙂

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


14 months ago

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


14 months ago

#17 @dmsnell
14 months ago

Sharing my vote for using the ?? operator because it acts like isset() as a language opcode instead of a function call. If the variable it's checking doesn't exist, or if a nested key isn't around, it defaults to the right side of the ?? instead of throwing an exception. That is, it's characteristically different than ?: because its inherently safe where ?: is unsafe, as are if statements and the like.

More interested in new code than updating old code, as we already have a high pace of refactoring old code that wasn't broken, and I recently caught a regression introduced during such a change. But still, for new code, it's a helpful construct.

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


10 months ago

#19 @chaion07
10 months ago

  • Keywords 2nd-opinion added
  • Milestone changed from 6.5 to Future Release

Thanks @costdev for reporting this. We reviewed this ticket during a recent bug-scrub session. Based on the feedback we are making the following changes:

  1. Updated milestone to Future Release
  2. Added 2nd-opinion keyword
  3. Waiting for the detailed PHP 7 Make post
  4. Also we have to decide about potentially updating existing code to use ??. For example, there are differing opinions on the ticket.

Cheers!

Props to @costdev & @mukesh27

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


9 months ago

#21 @noisysocks
5 months ago

@marybaum A Make post regarding PHP 7 is being prepared which will include issues such as the one raised in this ticket. See comment:4.

Has this happened?

I don't see a downside in allowing ?? in new code right now. It's frustrating rewriting code that uses ?? to use ?: when syncing changes from Gutenberg. It's also potentially error prone since we're rewriting perfectly good code that has received lots of testing into something that has slightly different semantics as @dmsnell notes above.

Waiting for existing code to be updated and/or an official Make/Core post on the subject just sounds like we're letting process and bureaucracy get in the way of good outcomes for WordPress and its users.

#22 @swissspidy
5 months ago

FWIW, by now some new code has already been introduced into core (and shipped) that uses the null coalescing operator.

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


4 months ago

Note: See TracTickets for help on using tickets.