Warning
All patches are generated automatically, if the tests fail, do not attempt to fix it by hand, but create an issue against
the coder module cross linking the issues. The more we can fix in the coder
module, the better.
Part of #2571965: [meta] Fix PHP coding standards in core, stage 1.
Approach
We are testing coding standards with PHP CodeSniffer, using the Drupal coding standards from the Coder module. Both of these packages are not installed in Drupal core. We need to do a couple of steps in order to download and configure them so we can run a coding standards check.
Step 1: Remove the coding standard from the blacklist
Every coding standard is identified by a "sniff". For example, an imaginary coding standard that would require all llamas to be placed inside a square bracket fence would be called the "Drupal.AnimalControlStructure.BracketedFence sniff". There are dozens of such coding standards, and to make the work easier we have started by blacklisting all the sniffs. For the moment all coding standards that are not yet fixed are simply skipped during the test.
Open the file core/phpcs.xml.dist and remove the line that matches the sniff of this ticket (it's in the issue title). Make sure your patch will include the removal of this line.
Step 2: Install PHP CodeSniffer and the ruleset from the Coder module
Both of these packages are not installed by default in Drupal core, so we need to download them. This can be done with Composer, from the root folder of your Drupal installation:
$ composer require drupal/coder squizlabs/php_codesniffer
Step 3: Prepare the phpcs.xml file
Next we need to make a copy of the file phpcs.xml.dist (in the core/ folder) and save it as phpcs.xml. This is the configuration file for PHP CodeSniffer.
We have to configure the location of the Drupal coding standard ruleset from the Coder module that we just downloaded. To do this open the newly created file core/phpcs.xml. Find the line containing:
<rule ref="Drupal">
Change it to:
<rule ref="../vendor/drupal/coder/coder_sniffer/Drupal">
Or if you prefer the command line:
$ cd core/ $ cp phpcs.xml.dist phpcs.xml $ perl -pi -e "s|ref=\"Drupal|ref=\"../vendor/drupal/coder/coder_sniffer/Drupal|" phpcs.xml
Step 4: Run the test
Now you are ready to run the test! From within the core/ folder, run the following command to launch the test:
$ ../vendor/bin/phpcs -p
This takes a couple of minutes. The -p flag shows the progress, so you have a bunch of nice dots to look at while it is running.
Step 5: Fix the failures
When the test is complete it will present you a list of all the files that contain violations of your sniff, and the line numbers where the violations occur. Go ahead and fix them all!
| Comment | File | Size | Author |
|---|---|---|---|
| #30 | 2572307-30.patch | 34.05 KB | alexpott |
| #26 | Generic.PHP_.UpperCaseConstant-2572307-25.patch | 46.13 KB | vprocessor |
| #26 | Generic.PHP_.UpperCaseConstant-2572307-25.interdiff.txt | 38.12 KB | vprocessor |
| #24 | Generic.PHP_.UpperCaseConstant-2572307-24.patch | 84.24 KB | vprocessor |
| #11 | fix-2572307-11.patch | 77.97 KB | andriyun |
Comments
Comment #2
attiks commentedComment #4
attiks commentedComment #8
duaelfrAs agreed between the mentors at Drupalcon, according to issues to avoid for novices, I am untagging this issue as "Beginner". This issue contains changes across a very wide range of files and might create too many other patches to need to be rerolled at this particular time. This patch has an automated way to be rerolled later so better to implement it after Drupalcon.
Comment #9
andriyun commentedPatch is outdated.
Need reroll
Comment #10
andriyun commentedComment #11
andriyun commentedNew patch created using phpcbf tool.
Comment #12
andriyun commentedComment #13
attiks commentedLooks good, thanks for reroll
Comment #14
andypostrtbc +1
checked with phpcbf 2.3.3 and get the same patch
Comment #15
tstoecklerEven though it is pretty weird the
ArchiveTarclass is third-party code, so we should not modify it.Comment #16
andypostLeaving the file in this state makes no sense because it will break all work that happens to make core pass code review.
Suppose we should remove third-party into vendor and leave BC shim in separate issue.
Also we have nice example in Diff namespace when third-party code was converted to proper core-compatible units
Comment #17
attiks commented#15 As said in #16, if it's third party, it should live in vendor, if we start adding exception, there's no end at the tunnel. If it stays it should follow the same standards.
Comment #18
andypostFiled #2610984: Add Archive Tar via Composer, with BC shim
this file looks outdated and needs proper review in context of #2400407: [meta] Ensure vendor (PHP) libraries are on latest stable release
Comment #19
catchPostponing this on #2610984: Add Archive Tar via Composer, with BC shim.
Comment #20
pfrenssenComment #22
andypostnow the file bypassed so we can continue here
Comment #23
vprocessor commentedComment #24
vprocessor commentedHi guys,
rerolled and refixed with phpcbf
Comment #25
tstoecklerApparently this is still a problem, because the patch includes changes to
ArchiveTar.Comment #26
vprocessor commentedcore/lib/Drupal/Core/Archiver/ArchiveTar.php - changes removed from patch
Comment #27
vprocessor commentedComment #28
vprocessor commentedComment #29
alexpottThis class is obeying symfony standards because it is a copy should have an
See here
This file has a // @codingStandardsIgnoreFile at the top - shouldn't be changed.
These also has a @codingStandardsIgnoreStart shouldn't be changed here.
Comment #30
alexpottI've added the suggested exclusions and re-run phpcs/phpcbf
Comment #31
dawehnerNice, thank you! We are slowly slowly getting there
Comment #32
alexpottCommitted 94a968e and pushed to 8.1.x and 8.2.x. Thanks!