Hey, found some best practice issues that I'll try to fix :), here's the list

vagrant@homestead:~/Code/d8/modules/contrib/google_analytics$ phpcs --standard=DrupalPractice --extensions=php,module,inc,install,test,profile,theme,info,txt,md .

FILE: ...Code/d8/modules/contrib/google_analytics/google_analytics.module
----------------------------------------------------------------------
FOUND 0 ERRORS AND 2 WARNINGS AFFECTING 2 LINES
----------------------------------------------------------------------
 10 | WARNING | @author tags are not usually used in Drupal, because
    |         | over time multiple contributors will touch the code
    |         | anyway
 28 | WARNING | Global constants should not be used, move it to a
    |         | class or interface
----------------------------------------------------------------------


FILE: ...b/google_analytics/src/Form/GoogleAnalyticsAdminSettingsForm.php
----------------------------------------------------------------------
FOUND 0 ERRORS AND 37 WARNINGS AFFECTING 31 LINES
----------------------------------------------------------------------
 136 | WARNING | \Drupal calls should be avoided in classes, use
     |         | dependency injection instead
 153 | WARNING | t() calls should be avoided in classes, use
     |         | dependency injection and $this->t() instead
 154 | WARNING | t() calls should be avoided in classes, use
     |         | dependency injection and $this->t() instead
 156 | WARNING | t() calls should be avoided in classes, use
     |         | dependency injection and $this->t() instead
 158 | WARNING | \Drupal calls should be avoided in classes, use
     |         | dependency injection instead
 159 | WARNING | t() calls should be avoided in classes, use
     |         | dependency injection and $this->t() instead
 160 | WARNING | t() calls should be avoided in classes, use
     |         | dependency injection and $this->t() instead
 161 | WARNING | t() calls should be avoided in classes, use
     |         | dependency injection and $this->t() instead
 164 | WARNING | t() calls should be avoided in classes, use
     |         | dependency injection and $this->t() instead
 195 | WARNING | t() calls should be avoided in classes, use
     |         | dependency injection and $this->t() instead
 196 | WARNING | t() calls should be avoided in classes, use
     |         | dependency injection and $this->t() instead
 221 | WARNING | t() calls should be avoided in classes, use
     |         | dependency injection and $this->t() instead
 222 | WARNING | t() calls should be avoided in classes, use
     |         | dependency injection and $this->t() instead
 223 | WARNING | t() calls should be avoided in classes, use
     |         | dependency injection and $this->t() instead
 274 | WARNING | t() calls should be avoided in classes, use
     |         | dependency injection and $this->t() instead
 274 | WARNING | \Drupal calls should be avoided in classes, use
     |         | dependency injection instead
 274 | WARNING | t() calls should be avoided in classes, use
     |         | dependency injection and $this->t() instead
 274 | WARNING | t() calls should be avoided in classes, use
     |         | dependency injection and $this->t() instead
 282 | WARNING | \Drupal calls should be avoided in classes, use
     |         | dependency injection instead
 324 | WARNING | t() calls should be avoided in classes, use
     |         | dependency injection and $this->t() instead
 324 | WARNING | \Drupal calls should be avoided in classes, use
     |         | dependency injection instead
 324 | WARNING | t() calls should be avoided in classes, use
     |         | dependency injection and $this->t() instead
 324 | WARNING | t() calls should be avoided in classes, use
     |         | dependency injection and $this->t() instead
 332 | WARNING | \Drupal calls should be avoided in classes, use
     |         | dependency injection instead
 401 | WARNING | \Drupal calls should be avoided in classes, use
     |         | dependency injection instead
 410 | WARNING | \Drupal calls should be avoided in classes, use
     |         | dependency injection instead
 457 | WARNING | \Drupal calls should be avoided in classes, use
     |         | dependency injection instead
 466 | WARNING | \Drupal calls should be avoided in classes, use
     |         | dependency injection instead
 488 | WARNING | \Drupal calls should be avoided in classes, use
     |         | dependency injection instead
 578 | WARNING | t() calls should be avoided in classes, use
     |         | dependency injection and $this->t() instead
 584 | WARNING | t() calls should be avoided in classes, use
     |         | dependency injection and $this->t() instead
 597 | WARNING | t() calls should be avoided in classes, use
     |         | dependency injection and $this->t() instead
 606 | WARNING | t() calls should be avoided in classes, use
     |         | dependency injection and $this->t() instead
 615 | WARNING | t() calls should be avoided in classes, use
     |         | dependency injection and $this->t() instead
 618 | WARNING | t() calls should be avoided in classes, use
     |         | dependency injection and $this->t() instead
 621 | WARNING | t() calls should be avoided in classes, use
     |         | dependency injection and $this->t() instead
 624 | WARNING | t() calls should be avoided in classes, use
     |         | dependency injection and $this->t() instead
----------------------------------------------------------------------


FILE: .../modules/google_analytics_test/google_analytics_test.routing.yml
----------------------------------------------------------------------
FOUND 0 ERRORS AND 1 WARNING AFFECTING 1 LINE
----------------------------------------------------------------------
 7 | WARNING | Open page callback found, please add a comment before
   |         | the line why there is no access restriction
----------------------------------------------------------------------


FILE: ..._analytics_test/src/Controller/GoogleAnalyticsTestController.php
----------------------------------------------------------------------
FOUND 0 ERRORS AND 4 WARNINGS AFFECTING 4 LINES
----------------------------------------------------------------------
 20 | WARNING | Messages are user facing text and must run through
    |         | t() for translation
 21 | WARNING | Messages are user facing text and must run through
    |         | t() for translation
 22 | WARNING | Messages are user facing text and must run through
    |         | t() for translation
 23 | WARNING | Messages are user facing text and must run through
    |         | t() for translation
----------------------------------------------------------------------

Time: 365ms; Memory: 12Mb

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

teeyo created an issue. See original summary.

teeyo’s picture

Title: t() calls should be avoided in classes » t() calls should be avoided in classes and fixing dependency injection (current user and module handler)
Status: Active » Needs review
FileSize
21.62 KB

Status: Needs review » Needs work

The last submitted patch, 2: google_analytics-drupal_practices_t_and_dependecy_injection-2934105-2-8.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

prafullsranjan’s picture

Status: Needs work » Needs review
FileSize
1.19 KB

Here's the correct patch for t();

  • hass committed 0d3fa17 on 8.x-3.x
    Issue #2934105 by hass: Use Drupal::messenger()
    

  • hass committed 47f3bd6 on 8.x-2.x
    Issue #2934105 by hass: Use Drupal::messenger()
    

  • hass committed bf88d93 on 8.x-2.x
    Revert "Issue #2934105 by hass: Use Drupal::messenger()"
    
    This reverts...
hass’s picture

Status: Needs review » Fixed

Status: Fixed » Closed (fixed)

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