Drupal.Classes.FullyQualifiedNamespace sniff doesn't work with attributes

Thanks for reporting! Can you post some test code where this triggers? Then we can try to reproduce here.

I've tried this again with Action Link plugins. Here's my code after using Rector and adding linebreaks so each unimported class is on its own line:

<?php

namespace Drupal\action_link_test_plugins\Plugin\StateAction;

use Drupal\action_link\Entity\ActionLinkInterface;
use Drupal\action_link\Plugin\StateAction\StateActionBase;
use Drupal\Core\Access\AccessResult;
use Drupal\Core\Session\AccountInterface;

/**
 * Test action which is always usable.
 */
#[\Drupal\action_link\Attribute\StateAction(
  id: 'test_always',
  label: new \Drupal\Core\StringTranslation\TranslatableMarkup('Test Always'),
  description: new \Drupal\Core\StringTranslation\TranslatableMarkup('Test Always'),
  directions: [
    'change' => 'change',
  ]
)]
class TestAlways extends StateActionBase {
SNIP

phpcs:

vendor/bin/phpcs --standard=Drupal  -s --sniffs=Drupal.Classes.FullyQualifiedNamespace web/modules/contrib/action_link/tests/modules/action_link_test_plugins/src/Plugin/StateAction/TestAlways.php

FILE: /Users/joachim/Sites/drupal-contrib/web/modules/contrib/action_link/tests/modules/action_link_test_plugins/src/Plugin/StateAction/TestAlways.php
-------------------------------------------------------------------------------------------------------------------------------------------------------
FOUND 2 ERRORS AFFECTING 2 LINES
-------------------------------------------------------------------------------------------------------------------------------------------------------
 15 | ERROR | [x] Namespaced classes/interfaces/traits should be referenced with use statements
    |       |     (Drupal.Classes.FullyQualifiedNamespace.UseStatementMissing)
 16 | ERROR | [x] Namespaced classes/interfaces/traits should be referenced with use statements
    |       |     (Drupal.Classes.FullyQualifiedNamespace.UseStatementMissing)
-------------------------------------------------------------------------------------------------------------------------------------------------------
PHPCBF CAN FIX THE 2 MARKED SNIFF VIOLATIONS AUTOMATICALLY
-------------------------------------------------------------------------------------------------------------------------------------------------------

It's finding the two uses of Drupal\Core\StringTranslation\TranslatableMarkup, but not the attribute class.

Similarly, phpcbf imports Drupal\Core\StringTranslation\TranslatableMarkup but doesn't change the attribute class.

Thanks, pushed a fix!

This is breaking Drush command classes where we use to have partially namespaces:

#[CLI\Command(name: 'cas:set-cas-username')]

This is breaking Drush command classes where we use to have partially namespaces:

Yes, I now have upwards of 50 PHPCS failures in some of my projects because of this.

• As of 7 Jan PHPCS jobs on GitLabCI reported no errors for my Drush command classes. (See https://git.drupalcode.org/project/regcode/-/jobs/3931521)
• As of 8 Jan I now get many of these errors: Drupal.Classes.FullyQualifiedNamespace.UseStatementMissing (See https://git.drupalcode.org/project/regcode/-/jobs/3947317)

Specifically, I have a lot of Drush commands across a lot of projects. I converted them all to using Attributes a while back.

The Drush standard usage and all the Drush core commands and documentation use this pattern to define Drush commands:

use Drush\Attributes as CLI;

final class RulesDrushCommands extends DrushCommands {

  #[CLI\Command(name: 'rules:list', aliases: ['rules-list', 'rlst'])]
  #[CLI\Help(description: 'Lists all the active and inactive rules for your site.')]
  #[CLI\Argument(name: 'type', description: "Type of Rule. Either 'rule' or 'component'.")]
  #[CLI\Usage(name: 'drush rules:list', description: 'Lists both Reaction Rules and Rules Components.')]
  #[CLI\Usage(name: 'drush rules:list component', description: 'Lists only Rules Components.')]
  #[CLI\Usage(name: 'drush rules:list --fields=machine-name', description: 'Lists just the machine names.')]
  #[CLI\Usage(name: 'drush rules:list --fields=machine-name --pipe', description: 'Outputs machine names in a format suitable for piping.')]
  #[CLI\FieldLabels(labels: ['machine-name' => 'Rule', 'label' => 'Label', 'event' => 'Event', 'active' => 'Active'])]
  #[CLI\DefaultFields(fields: ['machine-name', 'label', 'event', 'active'])]
  public function listAll(string $type = '', array $options = ['format' => 'table', 'fields' => '']): RowsOfFields {
    :
    :
  }

But this now fails PHPCS testing.

Changing this in all my Drush commands will mean several hundred lines of code need to be changed, and this will be a persistent problem because as I said all the core Drush commands which people use as examples, and all the Drush documentation, uses this pattern with is now deemed invalid.

Very sorry about this flagging Drush attributes. I was hoping to catch this error before the Coder release by running it on an internal codebase, but the composer install of Coder 8.3.x-dev installed an outdated version. Note to self: never trust composer with dev versions and verify that it actually has installed the latest commit.

Now back to the issue about fully qualified names in PHP attributes: The usual Drupal coding standards forbid fully or partially qualified names. "Specify a single class per use statement. Do not specify multiple classes in a single use statement." https://www.drupal.org/docs/develop/coding-standards/namespaces →

So the coding standards in PHP attributes from Drush violate the Drupal coding standards by using partially qualified names.

However, since this Drush standard is established and used so widely I think that we should revert this issue. Coder is technically correct, but I think we need a coding standards discussion first. Then we should write down PHP attributes coding standards, especially if fully or partially namespaced classes are allowed there.

Another reason to revert this is Symfony. They do weird aliasing with use statements as well, see 🐛 False Postive: Aliased PHP 8.0 Attributes on Class properties leading to "Namespaced classes/interfaces/traits should be referenced with use statements" Needs work .

So for now I think PHP attributes are undefined and lawless territory regarding use statements and namespaces, so Coder should not flag it.

Sorry Joachim, at least you can use the fixer in Coder version 8.3.27.

Workarounds for now:
use the Slevomat sniff:

  <rule ref="Drupal">
    <!-- @todo Disabling this sniff until PHP attributes are ignored. -->
    <exclude name="Drupal.Classes.FullyQualifiedNamespace.UseStatementMissing" />
  </rule>
  
  <!-- @todo Using this sniff while
    Drupal.Classes.FullyQualifiedNamespace.UseStatementMissing is disabled. -->
  <rule ref="SlevomatCodingStandard.Namespaces.ReferenceUsedNamesOnly">
    <properties>
      <property name="allowFullyQualifiedGlobalClasses" value="true"/>
      <property name="allowFullyQualifiedGlobalFunctions" value="true"/>
      <property name="allowFullyQualifiedGlobalConstants" value="true"/>
      <property name="allowFallbackGlobalFunctions" value="true"/>
      <property name="allowFallbackGlobalConstants" value="true"/>
    </properties>
  </rule>
  <rule ref="SlevomatCodingStandard.Namespaces.ReferenceUsedNamesOnly.ReferenceViaFullyQualifiedName">
    <exclude-pattern>*.api.php</exclude-pattern>
  </rule>
  <rule ref="SlevomatCodingStandard.Namespaces.ReferenceUsedNamesOnly.ReferenceViaFullyQualifiedNameWithoutNamespace">
    <severity>0</severity>
  </rule>

I will check how to revert and cover this with test cases.

And here is the revert with more test cases: https://github.com/pfrenssen/coder/pull/258

Let me know if you have any objections, will merge this soonish.

> Sorry Joachim, at least you can use the fixer in Coder version 8.3.27.

Thanks! :)

I can report it works perfectly for converting plugins to us attributes! :)

What's the reasoning for the partial namespace import for Drush command attributes? Why not import the whole thing, or alias them if there's risk of a name collision?

> What's the reasoning for the partial namespace import for Drush command attributes? Why not import the whole thing, or alias them if there's risk of a name collision?

I'd say because the attribute name themself are very generic (Command, Help, Usage, ...) and CLI gives them context. Technically it works correctly, but it's just against the common usage that drush itself has established, so maybe it just needs an exception for that.

Also, it sometimes takes a tall wall of Attributes to specify a command. That wall gets unnecessarily wide if you need a full namespace in every line. That would be so tedious to read.

I thought about what @Berdir said, if we should make an allow list of namespaces like "Cli" for Drush commands, so that Coder does not complain on all the defined Drush commands out there. We could even make it configurable, then people can set their own list of allowed names in attributes.

On the other hand: why even bother? There is also a pattern in Symfony emerging that seems to encourage namespacing and aliases in PHP attributes. I have a whole rant about too short class names in #3408527-4: False Postive: Aliased PHP 8.0 Attributes on Class properties leading to "Namespaced classes/interfaces/traits should be referenced with use statements" → . Drush made the same mistake: "Command" is not a google class name because it is ambiguous. Better would be "DrushCommand" for example. Library authors take notice: PHP attributes should be meaningful on their own and not be ambiguous.

So I think Coder should not check for namespaces in attributes at all for now. I would like to see this agreed upon in our coding standards first before we enable checks in Coder again.

How can you help?
1) Please open a coding standards issue and propose a way forward: allow all namespaces? only aliases? or don't allow it as everywhere else outside of attributes?
2) I would be willing to accept contributions to Coder where developers can opt in with a config setting like "checkInAttributes" true/false + "allowedNamespacesInAttributes" array to make an exception for "Cli"

I will push the proposed revert now.

Updating credits for all the discussion participants, thank you!

> Library authors take notice: PHP attributes should be meaningful on their own and not be ambiguous.

Agreed!

> So I think Coder should not check for namespaces in attributes at all for now

But where does that leave contrib module authors wanting to update their annotated plugins to use attributes?

Contrib authors can still update their annotated plugins and Coder will not complain whether they use full namespaces there or not.

Coder 8.3.28 was released with the revert.

> Contrib authors can still update their annotated plugins and Coder will not complain whether they use full namespaces there or not.

Yes, but what I mean is that they'll end up with ugly full class attributes, e.g.

#[Drupal\Core\Block\Attribute\Block(

and they have to fix them all manually, as phpcbf won't do it.

Could we keep the code that this issue added, but not put it in the Drupal standards set? That way, it can still be used when converting annotations to attributes.

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

Could be possible, we could use a config option that users can switch on to check and fix full namespaces in attributes. Please open a new issue for that!