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

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Mile23 created an issue. See original summary.

Mile23’s picture

Status: Active » Needs review
FileSize
78.87 KB

Generated by phpcfb and then reviewed to make sure use statements are correct and alphabetical.

Status: Needs review » Needs work

The last submitted patch, 2: 2715741_2.patch, failed testing.

pfrenssen’s picture

Issue summary: View changes
Mile23’s picture

Status: Needs work » Needs review
FileSize
78.66 KB
1.48 KB

Thanks, 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.

Status: Needs review » Needs work

The last submitted patch, 5: 2715741_5.patch, failed testing.

Mile23’s picture

Status: Needs work » Needs review
FileSize
78.69 KB
1.23 KB

Hopefully this gets the last lint error...

Mile23’s picture

Issue summary: View changes
alexpott’s picture

Status: Needs review » Needs work
  1. +++ b/core/modules/system/tests/modules/module_test/src/Controller/ModuleTestController.php
    @@ -1,4 +1,5 @@
     <?php
    +use Drupal\module_autoload_test\SomeClass;
     
     namespace Drupal\module_test\Controller;
    

    use before the namespace.

  2. +++ b/core/modules/taxonomy/tests/modules/taxonomy_crud/taxonomy_crud.module
    @@ -5,9 +5,12 @@
    +use Drupal\taxonomy\VocabularyInterface;
    +
    +
     /**
    

    This use is in a weird place

  3. +++ b/core/modules/views/tests/fixtures/update/argument-placeholder.php
    @@ -5,12 +5,16 @@
    +
    +
    
    +++ b/core/modules/views/tests/fixtures/update/duplicate-field-handler.php
    @@ -5,12 +5,16 @@
    +
    +
    
    +++ b/core/modules/views/tests/modules/views_test_data/views_test_data.module
    @@ -5,6 +5,9 @@
    +
    +
    
    +++ b/core/tests/bootstrap.php
    @@ -7,6 +7,9 @@
    +
    +
    

    Too many blank lines

The last submitted patch, 7: 2715741_7.patch, failed testing.

Mile23’s picture

Status: Needs work » Needs review
FileSize
79.14 KB
6.55 KB

Thanks 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.

alexpott’s picture

+++ b/core/modules/system/tests/fixtures/update/drupal-8.without_automated_cron.php
@@ -5,7 +5,10 @@
+
+

Still some additional space

Mile23’s picture

Got it... :-)

The patch applies cleanly to 8.1.x, so I'll run a test for 8.1.x as well.

pfrenssen’s picture

@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.

pfrenssen’s picture

Status: Needs review » Reviewed & tested by the community

This 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!

  1. +++ b/core/modules/field/tests/modules/field_test/field_test.module
    @@ -13,8 +13,9 @@
    -use Drupal\field\FieldStorageConfigInterface;
    +use Drupal\Core\Entity\EntityTypeInterface;
     use Drupal\Core\Form\FormStateInterface;
    +use Drupal\field\FieldStorageConfigInterface;
    

    Thanks for fixing the alphabetical ordering, this is one of my pet peeves :)

  2. +++ b/core/modules/system/src/Tests/TypedData/TypedDataTest.php
    @@ -2,6 +2,14 @@
    +use Drupal\Core\TypedData\Type\BinaryInterface;
    +use Drupal\Core\TypedData\Type\UriInterface;
    +use Drupal\Core\TypedData\Type\DurationInterface;
    +use Drupal\Core\TypedData\Type\DateTimeInterface;
    +use Drupal\Core\TypedData\Type\FloatInterface;
    +use Drupal\Core\TypedData\Type\IntegerInterface;
    +use Drupal\Core\TypedData\Type\StringInterface;
    +use Drupal\Core\TypedData\Type\BooleanInterface;
    

    It would be nice to have this in alphabetical order. This does not a blocker to get this issue accepted though.

  3. +++ b/core/modules/system/tests/modules/ajax_forms_test/ajax_forms_test.module
    @@ -5,7 +5,21 @@
    -use Drupal\Core\Ajax;
    +use Drupal\Core\Ajax\AddCssCommand;
    +use Drupal\Core\Ajax\SettingsCommand;
    +use Drupal\Core\Ajax\RestripeCommand;
    +use Drupal\Core\Ajax\RemoveCommand;
    +use Drupal\Core\Ajax\PrependCommand;
    +use Drupal\Core\Ajax\InsertCommand;
    +use Drupal\Core\Ajax\HtmlCommand;
    +use Drupal\Core\Ajax\InvokeCommand;
    +use Drupal\Core\Ajax\DataCommand;
    +use Drupal\Core\Ajax\CssCommand;
    +use Drupal\Core\Ajax\ChangedCommand;
    +use Drupal\Core\Ajax\BeforeCommand;
    +use Drupal\Core\Ajax\AppendCommand;
    +use Drupal\Core\Ajax\AlertCommand;
    +use Drupal\Core\Ajax\AfterCommand;
    

    Same here, would be nice to be alphabetically ordered.

  4. +++ b/core/modules/views_ui/src/Form/Ajax/ViewsFormBase.php
    @@ -3,17 +3,20 @@
     use Drupal\Component\Utility\Html;
    +use Drupal\Core\Ajax\AjaxResponse;
    +use Drupal\Core\Ajax\CloseModalDialogCommand;
     use Drupal\Core\Ajax\OpenModalDialogCommand;
     use Drupal\Core\Form\FormBase;
     use Drupal\Core\Form\FormState;
     use Drupal\Core\Form\FormStateInterface;
     use Drupal\Core\Render\BubbleableMetadata;
     use Drupal\Core\Render\RenderContext;
    +use Drupal\views\Ajax\HighlightCommand;
    +use Drupal\views\Ajax\ReplaceTitleCommand;
    +use Drupal\views\Ajax\ShowButtonsCommand;
    +use Drupal\views\Ajax\TriggerPreviewCommand;
     use Drupal\views\ViewEntityInterface;
    -use Drupal\views\Ajax;
    -use Drupal\views_ui\Ajax as AjaxUI;
    -use Drupal\Core\Ajax\AjaxResponse;
    -use Drupal\Core\Ajax\CloseModalDialogCommand;
    +use Drupal\views_ui\Ajax\SetFormCommand;
     use Symfony\Component\HttpFoundation\RedirectResponse;
    

    Same here.

  5. +++ b/core/tests/Drupal/KernelTests/Core/Entity/EntityFieldTest.php
    @@ -2,6 +2,10 @@
    +use Drupal\Core\TypedData\ComplexDataInterface;
    +use Drupal\Core\TypedData\ListInterface;
    +use Drupal\Core\Entity\TypedData\EntityDataDefinitionInterface;
    +use Drupal\Core\Entity\TypedData\EntityDataDefinition;
    

    Same here.

pfrenssen’s picture

FileSize
79.81 KB
4.2 KB

I 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.

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Committed 2d47677 and pushed to 8.1.x and 8.2.x. Thanks!

  • alexpott committed 6ab5a3a on 8.2.x
    Issue #2715741 by Mile23, pfrenssen: Fix 'Drupal.Classes....

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.