Problem/Motivation

Found this issues reported by phpcs:

FILE: /Users/studenttrainees/Drupal.org/drupalorg-site/docroot/modules/contrib/pdf/pdf.module
---------------------------------------------------------------------------------------------
FOUND 11 ERRORS AND 1 WARNING AFFECTING 8 LINES
---------------------------------------------------------------------------------------------
  1 | ERROR   | [x] Missing file doc comment
  7 | ERROR   | [x] Spaces must be used to indent lines; tabs are not allowed
  7 | ERROR   | [x] Line indented incorrectly; expected 2 spaces, found 1
  8 | ERROR   | [x] Spaces must be used to indent lines; tabs are not allowed
  8 | ERROR   | [x] Array indentation error, expected 3 spaces but found 2
  9 | ERROR   | [x] Spaces must be used to indent lines; tabs are not allowed
  9 | ERROR   | [x] Array indentation error, expected 4 spaces but found 3
 10 | ERROR   | [x] Spaces must be used to indent lines; tabs are not allowed
 10 | WARNING | [x] A comma should follow the last multiline array item. Found: NULL
 11 | ERROR   | [x] Spaces must be used to indent lines; tabs are not allowed
 12 | ERROR   | [x] Spaces must be used to indent lines; tabs are not allowed
 13 | ERROR   | [x] Spaces must be used to indent lines; tabs are not allowed
---------------------------------------------------------------------------------------------
PHPCBF CAN FIX THE 12 MARKED SNIFF VIOLATIONS AUTOMATICALLY
---------------------------------------------------------------------------------------------


FILE: /Users/studenttrainees/Drupal.org/drupalorg-site/docroot/modules/contrib/pdf/css/pdf.css
----------------------------------------------------------------------------------------------
FOUND 3 ERRORS AFFECTING 3 LINES
----------------------------------------------------------------------------------------------
  1 | ERROR | [x] End of line character is invalid; expected "\n" but found "\r\n"
  2 | ERROR | [x] Expected 1 space after colon in style definition; 0 found
 33 | ERROR | [x] Blank lines are not allowed in class definitions
----------------------------------------------------------------------------------------------
PHPCBF CAN FIX THE 3 MARKED SNIFF VIOLATIONS AUTOMATICALLY
----------------------------------------------------------------------------------------------


FILE: /Users/studenttrainees/Drupal.org/drupalorg-site/docroot/modules/contrib/pdf/pdf.routing.yml
---------------------------------------------------------------------------------------------------
FOUND 0 ERRORS AND 1 WARNING AFFECTING 1 LINE
---------------------------------------------------------------------------------------------------
 7 | WARNING | The administration page callback should probably use "administer site
   |         | configuration" - which implies the user can change something - rather than "access
   |         | administration pages" which is about viewing but not changing configurations.
---------------------------------------------------------------------------------------------------


FILE: ...studenttrainees/Drupal.org/drupalorg-site/docroot/modules/contrib/pdf/src/Form/ConfigForm.php
---------------------------------------------------------------------------------------------------
FOUND 0 ERRORS AND 1 WARNING AFFECTING 1 LINE
---------------------------------------------------------------------------------------------------
 10 | WARNING | The class short comment should describe what the class does and not simply repeat
    |         | the class name
---------------------------------------------------------------------------------------------------


FILE: ...l.org/drupalorg-site/docroot/modules/contrib/pdf/src/Plugin/Field/FieldFormatter/PdfPages.php
---------------------------------------------------------------------------------------------------
FOUND 8 ERRORS AND 3 WARNINGS AFFECTING 11 LINES
---------------------------------------------------------------------------------------------------
  8 | WARNING | [x] Unused use statement
 10 | ERROR   | [ ] Missing short description in doc comment
 20 | ERROR   | [x] Missing function doc comment
 26 | ERROR   | [x] Missing function doc comment
 29 | WARNING | [ ] t() calls should be avoided in classes, use
    |         |     \Drupal\Core\StringTranslation\StringTranslationTrait and $this->t() instead
 35 | ERROR   | [x] Missing function doc comment
 42 | WARNING | [ ] t() calls should be avoided in classes, use
    |         |     \Drupal\Core\StringTranslation\StringTranslationTrait and $this->t() instead
 48 | ERROR   | [x] Missing function doc comment
 57 | ERROR   | [x] No space found before comment text; expected "// '#value' => TODO," but found
    |         |     "//'#value' => TODO,"
 82 | ERROR   | [x] Expected 1 blank line after function; 0 found
 83 | ERROR   | [x] The closing brace for the class must have an empty line before it
---------------------------------------------------------------------------------------------------
PHPCBF CAN FIX THE 8 MARKED SNIFF VIOLATIONS AUTOMATICALLY
---------------------------------------------------------------------------------------------------


FILE: ...g/drupalorg-site/docroot/modules/contrib/pdf/src/Plugin/Field/FieldFormatter/PdfThumbnail.php
---------------------------------------------------------------------------------------------------
FOUND 9 ERRORS AND 6 WARNINGS AFFECTING 14 LINES
---------------------------------------------------------------------------------------------------
   8 | WARNING | [x] Unused use statement
  10 | ERROR   | [ ] Missing short description in doc comment
  19 | ERROR   | [x] Missing function doc comment
  19 | ERROR   | [x] Expected 1 blank line before function; 0 found
  27 | ERROR   | [x] Missing function doc comment
  31 | WARNING | [ ] t() calls should be avoided in classes, use
     |         |     \Drupal\Core\StringTranslation\StringTranslationTrait and $this->t() instead
  38 | WARNING | [ ] t() calls should be avoided in classes, use
     |         |     \Drupal\Core\StringTranslation\StringTranslationTrait and $this->t() instead
  44 | WARNING | [ ] t() calls should be avoided in classes, use
     |         |     \Drupal\Core\StringTranslation\StringTranslationTrait and $this->t() instead
  49 | ERROR   | [x] Missing function doc comment
  59 | WARNING | [ ] t() calls should be avoided in classes, use
     |         |     \Drupal\Core\StringTranslation\StringTranslationTrait and $this->t() instead
  62 | WARNING | [x] A comma should follow the last multiline array item. Found: $height
  68 | ERROR   | [x] Missing function doc comment
  76 | ERROR   | [x] No space found before comment text; expected "// '#value' => ," but found
     |         |     "//'#value' => ,"
 103 | ERROR   | [x] Expected 1 blank line after function; 0 found
 104 | ERROR   | [x] The closing brace for the class must have an empty line before it
---------------------------------------------------------------------------------------------------
PHPCBF CAN FIX THE 10 MARKED SNIFF VIOLATIONS AUTOMATICALLY
---------------------------------------------------------------------------------------------------


FILE: ...org/drupalorg-site/docroot/modules/contrib/pdf/src/Plugin/Field/FieldFormatter/PdfDefault.php
---------------------------------------------------------------------------------------------------
FOUND 1 ERROR AND 7 WARNINGS AFFECTING 4 LINES
---------------------------------------------------------------------------------------------------
  10 | ERROR   | Missing short description in doc comment
  46 | WARNING | t() calls should be avoided in classes, use
     |         | \Drupal\Core\StringTranslation\StringTranslationTrait and $this->t() instead
 139 | WARNING | t() calls should be avoided in classes, use
     |         | \Drupal\Core\StringTranslation\StringTranslationTrait and $this->t() instead
 139 | WARNING | t() calls should be avoided in classes, use
     |         | \Drupal\Core\StringTranslation\StringTranslationTrait and $this->t() instead
 139 | WARNING | t() calls should be avoided in classes, use
     |         | \Drupal\Core\StringTranslation\StringTranslationTrait and $this->t() instead
 139 | WARNING | Do not concatenate strings to translatable strings, they should be part of the
     |         | t() argument and you should use placeholders
 139 | WARNING | t() calls should be avoided in classes, use
     |         | \Drupal\Core\StringTranslation\StringTranslationTrait and $this->t() instead
 153 | WARNING | \Drupal calls should be avoided in classes, use dependency injection instead
---------------------------------------------------------------------------------------------------

Time: 277ms; Memory: 10MB

Steps to reproduce

Run this command in the module folder: phpcs --standard=Drupal,DrupalPractice --extensions=php,module,inc,install,test,profile,theme,css,info,txt,md,yml

Proposed resolution

Remaining tasks

FILE: ...org/drupalorg-site/docroot/modules/contrib/pdf/src/Plugin/Field/FieldFormatter/PdfDefault.php
---------------------------------------------------------------------------------------------------
FOUND 0 ERRORS AND 1 WARNING AFFECTING 1 LINE
---------------------------------------------------------------------------------------------------
156 | WARNING | \Drupal calls should be avoided in classes, use dependency injection instead
---------------------------------------------------------------------------------------------------

Time: 214ms; Memory: 10MB

Issue fork pdf-3364326

Command icon Show commands

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

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

    Comments

    paraderojether created an issue. See original summary.

    paraderojether’s picture

    Assigned: paraderojether » Unassigned
    Status: Active » Needs review
    StatusFileSize
    new10.25 KB

    Created a patch to fix the issues reported by phpcs except for the dependency injection.

    Please review.
    Thank You.

    arpitk’s picture

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

    working on remaining issues.

    arpitk’s picture

    Assigned: arpitk » Unassigned
    Status: Needs work » Needs review
    StatusFileSize
    new12.85 KB

    Here is the updated patch. Please review.

    Thanks!

    anjali mehta’s picture

    StatusFileSize
    new178.29 KB
    new101.05 KB

    Hello @arpitk
    Tested #3364326-4 patch on Drupal 9.5.9 and php version 8.1. Patch applied cleanly. But getting a warning and error on running the phpcs. Adding screenshot for Your references. Please have a look.
    Thank You for your support.

    pray_12’s picture

    StatusFileSize
    new19.17 KB

    Hi,
    Updated the patch. Please review.
    Thank you!!

    yashaswi18’s picture

    Applied the patch provided in #6.
    The patch is applied successfully and ran the command phpcs --standard=Drupal --extensions=php,module,inc,install,test,profile,theme,css,info,txt,md,yml pdf/ again, no errors and warnings found.

    Checking patch css/pdf.css...
    Checking patch pdf.module...
    Checking patch src/Form/ConfigForm.php...
    Checking patch src/Plugin/Field/FieldFormatter/PdfDefault.php...
    Checking patch src/Plugin/Field/FieldFormatter/PdfPages.php...
    Checking patch src/Plugin/Field/FieldFormatter/PdfThumbnail.php...
    Applied patch css/pdf.css cleanly.
    Applied patch pdf.module cleanly.
    Applied patch src/Form/ConfigForm.php cleanly.
    Applied patch src/Plugin/Field/FieldFormatter/PdfDefault.php cleanly.
    Applied patch src/Plugin/Field/FieldFormatter/PdfPages.php cleanly.
    Applied patch src/Plugin/Field/FieldFormatter/PdfThumbnail.php cleanly.
    silvi.addweb’s picture

    Status: Needs review » Reviewed & tested by the community

    I have tested this patch and it's working fine.

    m.abdulqader’s picture

    • 7690eb68 committed on 8.x-1.x
      Issue #3364326 by paraderojether, arpitk, pray_12, anjali mehta,...
    m.abdulqader’s picture

    Status: Reviewed & tested by the community » Fixed
    aimevp’s picture

    Status: Fixed » Needs review
    StatusFileSize
    new21.75 KB

    While working on implementing a patch from another issue I was surprised to see that only a small portion of the patch provided in #6 got merged to dev. Since I wanted to incorporate these changes on an active project I started working on a re-roll of that patch and then noticed the dependency injection fixes were done incorrectly (or maybe they were for D9 but are no longer for D10). So I combined all these changes + some code style improvements into a new patch which works and has been tested on a Drupal 10.3.6 install. However I'm not sure how this patch would behave on a Drupal 9.x install or if this requires a version bump of the pdf module then?

    • b4d50d7a committed on 8.x-1.x
      #3364326 Fix the issues reported by phpcs
      
    m.abdulqader’s picture

    Status: Needs review » Fixed

    Thank you @aimevp I merged the patch.

    Now that this issue is closed, review the contribution record.

    As a contributor, attribute any organization that helped you, or if you volunteered your own time.

    Maintainers, credit people who helped resolve this issue.

    Status: Fixed » Closed (fixed)

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

    adam-vessey’s picture

    It does not appear that the PHPCS fixes here have made it into a tagged release: https://git.drupalcode.org/project/pdf/-/commit/b4d50d7a2e1198615b85808d...