Reduce \Drupal usage in user module run-time code

Here is the patch for the issue, please review.

If we're fixing this form for \Drupal use then let's also fix $config = \Drupal::config('user.settings'); - that can be $config = $this->config('user.settings');

Plus in other similar issues about \Drupal usage we've pushed back and asked this to be done on the module level - so can we need to do

• \Drupal\user\RegisterForm::submitForm()
• \Drupal\user\RegisterForm::save()
• \Drupal\user\Plugin\Block\UserLoginBlock::build() - here we'll need to inject the form builder service, configFactory and the redirect.destination service - so that'll need to have a deprecation code path too
• \Drupal\user\Plugin\EntityReferenceSelection\UserSelection::entityQueryAlter - will need to inject the service
• \Drupal\user\Plugin\Validation\Constraint\UserMailRequiredValidator::validate - will need to implement Drupal\Core\DependencyInjection\ContainerInjectionInterface
• \Drupal\user\Plugin\views\argument_default\CurrentUser - will need to implement ::create()
• \Drupal\user\Plugin\views\field\Permissions::query - will need to inject the service
• \Drupal\user\Plugin\views\filter\Name::adminSummary - will need to implement ::create()

Covered all points as mentioned by @alexpott , Kindly review.

We can not used $this in static method i have removed from above page, kindly review

Solving test case failures , kindly review a new patch.

Kindly review a new patch.

@kiamlaluno thanks for reviewing the patch , yes i have noticed that and removed the implementation of ContainerInjectionInterface and tried to make compatible with PluginBase::create() in patch #16.

Tries to solve failure test , kindly review a new patch.

Hii @Hardik_Patel_12, @kiamlaluno
i have solved codesniffer issue regarding comment #15, also tries to solve test cases.

patch #28 got pass, kindly avoid #29, i am moving for review of #28.

There are some more coding standards that need to fixed in #28

phpcs --standard=DrupalPractice,Drupal core/modules/user/src/Plugin/views/filter/Name.php
-----------------------------------------------------------------------------------------------------------------------------
FOUND 10 ERRORS AFFECTING 9 LINES
-----------------------------------------------------------------------------------------------------------------------------
  57 | ERROR | [ ] Missing member variable doc comment
  59 | ERROR | [x] Missing function doc comment
  79 | ERROR | [x] Missing function doc comment
  90 | ERROR | [x] Missing function doc comment
 103 | ERROR | [x] Missing function doc comment
 133 | ERROR | [x] Missing function doc comment
 134 | ERROR | [x] Inline comments must start with a capital letter
 144 | ERROR | [x] Missing function doc comment
 145 | ERROR | [x] Inline comments must start with a capital letter
 145 | ERROR | [x] Inline comments must end in full-stops, exclamation marks, question marks, colons, or closing parentheses
---------------------------------------------------------------------------------------------------------------------------

phpcs --standard=DrupalPractice,Drupal core/modules/user/src/Plugin/views/field/Permissions.php
----------------------------------------------------------------------------------------------------
FOUND 6 ERRORS AFFECTING 4 LINES
----------------------------------------------------------------------------------------------------
 131 | ERROR | [x] Whitespace found at end of line
 132 | ERROR | [x] Line indented incorrectly; expected 2 spaces, found 4
 143 | ERROR | [x] Line indented incorrectly; expected 2 spaces, found 4
 143 | ERROR | [ ] Public method name "Permissions::render_item" is not in lowerCamel format
 143 | ERROR | [x] Expected 1 space before opening brace; found 0
 145 | ERROR | [x] Closing brace indented incorrectly; expected 4 spaces, found 2
----------------------------------------------------------------------------------------------------
PHPCBF CAN FIX THE 5 MARKED SNIFF VIOLATIONS AUTOMATICALLY
----------------------------------------------------------------------------------------------------

kindly review a new patch.

Thanks for work here. Some minor problems from the last patch:

1. +++ b/core/modules/user/src/AccountForm.php
   @@ -101,7 +101,7 @@ public function form(array $form, FormStateInterface $form_state) {
   -    // Only show name field on registration form or user can change own username.
   +    // Only show name field on registration form user can change own username.
   

   I think there should be:

   "Only show name field on registration form or when user can change own username."

2. +++ b/core/modules/user/src/Plugin/Block/UserLoginBlock.php
   @@ -33,6 +36,27 @@ class UserLoginBlock extends BlockBase implements ContainerFactoryPluginInterfac
   +   * Contains the configuration object factory.
   

   I think that "The config factory." will be better here.

3. +++ b/core/modules/user/src/Plugin/Block/UserLoginBlock.php
   @@ -33,6 +36,27 @@ class UserLoginBlock extends BlockBase implements ContainerFactoryPluginInterfac
   +   * Contains the configuration object factory.
   
   @@ -47,11 +71,20 @@ class UserLoginBlock extends BlockBase implements ContainerFactoryPluginInterfac
   +   * @param \Drupal\Core\Config\ConfigFactoryInterface $config
   

   $config_factory instead of $config will be more correctly here

4. +++ b/core/modules/user/src/Plugin/Block/UserLoginBlock.php
   @@ -47,11 +71,20 @@ class UserLoginBlock extends BlockBase implements ContainerFactoryPluginInterfac
   +   *   The configuration factory object.
   

   "The config factory."

5. +++ b/core/modules/user/src/Plugin/EntityReferenceSelection/UserSelection.php
   @@ -59,11 +67,14 @@ class UserSelection extends DefaultSelection {
   -  public function __construct(array $configuration, $plugin_id, $plugin_definition, EntityTypeManagerInterface $entity_type_manager, ModuleHandlerInterface $module_handler, AccountInterface $current_user, Connection $connection, EntityFieldManagerInterface $entity_field_manager = NULL, EntityTypeBundleInfoInterface $entity_type_bundle_info = NULL, EntityRepositoryInterface $entity_repository = NULL) {
   

   Required params should be passed before optional.

   But in that case, we can have potential BC problems(unsure about it).

6. +++ b/core/modules/user/src/Plugin/views/field/Permissions.php
   @@ -75,15 +87,21 @@ public function init(ViewExecutable $view, DisplayPluginBase $display, array &$o
   +   * Add anything to the query that we might need to.
   

   We should use {@inheritdoc} here.

7. +++ b/core/modules/user/src/Plugin/views/field/Permissions.php
   @@ -111,6 +129,17 @@ public function preRender(&$values) {
   +   * Renders a single item of a row.
   

   Here also should be {@inheritdoc}

8. +++ b/core/modules/user/src/Plugin/views/filter/Name.php
   @@ -16,8 +18,52 @@
   +   * Provide a simple textfield for equality.
   

   Here also should be used {@inheritdoc}

9. +++ b/core/modules/user/src/Plugin/views/filter/Name.php
   @@ -38,6 +84,9 @@ protected function valueForm(&$form, FormStateInterface $form_state) {
   +   * Validate the options form.
   

   {@inheritdoc} should be used

10. +++ b/core/modules/user/src/Plugin/views/filter/Name.php
    @@ -62,6 +114,9 @@ public function acceptExposedInput($input) {
    +   * Validate the exposed handler form.
    

    {@inheritdoc} should be used

11. +++ b/core/modules/user/src/Plugin/views/filter/Name.php
    @@ -92,8 +147,11 @@ public function validateExposed(&$form, FormStateInterface $form_state) {
    +   * Perform any necessary changes to the form values prior to storage.
    

    {@inheritdoc} should be used

12. +++ b/core/modules/user/tests/src/Unit/Plugin/Validation/Constraint/UserMailRequiredValidatorTest.php
    @@ -144,7 +144,8 @@ public function providerTestValidate() {
    -    $cases['Existing users without an email should be ignored if the current user is an administrator.'] = [$items->reveal(), FALSE, TRUE];
    +    $cases['Existing users without an email should be ignored if the current user is an administrator.']
    +      = [$items->reveal(), FALSE, TRUE];
    

    The previous variant looks more correctly for me

@init90 patch adjusted according to #37 please review the patch.

Hello @init90
Updated the changes as asked for.
Kindly review patch.

+++ b/core/modules/user/src/Plugin/Block/UserLoginBlock.php
@@ -47,11 +71,20 @@ class UserLoginBlock extends BlockBase implements ContainerFactoryPluginInterfac
    *   The route match.
+   * @param \Drupal\Core\Config\ConfigFactoryInterface config_factory
+   *   The config factory.

- * @param \Drupal\Core\Config\ConfigFactoryInterface config_factory
+ * @param \Drupal\Core\Config\ConfigFactoryInterface $config_factory

+++ b/core/modules/user/src/Plugin/Block/UserLoginBlock.php
@@ -145,7 +181,7 @@ public function build() {
-   * #lazy_builder callback; renders a form action URL including destination.
+   * Lazy_builder callback; renders a form action URL including destination.

Any thought on this? Unexpected change or change it to the below.

The #lazy_builder callback renders a form action URL including destination.

Change lazy_builder to Lazy_builder does not make sense even the first letter must be uppercase, or no _

- Lazy_builder
+ Lazy builder

Any thought on this? Unexpected change or change it to the below.

In core mostly used: "#lazy_builder callback;", so I think the optimal way doesn't change comment in the patch.

Some points which need to fix:

1. +++ b/core/modules/user/src/AccountForm.php
   @@ -101,7 +101,7 @@ public function form(array $form, FormStateInterface $form_state) {
   +    // Only show name field on registration form or when user can change own username.
   

   Now the comment is longer than 80 charsets, so the part of it should be moved to the next line.

2. +++ b/core/modules/user/src/Plugin/EntityReferenceSelection/UserSelection.php
   @@ -36,6 +37,13 @@ class UserSelection extends DefaultSelection {
   +   * Contains the configuration object factory.
   

   The config factory.

3. +++ b/core/modules/user/src/Plugin/EntityReferenceSelection/UserSelection.php
   @@ -59,11 +67,14 @@ class UserSelection extends DefaultSelection {
   +   * @param \Drupal\Core\Config\ConfigFactoryInterface $config
   

   $config_factory instead of $config

4. +++ b/core/modules/user/src/Plugin/EntityReferenceSelection/UserSelection.php
   @@ -59,11 +67,14 @@ class UserSelection extends DefaultSelection {
   +   *   The configuration factory object.
   

   The config factory.

5. +++ b/core/modules/user/src/Plugin/EntityReferenceSelection/UserSelection.php
   @@ -59,11 +67,14 @@ class UserSelection extends DefaultSelection {
   +  public function __construct(array $configuration, $plugin_id, $plugin_definition, EntityTypeManagerInterface $entity_type_manager, ModuleHandlerInterface $module_handler, AccountInterface $current_user, Connection $connection, EntityFieldManagerInterface $entity_field_manager = NULL, EntityTypeBundleInfoInterface $entity_type_bundle_info = NULL, EntityRepositoryInterface $entity_repository = NULL, ConfigFactoryInterface $config) {
   

   As I wrote earlier we shouldn't pass the required parameter after optional, but I don't know the correct solution in that case. Ignore the point yet.

6. +  /**
   +   * Renders a single item of a row.
   +   *
   +   * @param int $count
   +   *   The index of the item inside the row.
   +   * @param mixed $item
   +   *   The item for the field to render.
   +   *
   +   * @return string
   +   *   The rendered output.
   +   */
   +   public function render_item($count, $item) {
   

   Here should be {@inheritdoc} instead of comment.

7. +++ b/core/modules/user/src/RegisterForm.php
   @@ -103,7 +103,12 @@ public function save(array $form, FormStateInterface $form_state) {
   +    $this->logger('user')->notice('New user: %name %email.',
   +        [
   +          '%name' => $form_state->getValue('name'),
   +          '%email' => '<' . $form_state->getValue('mail') . '>',
   +          'type' => $account->toLink($this->t('Edit'), 'edit-form')->toString(),
   +        ]);
   

   It can be a bit nicer:

   $this->logger('user')->notice('New user: %name %email.', [
     '%name' => $form_state->getValue('name'),
     '%email' => '<' . $form_state->getValue('mail') . '>',
     'type' => $account->toLink($this->t('Edit'), 'edit-form')->toString(),
   ]);
   

@init90 Updated the changes please review the patch .

Working on this

Ingored #47, started from #43. Addressed concerns in #46 and #49

As I wrote earlier we shouldn't pass the required parameter after optional, but I don't know the correct solution in that case. Ignore the point yet.

ConfigFactoryInterface $config_factory = NULL) {

By changing the last one to optional.

The #lazy_builder callback: It renders a form action URL including the destination.

Exceeds 80 chars, break it to 3 lines -- Title, an empty line, the rest.

I also think that when the parameters of a class constructor are changed, there is something more to do, to provide BC.

I am thinking to do it without touch the class constructor(s) if BC matters.

See this post for more https://www.previousnext.com.au/blog/safely-extending-drupal-8-plugin-cl...

Revert IS changes unintendedly

Thanks, @jungle, looks really good.

One minor concern before RTBC, in UserLoginBlock we add RedirectDestination service but actually not use it. Let's start do it:)

Re-rolling the patch.

@abhisekmazumdar thanks, changes looks good. I missed that renderPlaceholderFormAction is static method and we cannot use injected RedirectDestination for it.

@kiamlaluno according to that https://www.drupal.org/core/d8-bc-policy#constructors seems that we can change the constructor params for plugins, so if you agree with it, then we can solve your second notice about the comment and go further.

@kiamlaluno you're right we really should provide BC layer. Here issue with a similar discussion #3018863: Make SystemMenuBlock's new constructor argument dependency optional

And issue which might make life much easier for such a situation: #3019332: Use final to define classes that are NOT extension points

As a resume, in order to move further need to solve points from #57

I performed Novice Triage on this issue. I am leaving the Novice tag on this issue because I think that updating the issue summary with some of the latest discussions would be a good novice task.

Only bug fixes and allowed changes during the beta phase should be filed against 9.0.x. New features, API additions, and deprecations should be filed against 9.1.x.

The scope of this issue is also questionable. While the end goal of this issue is a good one (using properly injected services where possible), we should scope changes by concept, not by file group or module. So, it would be better to have an issue that looked at \Drupal::foo() usage on a per-method basis, rather than a per-module basis. And if the scope of that turns out to be too large, then we would break it down an additional level with (e.g.) "Use $this->foo() instead of \Drupal::foo() in form classes."

So, work on this should be postponed until we split up the issues more correctly. I suspect there is an existing meta issue for this, so I would search for that meta and use it to rescope this. Thanks!

Yep that's it, thanks @jungle!

Alright, thanks everyone for your contributions so far!

I am closing this as a duplicate of #2729597: [meta] Replace \Drupal with injected services where appropriate in core. You can use the work in the patches from this and similar issues as starting points for new issues scoped by concept.