Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.Since we are now using PHP 7+ we can use the null coalescing operator (??). The related issue had this change, but rather split this off.
+++ b/core/modules/jsonapi/src/ResourceType/ResourceTypeRepository.php
@@ -188,7 +198,7 @@ public function get($entity_type_id, $bundle) {
- return isset($resource_types[$type_name]) ? $resource_types[$type_name] : NULL;
+ return $resource_types[$type_name] ?? NULL;
Technically the `??` construct is equal to the `isset` contruct, just more readable.
The related only consisted of changes in the jsonapi module, since it woukd be so many lines.
How to reproduce
php-cs-fixer mr914
Added a config this time so it will do everything at once.
git clone --branch '9.3.x' https://git.drupalcode.org/project/drupal.git
cd drupal
mkdir --parents tools/php-cs-fixer
composer require --working-dir=tools/php-cs-fixer friendsofphp/php-cs-fixer
Now create the config file at tools/php-cs-fixer/.php-cs-fixer.3222251.dist.php.
<?php
/*
* This document has been generated with
* https://mlocati.github.io/php-cs-fixer-configurator/#version:3.0.0|configurator
* you can change this configuration by importing this file.
*/
$config = new PhpCsFixer\Config();
$finder = PhpCsFixer\Finder::create()
->exclude('vendor')
->notPath('tests/Drupal/Tests/Component/Annotation/Doctrine/Fixtures/Annotation/Template.php')
->notPath('lib/Drupal/Component/Annotation/Doctrine/DocParser.php')
->name('*.php')
->name('*.theme')
->name('*.module')
->name('*.sh')
->name('*.inc')
->in(['core', 'composer']);
return $config
->setRules([
'ternary_to_null_coalescing' => true,
])
->setFinder($finder);
Run the fixer
tools/php-cs-fixer/vendor/bin/php-cs-fixer fix . -vvv --diff --config tools/php-cs-fixer/.php-cs-fixer.3222251.dist.php --path-mode=intersection
git add core
git add composer
There is some manual work to fix some style issues like: ([$var,] and indentation faults).
| Comment | File | Size | Author |
|---|
Issue fork drupal-3222251
Show commands
Start within a Git clone of the project using the version control instructions.
Or, if you do not have SSH keys set up on git.drupalcode.org:
changes
Comments
Comment #2
bbralaComment #3
bbralaThis patch is just an illustration, not ready yet.
Comment #4
bbralaComment #5
bbralaComment #6
bbralaComment #7
bbralaComment #8
bbralaI hate file permissions in git combined with Windows.
Comment #9
longwaveI didn't do a full review but I think these changes should be reverted:
This is a copy of code from the Doctrine project so I think we should leave this alone (there are other coding standards issues in these files).
Same here.
Comment #10
bbralaValid point about the Doctrine project classes.
Also i've just created a branch on Gitlab. The patches weren't playing nice :/ No real review needed yet though, want to wait for the reaction on the related issue. This is a lot of changes ;)
Comment #12
alexpott@bbrala are you scripting this patch? If so can you add instructions to the issue summary? If there's not a script then perhaps we want to try and put one together to make it really simple to re-create the patch.
I think we should schedule this patch for the 9.3.x beta and land it then.
Comment #13
bbralaI'm just using an inspection from PHPStorm which can scan a codebase for specific things and help you refactor those. The commands:
That was my workflow, unfortunately no script :)
The merge request is a clean patch though, but perhaps a trial of PHPStorm is something you could use.
Comment #14
bbralaAlso attached a screenshot of the workflow I used to refactor this.
Comment #15
andypostComment #17
bbralaUnfortunately php-cs-fixed doesn't include .inc and .theme files by default. There are missing there. Added to the script. This should reproduce MR903. Ofcourse if could be done with a custom php-cs-fixer config with adjusted filenames, but that didn't work right away.
The reset for the two files is based on #9.
Comment #18
bbralaThere were 4 files missed by php-cs-fixer that did have possible changes. See commit 0289376d.
Comment #19
guilhermevp CreditAttribution: guilhermevp at CI&T commentedIf there is any way I can help, I'm here!
Comment #20
bbralaComment #21
longwaveThanks for the work on this so far, and for providing the script - this should be helpful if we need to rebuild this MR later on.
I agree with @alexpott in #12 that this change is disruptive, as per the allowed changes policy this consists of "code style updates which are likely to conflict with other patches in the queue" and therefore should wait until the beta window, which unfortunately isn't until November.
Marking this as postponed and flagging the date; ideally we should aim to get this RTBC in early November so it can be committed on or shortly after the 8th.
Comment #22
bbralaNo problem ofcourse. Just update here or send a message on slack around that time and I'll create a new MR. Pretty easy to do.
Comment #23
andypostWould be great to explore phpstan ability to detect such cases
Comment #24
bbralaTechnically it could also live in the codestyle. There seems to be an implementation for PHPCS that has this SlevomatCodingStandard/Sniffs/ControlStructures/RequireNullCoalesceOperatorSniff.php. Perhaps something like that could help since we could connect to the existing codestyle checker and fixer in core.
Not too sure if phpstan should be the place for this. Isn't phpstan mostly about finding bugs early and codestyle about controle flow formatting and such?
Comment #26
bbralaAdded simple way with a config.
Comment #27
bbralaPing :)
What would you need here?
Comment #28
volegerComment #29
voleger#25 is not up to date. The changes needs to be rabased.
Comment #32
bbralaComment #35
bbralaI reran the scripts against 9.4.x, waiting for CI to succeed before sending for review.
There was some manual work to fix some style issues like: (
[$var,]and indentation faults). But that was pretty minimalComment #36
bbralaEverything is green, changes where automated for 95% setting to needs review.
Comment #37
bbralaComment #38
volegerI see the replacement of the list construction only. Is it expected?
Comment #39
bbralaOw ow, did i push the wrong branch, let met check
Comment #40
bbralaYeah, i made an error, i was doing two issues last night for 2 styles. Appearanly i mixed up something. I'll put up a fix soon
Comment #41
bbralaUpdated the PR, fixing the push. This monday i pushed the wrong changes to this issue while working on them both simulataniously.
Comment #42
alexpottWe need to run this on the composer directory as well as core. Plus we need to include run-tests.sh.
Comment #43
bbralaUpdated config
Updated command:
tools/php-cs-fixer/vendor/bin/php-cs-fixer fix . -vvv --diff --config tools/php-cs-fixer/.php-cs-fixer.3222251.dist.php --path-mode=intersectionThis removes the needfor a reset. I'll push the updated code.
Comment #44
bbralaRun tests was already updated in the MR btw :)
Comment #45
longwaveReviewed the full patch. A few nits about stylistic comments (mostly rewrapping lines and useless brackets - feel free to argue back on any of these!) but otherwise this looks good, and makes every single case more readable than before.
Comment #46
bbralaWent through them quickly, and a few need some extra attention and if correct a child issue imo.
Most nits I agree to, so many stray brackets :)
Comment #47
bbralaThat was a bit of spam. There are 3 thread that need your attention @longwave.
I've also made a child issue for the views plugin test.
Comment #48
bbralaComment #49
longwaveThanks - all issues resolved, this is RTBC for me.
Comment #52
larowlanCommitted 330473e and pushed to 9.4.x. Thanks!
Backported to 9.3.x to keep the two branches in alignment and maximise the chance of backports applying cleanly.
Amazing work here.
Full credit to `git diff --color-words` which made reviewing this locally much easier.
Can we get a follow up to investigate a PHPCS rule to prevent any of these creeping back in?
Wrote and published a change record.
Comment #53
quietone CreditAttribution: quietone as a volunteer commentedAs I understand it, In order to do that we first need agreement on that as a coding standard, If that is correct then an issue is needed in the Coding Standards project.
Looking now for an issue.
Yes, there is, it is #3074131: Use null coalescing operator ?? instead of a ternary operator with an isset() condition.
There is no need for a followup.
Comment #54
larowlanAwesome, thanks
Comment #55
larowlanCrediting guilhermevp from the druplicate
Comment #56
xjmWe should explore adding a phpcs rule for this so that we don't have to do cleanups of it every beta.
Comment #57
bbrala@xjm The parent issue refers to a new dependency, in coder. That would also allow this:
SlevomatCodingStandard.ControlStructures.RequireNullCoalesceOperator
(On mobile, links are hard sorry)
Comment #58
bbralaJust to be complete;
As mentioned in #3222769: [November 8, 2021] Replace all list (array destructuring) assignment to the array syntax by @xjm this could be enforced if we adopt the library mentioned in #3010032: Add dependency on slevomat/coding-standard. This includes a rule that could validatie this change.
Comment #59
bbralaAdded related issue in Coding standards project.
Comment #60
quietone CreditAttribution: quietone as a volunteer commentedRemoving tag, there are followups in Coding Standards project and the Coder project.