Problem/Motivation
Let's fix drupal/coder's sniffs in core.
This issue will cover only Drupal.Classes.FullyQualifiedNamespace
, which demands that we use fully-qualified class names in use
statements, and that fully-qualified class names *only* appear in use
statements.
Proposed resolution
Add drupal/coder and the phpcs package to your Drupal codebase:
# From Drupal root directory
$ composer require drupal/coder
$ ./vendor/bin/phpcs --config-set installed_paths DRUPALPATH/vendor/drupal/coder/coder_sniffer/
Add Drupal.Classes.FullyQualifiedNamespace
to core/phpcs.xml.dist
.
Run phpcbf from the core directory:
$ cd core
$ ../vendor/bin/phpcbf
This will fix some errors. Review what it did and fix problematic changes.
Run phpcs from the core directory in order to verify that no errors remain:
$ ../vendor/bin/phpcs -p
To review this issue
Install coder/phpcs as above.
Apply the patch.
Run phpcs from the core directory to verify that no errors remain.
$ cd core
$ ../vendor/bin/phpcs -p
Read the patch to check for errors.
Remaining tasks
User interface changes
API changes
Data model changes
Comment | File | Size | Author |
---|---|---|---|
#16 | interdiff.txt | 4.2 KB | pfrenssen |
#16 | 2715741-16.patch | 79.81 KB | pfrenssen |
#11 | 2715741_11.patch | 79.14 KB | Mile23 |
#7 | interdiff.txt | 1.23 KB | Mile23 |
#7 | 2715741_7.patch | 78.69 KB | Mile23 |
Comments
Comment #2
Mile23Generated by
phpcfb
and then reviewed to make sureuse
statements are correct and alphabetical.Comment #4
pfrenssenComment #5
Mile23Thanks, but now the issue summary is incorrect.
Same with the other issues you changed, so I can't copy-paste it back to being correct again.
Fixed the error, and one other thing phpcbf added that it shouldn't have.
Comment #7
Mile23Hopefully this gets the last lint error...
Comment #8
Mile23Comment #9
alexpottuse before the namespace.
This use is in a weird place
Too many blank lines
Comment #11
Mile23Thanks for the review.
Pretty sure I got all the failing tests. It's non-trivial to make a list from the CI report but I did some of them manually.
Also addressed #9.
Comment #12
alexpottStill some additional space
Comment #13
Mile23Got it... :-)
The patch applies cleanly to 8.1.x, so I'll run a test for 8.1.x as well.
Comment #14
pfrenssen@Mile23 sorry about the summary change, I updated all summaries to because the instructions were no longer correct. I failed to notice that this one was different from the others.
Comment #15
pfrenssenThis is looking good. I have a few remarks but there are all about alphabetical ordering of the use statements, this is more of an accepted practice than a formal coding standard so it shouldn't hold up this issue.
Patch looks good, still applies, and PHPCS comes back green. RTBC, thanks!
Thanks for fixing the alphabetical ordering, this is one of my pet peeves :)
It would be nice to have this in alphabetical order. This does not a blocker to get this issue accepted though.
Same here, would be nice to be alphabetically ordered.
Same here.
Same here.
Comment #16
pfrenssenI quickly fixed the alphabetical ordering now that I had the patch applied. This satisfies my alphabetical obsession :) Leaving RTBC, it does not affect the validity of the patch.
Comment #17
alexpottCommitted 2d47677 and pushed to 8.1.x and 8.2.x. Thanks!