Hey,
I checked the Drupal best practices and found the following issues :
phpcs --standard=DrupalPractice --extensions=php,module,inc,install,test,profile,theme,info,txt,md .
FILE: /home/vagrant/Code/d8/modules/contrib/pathauto/pathauto.module
----------------------------------------------------------------------
FOUND 0 ERRORS AND 1 WARNING AFFECTING 1 LINE
----------------------------------------------------------------------
33 | WARNING | Global constants should not be used, move it to a
| | class or interface
----------------------------------------------------------------------
FILE: ...me/vagrant/Code/d8/modules/contrib/pathauto/src/AliasCleaner.php
----------------------------------------------------------------------
FOUND 0 ERRORS AND 32 WARNINGS AFFECTING 32 LINES
----------------------------------------------------------------------
289 | WARNING | t() calls should be avoided in classes, use
| | dependency injection and $this->t() instead
290 | WARNING | t() calls should be avoided in classes, use
| | dependency injection and $this->t() instead
291 | WARNING | t() calls should be avoided in classes, use
| | dependency injection and $this->t() instead
292 | WARNING | t() calls should be avoided in classes, use
| | dependency injection and $this->t() instead
293 | WARNING | t() calls should be avoided in classes, use
| | dependency injection and $this->t() instead
294 | WARNING | t() calls should be avoided in classes, use
| | dependency injection and $this->t() instead
295 | WARNING | t() calls should be avoided in classes, use
| | dependency injection and $this->t() instead
296 | WARNING | t() calls should be avoided in classes, use
| | dependency injection and $this->t() instead
297 | WARNING | t() calls should be avoided in classes, use
| | dependency injection and $this->t() instead
298 | WARNING | t() calls should be avoided in classes, use
| | dependency injection and $this->t() instead
299 | WARNING | t() calls should be avoided in classes, use
| | dependency injection and $this->t() instead
300 | WARNING | t() calls should be avoided in classes, use
| | dependency injection and $this->t() instead
301 | WARNING | t() calls should be avoided in classes, use
| | dependency injection and $this->t() instead
302 | WARNING | t() calls should be avoided in classes, use
| | dependency injection and $this->t() instead
303 | WARNING | t() calls should be avoided in classes, use
| | dependency injection and $this->t() instead
304 | WARNING | t() calls should be avoided in classes, use
| | dependency injection and $this->t() instead
305 | WARNING | t() calls should be avoided in classes, use
| | dependency injection and $this->t() instead
306 | WARNING | t() calls should be avoided in classes, use
| | dependency injection and $this->t() instead
307 | WARNING | t() calls should be avoided in classes, use
| | dependency injection and $this->t() instead
308 | WARNING | t() calls should be avoided in classes, use
| | dependency injection and $this->t() instead
309 | WARNING | t() calls should be avoided in classes, use
| | dependency injection and $this->t() instead
310 | WARNING | t() calls should be avoided in classes, use
| | dependency injection and $this->t() instead
311 | WARNING | t() calls should be avoided in classes, use
| | dependency injection and $this->t() instead
312 | WARNING | t() calls should be avoided in classes, use
| | dependency injection and $this->t() instead
313 | WARNING | t() calls should be avoided in classes, use
| | dependency injection and $this->t() instead
314 | WARNING | t() calls should be avoided in classes, use
| | dependency injection and $this->t() instead
315 | WARNING | t() calls should be avoided in classes, use
| | dependency injection and $this->t() instead
316 | WARNING | t() calls should be avoided in classes, use
| | dependency injection and $this->t() instead
317 | WARNING | t() calls should be avoided in classes, use
| | dependency injection and $this->t() instead
318 | WARNING | t() calls should be avoided in classes, use
| | dependency injection and $this->t() instead
319 | WARNING | t() calls should be avoided in classes, use
| | dependency injection and $this->t() instead
320 | WARNING | t() calls should be avoided in classes, use
| | dependency injection and $this->t() instead
----------------------------------------------------------------------
FILE: ...ode/d8/modules/contrib/pathauto/src/Form/PathautoAdminDelete.php
----------------------------------------------------------------------
FOUND 0 ERRORS AND 3 WARNINGS AFFECTING 3 LINES
----------------------------------------------------------------------
59 | WARNING | \Drupal calls should be avoided in classes, use
| | dependency injection instead
135 | WARNING | \Drupal calls should be avoided in classes, use
| | dependency injection instead
139 | WARNING | \Drupal calls should be avoided in classes, use
| | dependency injection instead
----------------------------------------------------------------------
FILE: ...de/d8/modules/contrib/pathauto/src/Form/PathautoSettingsForm.php
----------------------------------------------------------------------
FOUND 0 ERRORS AND 6 WARNINGS AFFECTING 6 LINES
----------------------------------------------------------------------
91 | 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
133 | WARNING | \Drupal calls should be avoided in classes, use
| | dependency injection instead
160 | WARNING | \Drupal calls should be avoided in classes, use
| | dependency injection instead
209 | WARNING | \Drupal calls should be avoided in classes, use
| | dependency injection instead
244 | WARNING | Unused variable $original_entity_types.
----------------------------------------------------------------------
FILE: ...nt/Code/d8/modules/contrib/pathauto/src/Form/PatternEditForm.php
----------------------------------------------------------------------
FOUND 0 ERRORS AND 2 WARNINGS AFFECTING 2 LINES
----------------------------------------------------------------------
132 | WARNING | Unused variable $condition_id.
249 | WARNING | t() calls should be avoided in classes, use
| | dependency injection and $this->t() instead
----------------------------------------------------------------------
FILE: ...grant/Code/d8/modules/contrib/pathauto/src/PathautoGenerator.php
----------------------------------------------------------------------
FOUND 0 ERRORS AND 3 WARNINGS AFFECTING 3 LINES
----------------------------------------------------------------------
253 | WARNING | \Drupal calls should be avoided in classes, use
| | dependency injection instead
254 | WARNING | \Drupal calls should be avoided in classes, use
| | dependency injection instead
260 | WARNING | \Drupal calls should be avoided in classes, use
| | dependency injection instead
----------------------------------------------------------------------
FILE: ...d8/modules/contrib/pathauto/src/Tests/PathautoMassDeleteTest.php
----------------------------------------------------------------------
FOUND 0 ERRORS AND 1 WARNING AFFECTING 1 LINE
----------------------------------------------------------------------
174 | WARNING | Unused variable $id.
----------------------------------------------------------------------
FILE: ...8/modules/contrib/pathauto/src/Tests/PathautoTaxonomyWebTest.php
----------------------------------------------------------------------
FOUND 0 ERRORS AND 1 WARNING AFFECTING 1 LINE
----------------------------------------------------------------------
57 | WARNING | Unused variable $vocabulary.
----------------------------------------------------------------------
FILE: ...modules/pathauto_string_id_test/pathauto_string_id_test.info.yml
----------------------------------------------------------------------
FOUND 0 ERRORS AND 1 WARNING AFFECTING 1 LINE
----------------------------------------------------------------------
7 | WARNING | All dependencies must be prefixed with the project
| | name, for example "drupal:"
----------------------------------------------------------------------
FILE: ...o/tests/modules/pathauto_views_test/pathauto_views_test.info.yml
----------------------------------------------------------------------
FOUND 0 ERRORS AND 1 WARNING AFFECTING 1 LINE
----------------------------------------------------------------------
8 | WARNING | All dependencies must be prefixed with the project
| | name, for example "drupal:"
----------------------------------------------------------------------
FILE: ...modules/contrib/pathauto/tests/src/Kernel/PathautoKernelTest.php
----------------------------------------------------------------------
FOUND 0 ERRORS AND 2 WARNINGS AFFECTING 2 LINES
----------------------------------------------------------------------
367 | WARNING | There must be no blank line following an inline
| | comment
406 | WARNING | Unused variable $vocab.
----------------------------------------------------------------------
FILE: .../modules/contrib/pathauto/tests/src/Kernel/PathautoTokenTest.php
----------------------------------------------------------------------
FOUND 0 ERRORS AND 1 WARNING AFFECTING 1 LINE
----------------------------------------------------------------------
54 | WARNING | Variable $values is undefined.
----------------------------------------------------------------------
FILE: ...dules/contrib/views_bulk_operations/src/Form/ConfigureAction.php
----------------------------------------------------------------------
FOUND 0 ERRORS AND 1 WARNING AFFECTING 1 LINE
----------------------------------------------------------------------
88 | WARNING | Unused variable $selection.
----------------------------------------------------------------------
Time: 2.5 secs; Memory: 26Mb
I'll try to fixed them and upload a patch for these issues.
Comments
Comment #2
teeyo commentedComment #4
pramodga commentedComment #5
pramodga commentedHello,
The attached file contains the list of Drupal coding standard issues and the patch file contains the issue fixing code for the .module file only
I'm reviewing all the file one by one, and I'm using Drupal coder.
Comment #6
pramodga commentedAttached is the list of issues fixed under the committed patch.
Comment #7
pramodga commentedHello,
This is another patch fixing Drupal Coding Standard Issues for "PathautoMassDeleteTest.php" file.
Attaching error and patch fix file.
Thanks
Comment #8
pramodga commentedComment #9
berdirShort array syntax was fixed elswhere.
Comment #10
swatichouhan012 commentedShort array syntax was fixed in pathhauto, still here is issue of deprecated t function, i have created a patch to fix this kindly review.
Comment #12
swatichouhan012 commentedt() can be removed from test classes. Without conversion of t() to $this->t()
https://www.drupal.org/docs/8/testing/phpunit-in-drupal-8/phpunit-browse...
Comment #14
saphemmy commentedI will work on this.
Comment #15
saphemmy commentedFixed best practices with deprecated code within test:
Example:
Comment #17
saphemmy commentedWill fix the broken tests as there seem to be no deprecated codes anymore.
Comment #18
berdirYou are mixing many different things in a single big patch, that is very hard to review and will most likely not get committed like this as my time is very limited. There are probably already issues for deprecated method changes, if not, it should be split out.
Comment #19
saphemmy commentedAs commented by Berdir: Mixing many different things in the previous patch #2933046-18: Drupal best practices I fixed the scope of the issue and will definitely create a new issue for deprecated codes
Comment #20
victoria-marina commentedComment #21
victoria-marina commentedHi!
I made a patch that complements the previous one. Please review it.
Comment #22
victoria-marina commentedComment #23
vighneshh commentedi will review this
Comment #24
berdirAll the D10 related changes have been committed elsewhere, this will require a big and possibly painful reroll.
Disclaimer: This issue change does not mean that I mean to commit this soon. As commented elsewhere, reviewing huge coding standard change issues is a lot of work and I'd strongly recommend to not work on such issues without discussing such changes with the maintainer *first*, to avoid wasting time (both yours and that of maintainers) :)
Comment #25
rohit.rawat619 commentedi am working in this issue.
Comment #26
rohit.rawat619 commentedI have fix some coding standard please review on this
Comment #28
vitorbs commentedAll the t() function were removed, i think we just need to use $this->t() instead, to avoid any type of bugs.
Comment #29
vitorbs commentedComment #30
viniciuscosta commentedHii, I am work on it =)
Comment #31
viniciuscosta commentedComment #32
WebbehAs a friendly reminder, please post interdiffs between patches if you're conducting meaningful work, so maintainers and other users can understand the changes.
Alternatively, this would be much more succinct to use the fork + MR functionaliy, but I understand that doesn't assign Drupal.org credit upon a commit/MR, so it may not be as attractive.
Comment #34
arti_parmar commentedComment #35
arti_parmar commentedi have created a new patch to fix this-"Drupal best practices", kindly review.
Thanks!
Comment #37
WebbehTo reiterate:
Comment #38
rohit.rawat619 commentedComment #39
rohit.rawat619 commentedComment #40
akshay kashyap commentedThanks for the work @RohitRawat676
Applied Patch cleanly 39
But few error showing when i run
vendor/bin/phpcs --standard=Drupal,DrupalPractice modules/pathautocommand so please fix all these coding standard issues.Comment #41
akshay kashyap commentedComment #42
WebbehPlease read the issue before contributing.
Removing files uploaded in #39 (and other, non-interdiff patches) because, to reiterate again:
As a friendly reminder, please post interdiffs between patches if you're conducting meaningful work, so maintainers and other users can understand the changes.
Frankly, I recommend closing this issue as "Will Not Fix" and handling these outside of an issue, as this issue is largely a drive-by patch effort rather than coordinated efforts.
Comment #44
mably commentedClosing as a duplicate of many other similar issues.