Problem/Motivation

Because GlobalDrupalSniff only checks the base class extended by the given class, it fails to detect a variety of instances where global Drupal calls are "bad." This might be a case where the base class simply isn't listed, such as any generic plugin that would extend PluginBase, or one that extends a class that extends one of the listed $baseClasses, or simply a generic service implementation that does not extend any other classes.

Steps to reproduce

Run DrupalPractice against any class that would go into /src/ that does not explicitly extend one of the classes listed in $baseClasses.

Proposed resolution

Replace the $baseClasses check with one that simply checks if this class's file resides in /src/, and add additional help text to the $warning to suggest that Plugins should implement ContainerFactoryPluginInterface.

Issue fork coder-3186635

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

About issue forks

Comments

mrweiner created an issue. See original summary.

mrweiner opened merge request !1

mrweiner’s picture

mrweiner commented
Title: GlobalDrupalSniff's use of $baseClasses is insufficient » GlobalDrupalSniff's use of $baseClasses is insufficient and fails to detect relevant cases
Status: Active » Needs review

Opened an MR that implements the Proposed Resolution from the issue description. Note that this does not yet include test coverage as I wanted to get the approach approved first. The patch also results in $extendsName being unused, but I didn't delete it.

mrweiner’s picture

mrweiner commented

Realizing that maybe this approach needs to make sure that static classes are excluded from the warnings as well?

klausi’s picture

klausi
he/him||they/them
Primary language German
Location 🇦🇹 Vienna
commented
Priority: Major » Normal
Status: Needs review » Needs work

Thanks, this sniff has changed a bit over the years because it triggered too many false positives. I think we should keep the base classes where we know it is safe to trigger a warning. In addition we could trigger the warning if "Plugin" is in the file path, I think that is a good idea.

Would that work for you?

Please create pull requests against https://github.com/pfrenssen/coder so that we see the test cases run there.

Setting priority down to normal, I think this is not a major a bug and Coder just underreports a couple of cases in this tricky sniff.

mrweiner’s picture

mrweiner commented

Thanks for the feedback, happy to open a PR. If the recommendation is for all OOP code to leverage DI, though, I'm curious what implementations in /src would we not want to flag?

Right now if I create a Utility class /src/SubscriberCheck.php, I might call \Drupal::currentUser() and \Drupal::entityTypeManager() directly. I'd imagine that this should be flagged as a violation, as it should really be added as a service using DI, but right now it's completely ignored. Am I misunderstanding something?

I guess I see now that there's a guard clause earlier to explicitly exclude static classes, so maybe I just don't get why that would be desirable.

klausi’s picture

klausi
he/him||they/them
Primary language German
Location 🇦🇹 Vienna
commented

Hm, I checked the history of this sniff and it seems we wrote it very restrictive from the beginning to not introduce false positives. There are Drupal core APIs where you cannot use constructor injection like the Typed Data API (for example Drupal\Core\TypedData\Plugin\DataType\Language). Then you would have \Drupal calls in classes for example that cannot be avoided. Not sure if Drupal core has many APIs left that have this disadvantage. You could run the sniff on Drupal core and check where it would report problems and how many instances we have.

Then we could open up this sniff more and make special exceptions where we know Drupal core does not allow constructor injection.

mrweiner’s picture

mrweiner commented

Actually just opened up a thread about this on Slack.

@jhodgdon

Drupal is also used a lot in Functional tests.

@larowlan

Entities, field type plugins and data type plugins currently have no scope for DI

@mikelutz

\Drupal calls are also used in core to maintain backwards compatibility. We can’t just add a new dependency injection, we have to make it optional, get it by \Drupal and throw a deprecation error if it isn’t passed so that existing code continues to work, We can eventually make the parameter required in the next major version and remove the \Drupal call

He said that they don't run DrupalPractice on Core, though, so I'd wager those bits would get flagged no matter what. For entities, somebody dropped in https://www.drupal.org/project/drupal/issues/2142515, so I'd imagine those should be flagged until that is resolved.

So given all of that, I wonder if it might be worth changing the sniff to use a deny list of some sort instead of an approve list (like you mentioned). We'd want to ignore anything in /tests, and then Entities and FieldType/DataType plugins. Entities should be fairly simple to pick up, but the latter two might be trickier since the way to tell is from their namespace and annotations. Do you know if phpcs gives us access either of those?

mrweiner’s picture

mrweiner commented
mrweiner’s picture

mrweiner commented
Related issues: +#3187536: Document cases where it's appropriate to use global \Drupal calls as opposed to DI

Opened a related issue in Coding Standards. Who knows when/if it will actually be addressed, but figured it made sense to link it here.

klausi’s picture

klausi
he/him||they/them
Primary language German
Location 🇦🇹 Vienna
commented

PHPCS is not good at detecting class hierarchies, I would recommend PHPStan + https://github.com/mglaman/phpstan-drupal instead which also has a check for \Drupal calls and uses an actual PHP abstract tree parser to analyze code.

Otherwise I'm open to switch to a deny list in Coder if we can improve the sniff that way.

klausi closed merge request !1

b_sharpe’s picture

b_sharpe commented
Status: Needs work » Needs review

Added https://github.com/pfrenssen/coder/issues/238, please review.

klausi’s picture

klausi
he/him||they/them
Primary language German
Location 🇦🇹 Vienna
commented
Status: Needs review » Needs work
Issue tags: +Needs tests

Thanks, reviewed https://github.com/pfrenssen/coder/pull/239

Approach makes sense, please add test coverage.