Use null coalescing operator ?? instead of a ternary operator with an isset() condition

+1

Because this style will be unenforceable without a coding standard for it, I am moving this issue to the coding standards queue. slevomat/coding-standard can check for this. It could be added as a dependency to coder.

For PHP 7 code, I much prefer this style, too. I’m not quite sure whether we really need a coding standard for it, though. I think most developers already prefer this, so if you are aware that it can be used, they will.
And whether going through all of Core replacing this makes sense is also debatable.

I disagree about the need to update core to use ?? rather than ternaries. Developers look to core to show the "right" way things should be done. If the expectation is that they are using null coalescing but core still uses ternaries, that will create confusion.

Core has been updated to use null coalescing operator so this issue is quite relevant again :)

#3222251: [November 8, 2021] Replace all isset constructs with the null coalescing operator

This would be technically possible to enforce if #3222769: [November 8, 2021] Replace all list (array destructuring) assignment to the array syntax is accepted.

Now that Core follows it completely, I’m also in favor of adding a coding standard for it.

Related issue is merged, the dev version of coder now supports a new set of sniffs that supports ?? also.

As it stands currently, the Drupal coding standards do not appear to have officially adopted the standard that the null-coalescing operator should be used. However, in contrib land, where the default is to use all sniffs defined and provided by Coder (unlike core, which only uses the specific named sniffs in phpcs.xml.dist) we are seeing coding standards messages for this. If the standard has not been discussed, agreed and adopted yet, then it seems wrong to show the messages by default.

The two slevomat sniffs were added in https://git.drupalcode.org/project/coder/-/commit/929d7d4 - 23 Jan 2022, and first appear in Coder 8.3.14

I notice that there has been no test coverage added to Coder for the null-coalescing operator (is that because the actual sniff code is provided by a third-party?) Anyway, that's another issue, the point here is that the two new sniffs are active and causing coding standards faults when the standard has not been officially agreed and documented on https://www.drupal.org/docs/develop/standards/coding-standards

What's the best way forward to resolve this?

@jonathan1055 do you mean that this standard has not been adopted? I've never been involved w/ adopting a drupal coding standard but it looks like the process is for two people to "sponsor" the new standard - if that is still needed I'm happy to do so.

Hi mfb, yes I noticed I'd made a rather critical typo in missing out 'not' in my first line above [I have now edited to fix that].

Apologies if you know this already, but I am sure there is a proper process somewhere to discuss and adopt new standards. After Coder implements the sniff, then contrib developers can use and test it (by using the dev version of Coder). Then when a new Coder releases a new version, it can be used by all in contrib, but Core testing is locked to specific Coder versions, which only get updated at times when it is agreed. However, Core 9.4 and 9.5 both user Coder 8.3.15 so this sniff is available for use, and is added to Coder's Drupal standard

<rule ref="SlevomatCodingStandard.ControlStructures.RequireNullCoalesceOperator" 

When Core adheres to the standard the sniff is added to core phpcs.xml.dist so that it remains clean. This sniff has not yet been added to core phpcs.xml.dist so it is not being checked in core tests, but will be checked in contrib tests if they use the full 'Drupal' standard as defined by Coder.

There is a coding standards channel on slack https://app.slack.com/client/T06GX3JTS/C02LJCF78E8 and I am sure there are places on drupal.org where addoption of new standards is discussed.

@jonathan1055 ok, I've not been on drupal slack lately. I was looking at https://www.drupal.org/project/coding_standards which says "At least two additional active community members (contrib module maintainers, active patch contributors, etc) need to agree to sponsor the change in the form of a comment on the issue."

OK, so I think this is the issue where that process happens :-)
Steps 1 and 2 are done (I second your offer to sponsor), so now tagging "needs announcement for final discussion" as per step 3

What exactly is the actionable proposed resolution here? is it to make an issue in core to enable <rule ref="SlevomatCodingStandard.ControlStructures.RequireNullCoalesceOperator" ?

Has anyone done a documentation search to see if anything needs to change?

What exactly is the actionable proposed resolution here? is it to make an issue in core to enable SlevomatCodingStandard.ControlStructures.RequireNullCoalesceOperator?

Yes that is it. Reading comments #5-9 it suggests that two years ago Core was fixed to use the ?? operator. But we need a core issue to add the sniff. That issue will also be used to re-check if anything has regressed.

docs/develop/standards/php/php-coding-standards#operators is probably the place where this new standard should be stated.

Updated the IS for an announcement.

Fixed IS XML visibilty.

This also seems fine for announcement like this.

I agree, adding a cs rule for this is a good idea and the rules already exist as well. +1

@borisson_ could you update the issue summary to follow the new Coding Standard template? To get the new template, probably the easiest way is to create a new issue, copy/paste the template then discard that issue. Put yourself as a supporter and then fill in as much as you can of the rest of it. Setting to 'needs work' for this.

The Coding Standards Committee has developed a custom issue template to assist in managing issues efficiently. We have already found that it's use is helping. For issues created before the template was in use I am adding the new template to the Issue Summary and asking everyone here to help convert to it..

Thank you for your help!

Made some updates to fill in the new template. Other supporters are required, please add your own (I don't think it is right for anyone to add someone else's name)

The proposed text is currently very short. Do we need an example?

Does the standard / sniff cover other null-coalescing operators, such as assignment using ??=

Added my name to support. Thanks for updating the issue summary @jonathan1055 💙

I have created a change record but I think it needs more detail. Do we just state the new coding standard text, as shown in the issue summary here?

The CR looks good to me, I think this is now back for another consideration before an announcement

@larowlan I think you must have submitted with stale form data or something, because your update in #28 has reverted all the changes we made since #23 to bring in the new template and proposed text.

Hmm, I thought all I did was mark 'Add a change record' as done.

I reverted to the previous version and tried again

Apologies

Thanks for reverting, and getting us back on track.

I have just two questions (from #24) to resolve before this goes to the Coding Standards meeting for review

1. Do we also want to include the null coalese equal operator ??= in this standard? There is a separate phpcodesniffer rule for it, so we can include both in the new standard, and the implementation of the sniffs to enforce can be independent.
2. Does the proposed text need an example?

Do we also want to include the null coalese equal operator ??= in this standard? There is a separate phpcodesniffer rule for it, so we can include both in the new standard, and the implementation of the sniffs to enforce can be independent.

I think we should do that in a separate issue to keep the scope tighter and increase the chance of this progressing.

Does the proposed text need an example?

Added one

Agreed, ??= can be a separate issue.

Thanks for adding the example. I have updated the change request too.

I think this is RTBC for the next Standards Meeting. But if anyone sees a problem, feel free to revert to 'needs work' and explain what's missing.

The announcement has been published
coding-standards-proposals-for-final-discussion-on-2-january-2024

Removing the "Needs announcement for final discussion" tag. Now at step 5.

I am late to reviewing this. The proposed text states "Null-coalescing operators such as ??" but "??" is the null coalescing operator. So, it should be

The null coalescing operator ?? should be used to make code more readable. For example use $value = $values['entry'] ?? ''; instead of $value = isset($values['entry']) ? $values['entry'] : '';

and I am thinking further to

The null coalescing operator ?? should be used instead of the ternary operator. For example use $value = $values['entry'] ?? ';' instead of $value = isset($values['entry']) ? $values['entry'] : '';

But the second idea may be a step to far at this stage.

Agree with #35, the current description might indicate that the null coalescing assignment operator is included in this coding standard. Scoping this in the description to ?? would clarify this.

I would also clarify the title a bit, the current title has some ambiguity in regards to what we 're replacing.

Use Null-Coalescing Operator ?? to replace isset with ternary operator

with something like

Use Null-Coalescing Operator ?? instead of a ternary operator with an isset statement

#35 and #36 are both excellent improvements. +1 for updating the title and IS with that.

Also the example, it would be easier to read if we changed the name of $value, maybe to $result or anything other than $value. Then we don't have the extra cognative overload of dealing with $value and $values. The examples must be really clear and simple.

I agree with #35 and #37. (#36 seems like it has been addressed already?) So maybe something like this:

The null coalescing operator ?? should be used instead of a ternary operator with an isset() condition. For example, use $result = $values['entry'] ?? ';' instead of $result = isset($values['entry']) ? $values['entry'] : '';.

Updated IS with the agreed changes in #35-38

Formatting of the example

This is now formally pending review by core committers. Tagging as such.

As per meeting 26-3-2024 by larowlan, this has been approved.

The was brought up as the core committer meeting on 5 Mar. There was only positive feedback.

I don't have permissions right now to do the doc updates. We are working on that.

Documentation is updated. Think this is all done :)

I'm not completely sure on the credit rules for this project. So keeping rtbc unril credits are fixed.

I reviewed the documentation updates and they agree with the Issue Summary.
The core issue exists, #3418494: Switch long-form single-line assign and null coalesce (= ??) to null coalesce assign operator (??=)

So, it is just the publishing of the change record that needs to be done.

Fixed credits

Think you misreferenced @quietone. I didnt find one specifically for this change. So made #3446107: Enforce null coalescing operator ?? instead of a ternary operator through phpcs.

Published the CR, since all is done i'm going ahead and set this to fixed :) Thanks everyone.