Switch long-form single-line assign and null coalesce (= ??) to null coalesce assign operator (??=)

Thanks for the suggestion. I assume there are many similar changes we could make across all of core, and ideally a coding standards sniff we could enable to prevent this code from creeping back in.

Ideally we would change all instances of this across core in one go, please read the issue scoping guidelines for more info: https://www.drupal.org/docs/develop/issues/issue-procedures-and-etiquett... →

If you can help expand the scope of this issue, please do!

I have uploaded the patch.
Please review

Indeed, that is clear. Here the idea was to suggest the principle with a concrete case. The general purpose is necessary but more difficult.

As first sniffer,
grep -Prn '^\s*([^\s]+)\s=\s\1\s\?\?\s.+$'

I think this issue is the initiative.

Then again, maybe not. This issue may be about using one-line assignment statements instead of multi-line conditionals. Is it?

Yes, reducing the number of instruction lines. When it is possible.

Two files corresponding to patches giving an idea of what kind of code transformations the rule gives. One is small, A, grouping uniligne code, the other is bigger, B, grouping multiligne code. All the code changes here are produced automatically via a sed command (one command, one file). It is the moment for comments. Is it right? Is it complete? Is reasonable?

Title and issue summary mention just layout_discovery.module but the patches appear to be entire drupal so tagging for those to be updated.

Also recommended to use MRs

Also recommended to use MRs

I thought about it, but the work needs a first general code review, and a patch file seems for me a simple means to do it. If it will be, look right try a MR now, or some things likes, to big must be cut, or, some rules look wrong.

The Needs Review Queue Bot → tested this issue.

While you are making the above changes, we recommend that you convert this patch to a merge request → . Merge requests are preferred over patches. Be sure to hide the old patch files as well. (Converting an issue to a merge request without other contributions to the issue will not receive credit.)

Created an MR against 11.x branch. Please review.

@sakthi_dev your MR seems correspond with the patch 9A. Agree with this choice. Thank you for your MR.

May be this issue would be more easy to manage with a parent issue with children corresponding to the different cases. Actually two.

Issue changed from general to the particular simpler case A corresponding to the MR. See the parent issue 📌 Use null coalescing assignment operator Active for the general purpose.

Can we simply resolve to run rector with the result of the relevant rector rule: https://github.com/rectorphp/rector/blob/main/docs/rector_rules_overview... : Rector\Php74\Rector\Assign\NullCoalescingOperatorRector, instead of sed per IS.

That way we have an easily testable/reproducable set of changes.

I'm seeing at least twice as many changes as the MR, compared to a local rector run.

You are right @dpi! Rector is much better. Indeed, some cases was missed...

13 missing files found, in core/lib. Got with rector.

Added 📌 Require short ternary (Elvis operator) syntax Postponed as related.

Should we work on some checker to catch these before merging so they don't resurface?

@smustgrave, this particular case is efficiently managed by rector. As indicated by @dpi in #23 📌 Switch long-form single-line assign and null coalesce (= ??) to null coalesce assign operator (??=) Needs work . The general purpose is some thing else. I am in favour of dividing the general problem into different particular cases.

@smustgrave, for phpcs, a rule exists about this case,
slevomat/coding-standard/SlevomatCodingStandard/Sniffs/ControlStructures/RequireNullCoalesceEqualOperatorSniff
but is missing in drupal/coder/coder_sniffer/Drupal/ruleset.xml,
<rule ref="SlevomatCodingStandard.ControlStructures.RequireNullCoalesceEqualOperator" />
I think this is this case since the rule message is,
Use "??=" operator instead of "=" and "??

The Needs Review Queue Bot → tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".

This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide → to find step-by-step guides for working with issues.

Use phpcs with rule RequireNullCoalesceEqualOperator from SlevomatCodingStandard:
phpcs --standard=SlevomatCodingStandard --sniffs=SlevomatCodingStandard.ControlStructures.RequireNullCoalesceEqualOperator core

1 test error occurs. Some thing wrong happen with,
Drupal\Tests\Composer\Plugin\Scaffold\Functional\ManageGitIgnoreTest::testUnmanagedGitIgnoreWhenGitNotAvailable
in core/tests/Drupal/Tests/Composer/Plugin/Scaffold/Functional/ManageGitIgnoreTest.php line 253,
$this->assertEquals($expected, $output);
The $expected is not equal to the $output. In this latter an error message exists,
Composer could not detect the root package (fixtures/drupal-composer-drupal-project) version, defaulting to '1.0.0'. See https://getcomposer.org/root-version

The test error message,

Drupal.Tests.Composer.Plugin.Scaffold.Functional.ManageGitIgnoreTest
Name testUnmanagedGitIgnoreWhenGitNotAvailable
File core/tests/Drupal/Tests/Composer/Plugin/Scaffold/Functional/ManageGitIgnoreTest.php
Execution time 448.86ms
System output

Drupal\Tests\Composer\Plugin\Scaffold\Functional\ManageGitIgnoreTest::testUnmanagedGitIgnoreWhenGitNotAvailable
Failed asserting that two strings are equal.
--- Expected
+++ Actual
@@ @@
-'Scaffolding files for fixtures/drupal-assets-fixture:\n
+'Composer could not detect the root package (fixtures/drupal-composer-drupal-project) version, defaulting to '1.0.0'. See https://getcomposer.org/root-version\n
+Scaffolding files for fixtures/drupal-assets-fixture:\n
   - Copy [web-root]/.csslintrc from assets/.csslintrc\n
   - Copy [web-root]/.editorconfig from assets/.editorconfig\n
   - Copy [web-root]/.eslintignore from assets/.eslintignore\n

vendor/phpunit/phpunit/src/Framework/Constraint/Equality/IsEqual.php:94
core/tests/Drupal/Tests/Composer/Plugin/Scaffold/Functional/ManageGitIgnoreTest.php:253
vendor/phpunit/phpunit/src/Framework/TestResult.php:728
vendor/phpunit/phpunit/src/Framework/TestSuite.php:684
vendor/phpunit/phpunit/src/TextUI/TestRunner.php:651
vendor/phpunit/phpunit/src/TextUI/Command.php:144
vendor/phpunit/phpunit/src/TextUI/Command.php:97

I don't see the relation with ??=.

I don't see the relation with ??=.

There is indeed no relation, HEAD was broken for a while 🐛 ManageGitIgnoreTest failing in HEAD RTBC .
Is fixed now, a rebase should make the error go away.

The $output comes from,

    // Confirm that 'git' is no longer available.
    $output = [];
    exec('git --help', $output, $status);
    $this->assertEquals(127, $status);
    // Run the scaffold command.
    $output = $this->mustExec('composer drupal:scaffold 2>&1', NULL);

And the composer error comes from the latter line.

From https://getcomposer.org/root-version the error message correspond to the case 4. The information can not come from case 3, since git is not available here. Therefore, to provide the information remain either case 1, through the version key in composer.json, or case 2, from the COMPOSER_ROOT_VERSION environment variable. Else the message is right, and the the $expected is erroneous with this message missing.

Thank you @Spokje for the information. You mean #30 📌 Switch long-form single-line assign and null coalesce (= ??) to null coalesce assign operator (??=) Needs work ? Normally it is a new rebased version.

Okay @Spokje, that is clear.

MR Mergeable. Need review. Not a straight line, but seems okay now. Rebased on #3428032 to pass the test.

The Needs Review Queue Bot → tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".

This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide → to find step-by-step guides for working with issues.

On the coding standard decision #3436307: Change !isset to the null coalescing assignment operator ??= →