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

Comments

Mile23 created an issue. See original summary.

mile23’s picture

Status: Active » Needs review
StatusFileSize
new78.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
StatusFileSize
new78.66 KB
new1.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
StatusFileSize
new78.69 KB
new1.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
StatusFileSize
new79.14 KB
new6.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

StatusFileSize
new79.14 KB
new495 bytes

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

StatusFileSize
new79.81 KB
new4.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.