\Drupal calls should be avoided in classes, use dependency injection instead

Hi,

I am working on it.

Hi,

I have created a patch. Please review.

Thanks!

Hi,

Rerolled the patch and fix few issues. Please review the patch.

Thanks!

Hi,

Rerolled the patch and fix few issues and warnings for tests failing. Please review the patch.

Thanks!

Hi,

Rerolled the patch and fix few issues and warnings for tests failing. Please review the patch.

Thanks!

Thanks @gg24 that looks like good progress. These changes are not BC so if we are going to commit this it needs to be in before 2.x beta. If we can get a good patch in the next 2 weeks that would be great.

Here are some comments.

In many cases you can find code in Drupal Core that you can copy exactly. Here is some code I grabbed randomly from AjaxResponseAttachmentsProcessor based on a grep.

  /**
   * The module handler.
   *
   * @var \Drupal\Core\Extension\ModuleHandlerInterface
   */
  protected $moduleHandler;

   * @param \Drupal\Core\Extension\ModuleHandlerInterface $module_handler
   *   The module handler.

This shows three problems present in many places in your code:

1. Must use the interface not the class of the default implementation. Otherwise when someone injects a different class the code will fail. For example ModuleHandlerInterface instead of ModuleHandler.

2. For a comment containing the name of a service, please use spaces to make words: "The module handler".

3. For a @param comment use the full class name including a leading slash.

One other block copy error:

4. Name mismatch in SubscriptionsFormBase.php:

   * @param \Drupal\Core\Extension\ModuleHandlerInterface $module_handler
   *   The module handler.

I am working on this issue

Thanks @prabha1997.

Unfortunately as we have now had a public beta release for 2.x I can't commit a patch to 2.x. This will have to wait until 3.x, and there isn't a branch for that yet. Please can you hold off for now?

Hi @AdamPS! Does this issue still need to be addressed? If so, I would gladly work on it.

Thank you for the offer. Now is a good time - if you are quick😃.

We can make the changes in 3.x, provided that it has not yet had a beta release. I plan to make a beta soon, ideally within a week.

All right! Assigning it to myself then.

I rerolled patch #6, then opened an Issue Fork and made the changes proposed in #7. I made a MR so it becomes easier to see the differences between versions and commit the changes in the end.

According to PHPCS, there still are \Drupal calls in ConfirmationController.php, SubscriberExportForm.php and SubscriptionsFormBase.php. I will try to address these tomorrow and commit to the MR.

Great thanks. Even if we miss out a few cases it would still be good progress and worth committing.

Thanks for the feedback @AdamPS! I made a commit adding Dependency Injection in ConfirmationController.php and in the \Drupal call in SubscriberExportForm.php. Some things I would like to point:

1. In SubscriberExportForm.php I had some trouble because the entity.query service is deprecated (see entity.query service deprecated in favor of EntityStorageInterface::getQuery() and Deprecate entity.query service and replace with using the entity storage's getQuery() method) and I found that the QueryFactory usually used in its injection was already removed from Drupal 9 (see class QueryFactory).
So I changed it in favor of the getStorage() method, as the deprecation notice suggests, which resulted in changing the line:
$query = \Drupal::entityQuery('simplenews_subscriber')
To:
$query = $this->entityManager->getStorage('simplenews_subscriber')->getQuery()
And the service entity.query to entity_type.manager, so I could effectively get the entity query with a injected service.
If this is a unwanted change I can reverse it.

2. PHPCS warns about another dependency injection issue in SubscriberExportForm.php, but it is not from a \Drupal direct call, but from a direct $_GET use:

69 | ERROR   | The $_GET super global must not be accessed directly; inject the request.stack service and use
     |         | $stack->getCurrentRequest()->query->get('states') instead

There are several occurrences on this file. Should I address this in this issue too or in a separate issue to keep the scope of this one on \Drupal calls?

3. Corrected a mistake in the String Translation dependency injection in MailSettingsForm.php.

There are still \Drupal calls left in SubscriptionsFormBase.php, I will address these on Monday.

A large number of the changes in the MR are wrong.

ControllerBase already has the form builder and config factory services injected - you just need to use those, DON'T inject them again.

Likewise, ConfigFormBase already has the string translation service injected.

NewsletterForm too - at least two of the services you're injecting are already injected in the base EntityForm class.

Between these, that's well over half the patch that's not needed. I didn't review the rest in detail.

@marcos_lima Thanks for your progress so far. TR is very thorough on details like this so I would trust him, but he can be a little blunt😃. Please don't be discouraged. It's definitely worth checking if a service is already injected as that's an easy fix.

You don't need to fix every problem. Even if you only fix 50%, it's still good progress. However all the code in your patch would need to be 100% right before we can commit it. So if you are not sure, you can leave something out.

1. It makes sense to me. Although the variable for entity_type.manager would normally be called $entityTypeManager.

2. It's up to you. You would likely need to inject the 'request_stack'. If you search for ->getCurrentRequest() you can find lots of examples.

Actually, let's not rush this issue. True, 3.x will soon be beta, and we can't fix it in 3.x after that. However soon 4.x will open, and we can fix it there.

Let me note that injecting services like this is considered by core to be a backwards-compatible change - it's not an API change, so there is no need to defer the patch until the next major issue. It can be added at any point in the development cycle.

For the $_GET super global, I would handle that in a separate issue, since that's not really just a dependency injection issue, it's more of a porting to Drupal 8 issue since the last time $_GET was supposed to be used was Drupal 7.

I would prefer to get the patch fixed up and committed rather than postponed. I will be glad to review and make changes if necessary. I would have just re-rolled the above myself except I was away from my computer for the last several days, and also with the number of changes needed it would have been easier to just write a new patch rather than fix up the MR.

@AdamPS Sure, no problem, thanks for the heads up. Some of the injections @TR pointed out I inherited from the patch re-roll I did (such as the NewsletterForm.php ones), but anyway I should have spotted that, so my mistake too, and others I introduced myself. Besides the bluntness at least I learned new things. As per your comment #18.1, true, I changed the variable name to $entityTypeManager, thanks.

Just made a commit. This new commit changed:

1. Removed unnecessary injections (config factory and form builder) from ConfirmationController.php and used the already injected services.
2. Removed unnecessary string translation and used the already injected one in MailSettingsForm.php.
3. Removed unnecessary injections (module handler and logger) from NewsletterForm.php and used the already injected ones. TR didn’t specify which ones were wrong but from what I figured out, it was those two.
4. In SubscriberExportForm.php the only injected service was the entity type manager, I don’t think this one was already injected by a parent class.
5. In SubscriptionSettingsForm.php the only injected service was the module handler, I think it’s the same case as 4.
6. In SimplenewsSubsctriptionBlock.php the injected services were the uuid and the current user, and I think it’s the same case as 4 too.

And I think that’s the status of all the modified files. Feel free to correct me about those points.
The monitoring module related test continues to fail and I think that’s related to the issue Create a composer.json file.
Maybe the MR is a little cleaner now and I hope that now whoever wants to work on it can continue from there instead of having to make another one from scratch.

Great thanks @marcos_lima that looks good to me. It would be great to have another review from @TR.

One simple way to do that in constructors is to have any new service be an optional parameter that is initialized to \Drupal::service('foo) if not provided. And we can already mark the parameter no longer being optional in 4.x.

Yes that's true, I have seen that done in core. However it's more work, and so I'd rather avoid it. I would propose either to commit to 3.x now, or create 4.x soon and commit to that.

Core is quite strict on the BC part, you can do it in the same way to be certain, by making the arguments optional and fall back to \Drupal::service() in the constructor, up to us and mostly Adam to decide how much he cares about that. Agreed that it is unlikely to be subclasses like that.

Also, injecting classes that exist on ControllerBase isn't wrong, just not that commonly done. The respective methods are specifically designed to support explicit injection, it is _not_ injected by default, just does a fallback t the service container. See \Drupal\Core\Controller\ControllerBase::entityTypeManager() for example. Also, ControllerBase has a lot more of those than FormBase so many of these really are required.

Thanks. I agree with the reasoning in #24 so let's commit it to 3.x.

Strange I can't see any "retest" link for the MR, only for the patches.

Indeed @AdamPS. I think it's a bug that happens sometimes, for me it wasn't showing too. Then I refreshed the page with ctrl+F5 to clear cache and it worked. I ran a retest, let's see.

Great thanks. I tried to merge and it said "merging failed". Any idea why?

OK it worked after I hard-refreshed the page. Thanks everyone.