\Drupal calls should be avoided in classes, use dependency injection instead [#3214545]

Problem/Motivation

From Codde Sniffer DrupalPractices.

$ /app/vendor/bin/phpcs --standard=Drupal,DrupalPractice --extensions=php,module,inc,install,test,profile,theme,css,info,txt,md web/modules/contrib/google_analytics_counter/

FILE: ...tics_counter/src/Controller/GoogleAnalyticsCounterController.php
----------------------------------------------------------------------
FOUND 0 ERRORS AND 2 WARNINGS AFFECTING 2 LINES
----------------------------------------------------------------------
  92 | WARNING | \Drupal calls should be avoided in classes, use
     |         | dependency injection instead
 344 | WARNING | \Drupal calls should be avoided in classes, use
     |         | dependency injection instead
----------------------------------------------------------------------


FILE: ...oogle_analytics_counter/src/GoogleAnalyticsCounterAppManager.php
----------------------------------------------------------------------
FOUND 0 ERRORS AND 5 WARNINGS AFFECTING 5 LINES
----------------------------------------------------------------------
 180 | WARNING | \Drupal calls should be avoided in classes, use
     |         | dependency injection instead
 447 | WARNING | \Drupal calls should be avoided in classes, use
     |         | dependency injection instead
 469 | WARNING | Exceptions should not be translated
 498 | WARNING | \Drupal calls should be avoided in classes, use
     |         | dependency injection instead
 509 | WARNING | \Drupal calls should be avoided in classes, use
     |         | dependency injection instead
----------------------------------------------------------------------


FILE: ...e_analytics_counter/src/GoogleAnalyticsCounterMessageManager.php
----------------------------------------------------------------------
FOUND 0 ERRORS AND 1 WARNING AFFECTING 1 LINE
----------------------------------------------------------------------
 102 | WARNING | t() calls should be avoided in classes, use
     |         | \Drupal\Core\StringTranslation\StringTranslationTrait
     |         | and $this->t() instead
----------------------------------------------------------------------


FILE: ...cs_counter/src/Form/GoogleAnalyticsCounterConfigureTypesForm.php
----------------------------------------------------------------------
FOUND 0 ERRORS AND 2 WARNINGS AFFECTING 2 LINES
----------------------------------------------------------------------
 114 | WARNING | \Drupal calls should be avoided in classes, use
     |         | dependency injection instead
 159 | WARNING | \Drupal calls should be avoided in classes, use
     |         | dependency injection instead
----------------------------------------------------------------------


FILE: ...le_analytics_counter/src/Form/GoogleAnalyticsCounterAuthForm.php
----------------------------------------------------------------------
FOUND 0 ERRORS AND 2 WARNINGS AFFECTING 2 LINES
----------------------------------------------------------------------
 181 | WARNING | \Drupal calls should be avoided in classes, use
     |         | dependency injection instead
 182 | WARNING | \Drupal calls should be avoided in classes, use
     |         | dependency injection instead
----------------------------------------------------------------------


FILE: ...nalytics_counter/src/Form/GoogleAnalyticsCounterSettingsForm.php
----------------------------------------------------------------------
FOUND 0 ERRORS AND 2 WARNINGS AFFECTING 1 LINE
----------------------------------------------------------------------
 280 | WARNING | \Drupal calls should be avoided in classes, use
     |         | dependency injection instead
 280 | WARNING | t() calls should be avoided in classes, use
     |         | \Drupal\Core\StringTranslation\StringTranslationTrait
     |         | and $this->t() instead
----------------------------------------------------------------------


FILE: ...ytics_counter/src/GoogleAnalyticsCounterCustomFieldGenerator.php
----------------------------------------------------------------------
FOUND 0 ERRORS AND 10 WARNINGS AFFECTING 10 LINES
----------------------------------------------------------------------
  95 | WARNING | \Drupal calls should be avoided in classes, use
     |         | dependency injection instead
 130 | WARNING | \Drupal calls should be avoided in classes, use
     |         | dependency injection instead
 143 | WARNING | t() calls should be avoided in classes, use
     |         | \Drupal\Core\StringTranslation\StringTranslationTrait
     |         | and $this->t() instead
 151 | WARNING | \Drupal calls should be avoided in classes, use
     |         | dependency injection instead
 157 | WARNING | t() calls should be avoided in classes, use
     |         | \Drupal\Core\StringTranslation\StringTranslationTrait
     |         | and $this->t() instead
 162 | WARNING | \Drupal calls should be avoided in classes, use
     |         | dependency injection instead
 172 | WARNING | \Drupal calls should be avoided in classes, use
     |         | dependency injection instead
 174 | WARNING | \Drupal calls should be avoided in classes, use
     |         | dependency injection instead
 244 | WARNING | \Drupal calls should be avoided in classes, use
     |         | dependency injection instead
 245 | WARNING | \Drupal calls should be avoided in classes, use
     |         | dependency injection instead
----------------------------------------------------------------------


FILE: ...ogle_analytics_counter/src/GoogleAnalyticsCounterAuthManager.php
----------------------------------------------------------------------
FOUND 1 ERROR AFFECTING 1 LINE
----------------------------------------------------------------------
 144 | ERROR | The $_GET super global must not be accessed directly;
     |       | inject the request_stack service and use
     |       | $stack->getCurrentRequest()->query->get('code')
     |       | instead
----------------------------------------------------------------------


FILE: ...trib/google_analytics_counter/src/GoogleAnalyticsCounterFeed.php
----------------------------------------------------------------------
FOUND 1 ERROR AFFECTING 1 LINE
----------------------------------------------------------------------
 172 | ERROR | The $_GET super global must not be accessed directly;
     |       | inject the request_stack service and use
     |       | $stack->getCurrentRequest()->query->get('code')
     |       | instead
----------------------------------------------------------------------


FILE: ...unter/src/Plugin/QueueWorker/GoogleAnalyticsCounterQueueBase.php
----------------------------------------------------------------------
FOUND 0 ERRORS AND 1 WARNING AFFECTING 1 LINE
----------------------------------------------------------------------
 18 | WARNING | There must be no blank line following an inline
    |         | comment
----------------------------------------------------------------------


FILE: ...ib/google_analytics_counter/src/GoogleAnalyticsCounterHelper.php
----------------------------------------------------------------------
FOUND 0 ERRORS AND 2 WARNINGS AFFECTING 2 LINES
----------------------------------------------------------------------
  25 | WARNING | Unused variable $t_arg.
 123 | WARNING | There must be no blank line following an inline
     |         | comment
----------------------------------------------------------------------


FILE: ...ontrib/google_analytics_counter/google_analytics_counter.install
----------------------------------------------------------------------
FOUND 0 ERRORS AND 1 WARNING AFFECTING 1 LINE
----------------------------------------------------------------------
 290 | WARNING | Unused variable $key.
----------------------------------------------------------------------


FILE: ...counter/tests/src/Functional/GoogleAnalyticsCounterBlockTest.php
----------------------------------------------------------------------
FOUND 0 ERRORS AND 1 WARNING AFFECTING 1 LINE
----------------------------------------------------------------------
 141 | WARNING | t() calls should be avoided in classes, use
     |         | \Drupal\Core\StringTranslation\StringTranslationTrait
     |         | and $this->t() instead
----------------------------------------------------------------------


FILE: ...nter/tests/src/Functional/GoogleAnalyticsCounterSettingsTest.php
----------------------------------------------------------------------
FOUND 0 ERRORS AND 2 WARNINGS AFFECTING 2 LINES
----------------------------------------------------------------------
 88 | WARNING | t() calls should be avoided in classes, use
    |         | \Drupal\Core\StringTranslation\StringTranslationTrait
    |         | and $this->t() instead
 89 | WARNING | t() calls should be avoided in classes, use
    |         | \Drupal\Core\StringTranslation\StringTranslationTrait
    |         | and $this->t() instead
----------------------------------------------------------------------


FILE: .../tests/src/Functional/GoogleAnalyticsCounterAuthSettingsTest.php
----------------------------------------------------------------------
FOUND 0 ERRORS AND 1 WARNING AFFECTING 1 LINE
----------------------------------------------------------------------
 63 | WARNING | t() calls should be avoided in classes, use
    |         | \Drupal\Core\StringTranslation\StringTranslationTrait
    |         | and $this->t() instead
----------------------------------------------------------------------


FILE: /app/web/modules/contrib/google_analytics_counter/README.md
----------------------------------------------------------------------
FOUND 1 ERROR AND 39 WARNINGS AFFECTING 40 LINES
----------------------------------------------------------------------
  15 | WARNING | [ ] Line exceeds 80 characters; contains 147
     |         |     characters
  18 | WARNING | [ ] Line exceeds 80 characters; contains 82
     |         |     characters
  19 | WARNING | [ ] Line exceeds 80 characters; contains 88
     |         |     characters
  20 | WARNING | [ ] Line exceeds 80 characters; contains 107
     |         |     characters
  26 | WARNING | [ ] Line exceeds 80 characters; contains 104
     |         |     characters
  32 | WARNING | [ ] Line exceeds 80 characters; contains 123
     |         |     characters
  33 | WARNING | [ ] Line exceeds 80 characters; contains 157
     |         |     characters
  34 | WARNING | [ ] Line exceeds 80 characters; contains 262
     |         |     characters
  35 | WARNING | [ ] Line exceeds 80 characters; contains 165
     |         |     characters
  39 | WARNING | [ ] Line exceeds 80 characters; contains 188
     |         |     characters
  41 | WARNING | [ ] Line exceeds 80 characters; contains 265
     |         |     characters
  43 | WARNING | [ ] Line exceeds 80 characters; contains 177
     |         |     characters
  44 | WARNING | [ ] Line exceeds 80 characters; contains 118
     |         |     characters
  46 | WARNING | [ ] Line exceeds 80 characters; contains 89
     |         |     characters
  48 | WARNING | [ ] Line exceeds 80 characters; contains 243
     |         |     characters
  54 | WARNING | [ ] Line exceeds 80 characters; contains 110
     |         |     characters
  60 | WARNING | [ ] Line exceeds 80 characters; contains 219
     |         |     characters
  63 | WARNING | [ ] Line exceeds 80 characters; contains 248
     |         |     characters
  71 | WARNING | [ ] Line exceeds 80 characters; contains 176
     |         |     characters
  73 | WARNING | [ ] Line exceeds 80 characters; contains 146
     |         |     characters
  75 | WARNING | [ ] Line exceeds 80 characters; contains 81
     |         |     characters
  88 | WARNING | [ ] Line exceeds 80 characters; contains 185
     |         |     characters
  95 | WARNING | [ ] Line exceeds 80 characters; contains 92
     |         |     characters
 111 | WARNING | [ ] Line exceeds 80 characters; contains 147
     |         |     characters
 112 | WARNING | [ ] Line exceeds 80 characters; contains 84
     |         |     characters
 118 | WARNING | [ ] Line exceeds 80 characters; contains 264
     |         |     characters
 120 | WARNING | [ ] Line exceeds 80 characters; contains 204
     |         |     characters
 124 | WARNING | [ ] Line exceeds 80 characters; contains 157
     |         |     characters
 128 | WARNING | [ ] Line exceeds 80 characters; contains 107
     |         |     characters
 137 | WARNING | [ ] Line exceeds 80 characters; contains 107
     |         |     characters
 155 | WARNING | [ ] Line exceeds 80 characters; contains 120
     |         |     characters
 161 | WARNING | [ ] Line exceeds 80 characters; contains 131
     |         |     characters
 168 | WARNING | [ ] Line exceeds 80 characters; contains 110
     |         |     characters
 172 | WARNING | [ ] Line exceeds 80 characters; contains 131
     |         |     characters
 180 | WARNING | [ ] Line exceeds 80 characters; contains 124
     |         |     characters
 182 | WARNING | [ ] Line exceeds 80 characters; contains 161
     |         |     characters
 186 | WARNING | [ ] Line exceeds 80 characters; contains 107
     |         |     characters
 192 | WARNING | [ ] Line exceeds 80 characters; contains 413
     |         |     characters
 202 | WARNING | [ ] Line exceeds 80 characters; contains 96
     |         |     characters
 207 | ERROR   | [x] Expected 1 newline at end of file; 2 found
----------------------------------------------------------------------
PHPCBF CAN FIX THE 1 MARKED SNIFF VIOLATIONS AUTOMATICALLY
----------------------------------------------------------------------

Time: 2.98 secs; Memory: 16MB

Issue fork google_analytics_counter-3214545

Show commands

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

Add & fetch this issue fork’s repository

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

Add & fetch this issue fork’s repository

3214545-drupal-calls-should changes, plain diff MR !2
Check out this branch for the first time

Check out existing branch, if you already have it locally

1 hidden branch

3214545-drupal-calls-should-avoid

changes, plain diff MR !9

Check out this branch for the first time

Check out existing branch, if you already have it locally

About issue forks

Comments

Comment #1

18 May 2021 at 12:29

guilhermevp created an issue. See original summary.

Comment #2

18 May 2021 at 12:41

guilhermevp opened merge request !2

Comment #3

Willy Christian commented 2 February 2022 at 11:03

Assigned:

guilhermevp

» Willy Christian

I'll work on it.

Comment #4

Willy Christian commented 4 February 2022 at 18:22

Assigned:

Willy Christian

» Unassigned

I pushed what I think I was able to fix. Hope it helps.

Comment #5

Willy Christian commented 4 February 2022 at 18:34

Status:

Active

» Needs work

Comment #6

tmaiochi commented 7 February 2022 at 14:24

Assigned:

Unassigned

» tmaiochi

Working on that.

Comment #7

tmaiochi commented 7 February 2022 at 18:15

Assigned:	tmaiochi	» Unassigned
Status:	Needs work	» Needs review

I fixed all dependency injection problems. Kindly review it.

Comment #8

victoria-marina commented 7 February 2022 at 20:07

Assigned:

Unassigned

» victoria-marina

I'll review this.

Comment #9

victoria-marina commented 7 February 2022 at 20:59

Assigned:	victoria-marina	» Unassigned
Status:	Needs review	» Reviewed & tested by the community

After the latest commit, I've ran phpcs and all the dependency injection warnings were gone. It's a RTBC for me.

Comment #10

kaszarobert

Hungarian

commented 18 July 2022 at 19:33

Status:

Reviewed & tested by the community

» Needs work

This needs a reroll.

Comment #11

sophiavs commented 1 August 2022 at 14:32

Assigned:

Unassigned

» sophiavs

Hi, i'll be working on it

Comment #12

sophiavs commented 1 August 2022 at 17:07

I analyzed the changes made in the MR and didn't found any error in phpcs related to this issue, can you explain what would be the reroll to make mentioned in #10? Kaszarobert

Comment #13

kaszarobert

Hungarian

commented 1 August 2022 at 19:12

Issue summary:

View changes

I updated the issue summary with the current DrupalPractice code style issues. The main goal of this issue is to replace static \Drupal:: calls to proper dependency injection in PHP classes to make writing tests easier. The current merge request has conflicts because of the recent changes in the codebase. That's why I wrote this needs a reroll because no patch or branch can be applied or merged right now to the 8.x-3.x branch.

Comment #14

2 August 2022 at 11:23

sophiavs opened merge request !9

Comment #15

sophiavs commented 2 August 2022 at 11:26

Assigned:	sophiavs	» Unassigned
Status:	Needs work	» Needs review

I corrected the conflicts on the commit in#7 and created a new branch, but i didn't understand if the warning is saying to delete the last MR or something else, but the mr is mergeable now

Comment #16

diegors

He/Him

Portuguese, Brazil

Campinas - Brazil

commented 2 August 2022 at 14:27

Assigned:

Unassigned

» diegors

I'll be reviewing this one.

Comment #17

diegors

He/Him

Portuguese, Brazil

Campinas - Brazil

commented 3 August 2022 at 18:21

Assigned:

diegors

» Unassigned

I fixed some errors but still some phpcs erros.

Comment #18

gquisini commented 4 August 2022 at 11:14

Assigned:

Unassigned

» gquisini

I'll be doing the review

Comment #19

gquisini commented 4 August 2022 at 16:10

Assigned:

gquisini

» Unassigned

I found some problems when reviewing. I'm not sure if its related with the issue purpose, but...

When I was running the automated tests, I found a problem with path_alias.manager in Drupal\google_analytics_counter\Form\GoogleAnalyticsCounterAuthForm::create(). Was a typo, so I fixed this one.

And in Drupal\google_analytics_counter\GoogleAnalyticsCounterCustomFieldGenerator, has duplicated properties with deprecated values. This one looks more critical, maybe has a issue about it, so I didn't fix.

BEFORE:

FILE: /home/gquisini/Documents/contributing/9.5.x/modules/contrib/google_analytics_counter/src/GoogleAnalyticsCounterAuthManager.php
----------------------------------------------------------------------------------------------------
FOUND 1 ERROR AFFECTING 1 LINE
----------------------------------------------------------------------------------------------------
 144 | ERROR | The $_GET super global must not be accessed directly; inject the request_stack service and use $stack->getCurrentRequest()->query->get('code') instead
----------------------------------------------------------------------------------------------------

FILE: /home/gquisini/Documents/contributing/9.5.x/modules/contrib/google_analytics_counter/src/GoogleAnalyticsCounterFeed.php
----------------------------------------------------------------------------------------------------
FOUND 1 ERROR AFFECTING 1 LINE
----------------------------------------------------------------------------------------------------
 172 | ERROR | The $_GET super global must not be accessed directly; inject the request_stack service and use $stack->getCurrentRequest()->query->get('code') instead
----------------------------------------------------------------------------------------------------

FILE: /home/gquisini/Documents/contributing/9.5.x/modules/contrib/google_analytics_counter/src/GoogleAnalyticsCounterHelper.php
----------------------------------------------------------------------------------------------------
FOUND 0 ERRORS AND 1 WARNING AFFECTING 1 LINE
----------------------------------------------------------------------------------------------------
 123 | WARNING | There must be no blank line following an inline comment
----------------------------------------------------------------------------------------------------

FILE: /home/gquisini/Documents/contributing/9.5.x/modules/contrib/google_analytics_counter/README.md
----------------------------------------------------------------------------------------------------
FOUND 0 ERRORS AND 39 WARNINGS AFFECTING 39 LINES
----------------------------------------------------------------------------------------------------
...
----------------------------------------------------------------------------------------------------

AFTER:

FILE: /google_analytics_counter/src/GoogleAnalyticsCounterHelper.php
----------------------------------------------------------------------------------------------------
FOUND 0 ERRORS AND 1 WARNING AFFECTING 1 LINE
----------------------------------------------------------------------------------------------------
 123 | WARNING | There must be no blank line following an inline comment
----------------------------------------------------------------------------------------------------

FILE: /google_analytics_counter/README.md
----------------------------------------------------------------------------------------------------
FOUND 0 ERRORS AND 39 WARNINGS AFFECTING 39 LINES
----------------------------------------------------------------------------------------------------
 ...
----------------------------------------------------------------------------------------------------

Comment #20

gquisini commented 9 September 2022 at 13:27

Assigned:

Unassigned

» gquisini

Comment #21

gquisini commented 9 September 2022 at 13:57

Assigned:

gquisini

» Unassigned

Well, since nobody commented anything about GoogleAnalyticsCounterCustomFieldGenerator with duplicate properties and I didn't find any problem related to this issue. I removed the duplicate and fixed some phpcs errors.

Comment #22

lucassc

he/him

Portuguese, Brazil

Rio de Janeiro

commented 9 September 2022 at 16:47

Assigned:

Unassigned

» lucassc

Comment #23

lucassc

he/him

Portuguese, Brazil

Rio de Janeiro

commented 9 September 2022 at 20:19

Status:

Needs review

» Needs work

Hi!

Good catch, @gquisini! I confirmed that both bugs were introduced in this issue. You can see the 1st in the line 114 and the 2nd in line 68.

1) I also notice that Drupal\Component\Datetime\TimeInterface was replaced by Drupal\Component\Datetime\Time. Is there any particular reason for this? Generally interfaces are preferred over their classes whenever possible.

2) We introduced commented code in line 134 of src/Controller/GoogleAnalyticsCounterController.php, it should be removed.

3) Maybe we can skip the assignment $config_factory = $this->config; and use just $this->config instead, since we are already going to change this line.

Comment #24

lucassc

he/him

Portuguese, Brazil

Rio de Janeiro

commented 9 September 2022 at 20:30

Assigned:	lucassc	» Unassigned
Status:	Needs work	» Needs review

I committed my proposals in #23.

Please review this.

Comment #25

lucasbaralm

He/Him

commented 12 September 2022 at 14:14

Assigned:

Unassigned

» lucasbaralm

i will review this.

Comment #26

lucasbaralm

He/Him

commented 12 September 2022 at 18:00

Assigned:	lucasbaralm	» Unassigned
Status:	Needs review	» Reviewed & tested by the community

The latest MR commits fixed the dependency injection errors and kept the modules functionality. All tests are passing so I'm moving to RTBC.

Comment #27

6 November 2022 at 19:14

kaszarobert committed 22e2180 on 8.x-3.x

Issue #3214545 by guilhermevp, gquisini, lucassc, Willy Christian,...

Comment #28

kaszarobert

Hungarian

commented 6 November 2022 at 19:18

Status:

Reviewed & tested by the community

» Fixed

I tested the changes and I decided to help you, so I fixed errors I met:
- Drupal\Core\Config\ImmutableConfigException: Can not set values on immutable configuration google_analytics_counter.settings:general_settings.gac_type_article. Use \Drupal\Core\Config\ConfigFactoryInterface::getEditable() to retrieve a mutable configuration object in Drupal\Core\Config\ImmutableConfig->set() (line 27 of core/lib/Drupal/Core/Config/ImmutableConfig.php).
- an old Drupal 8 service EntityManager was still referenced, so needed to be changed to EntityDisplayRepository and EntityTypeManager.

Plus, I added an empty update hook to fix Fixing ArgumentCountError: Too few arguments to function Drupal\google_analytics_counter\GoogleAnalyticsCounterAuthManager::__construct(), 5 passed. because there were parameter changes in service constructors.

No more errors, so I decided to commit the code.

Comment #29

20 November 2022 at 19:19

Status:

Fixed

» Closed (fixed)

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

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

Problem/Motivation

Issue fork google_analytics_counter-3214545

Comments

Comment #1

Comment #2

Comment #3

Comment #4

Comment #5

Comment #6

Comment #7

Comment #8

Comment #9

Comment #10

Comment #11

Comment #12

Comment #13

Comment #14

Comment #15

Comment #16

Comment #17

Comment #18

Comment #19

BEFORE:

AFTER:

Comment #20

Comment #21

Comment #22

Comment #23

Comment #24

Comment #25

Comment #26

Comment #27

Comment #28

Comment #29

News items

Our community

Documentation

Drupal code base

Governance of community