- Issue created by @ptmkenny
- @ptmkenny opened merge request.
- Status changed to Needs review
almost 2 years ago 9:25am 6 February 2023 - ๐ฏ๐ตJapan ptmkenny
This MR applies the automated fixes and I also manually fixed the low-hanging fruit (breaking up arrays onto multiple lines, removing unused variables, etc.)
Here is a scan after applying the MR showing the remaining errors:
FILE: /var/www/html/web/modules/contrib/jsonapi_extras/modules/jsonapi_defaults/src/Controller/EntityResource.php ----------------------------------------------------------------------------------------------------------------- FOUND 0 ERRORS AND 1 WARNING AFFECTING 1 LINE ----------------------------------------------------------------------------------------------------------------- 191 | WARNING | Redeclaration of function parameter $arr as variable. ----------------------------------------------------------------------------------------------------------------- FILE: /var/www/html/web/modules/contrib/jsonapi_extras/tests/src/Functional/JsonApiExtrasFunctionalTest.php ----------------------------------------------------------------------------------------------------------- FOUND 3 ERRORS AFFECTING 3 LINES ----------------------------------------------------------------------------------------------------------- 145 | ERROR | Doc comment is empty 162 | ERROR | Doc comment is empty 195 | ERROR | The @see reference should not contain any additional text ----------------------------------------------------------------------------------------------------------- FILE: /var/www/html/web/modules/contrib/jsonapi_extras/tests/src/Kernel/EntityToJsonApiTest.php ----------------------------------------------------------------------------------------------- FOUND 4 ERRORS AFFECTING 4 LINES ----------------------------------------------------------------------------------------------- 55 | ERROR | Missing short description in doc comment 60 | ERROR | Missing short description in doc comment 65 | ERROR | Missing short description in doc comment 105 | ERROR | Missing short description in doc comment ----------------------------------------------------------------------------------------------- FILE: /var/www/html/web/modules/contrib/jsonapi_extras/src/EventSubscriber/ConfigSubscriber.php ----------------------------------------------------------------------------------------------- FOUND 0 ERRORS AND 1 WARNING AFFECTING 1 LINE ----------------------------------------------------------------------------------------------- 53 | WARNING | \Drupal calls should be avoided in classes, use dependency injection instead ----------------------------------------------------------------------------------------------- FILE: /var/www/html/web/modules/contrib/jsonapi_extras/src/EventSubscriber/JsonApiBuildSubscriber.php ------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- FOUND 1 ERROR AND 3 WARNINGS AFFECTING 3 LINES ------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- 19 | ERROR | Missing short description in doc comment 43 | WARNING | The deprecation-version 'jsonapi_extras:8.x-3.x' does not match the lower-case machine-name standard: drupal:n.n.n or project:n.x-n.n or project:n.x-n.n-label[n] or | | project:n.n.n or project:n.n.n-label[n] 43 | WARNING | The removal-version 'jsonapi_extras:8.x-4.x' does not match the lower-case machine-name standard: drupal:n.n.n or project:n.x-n.n or project:n.x-n.n-label[n] or | | project:n.n.n or project:n.n.n-label[n] 44 | WARNING | \Drupal calls should be avoided in classes, use dependency injection instead ------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- FILE: /var/www/html/web/modules/contrib/jsonapi_extras/src/Form/JsonapiResourceConfigForm.php ------------------------------------------------------------------------------------------------------------------------------------------ FOUND 0 ERRORS AND 1 WARNING AFFECTING 1 LINE ------------------------------------------------------------------------------------------------------------------------------------------ 417 | WARNING | t() calls should be avoided in classes, use \Drupal\Core\StringTranslation\StringTranslationTrait and $this->t() instead ------------------------------------------------------------------------------------------------------------------------------------------ FILE: /var/www/html/web/modules/contrib/jsonapi_extras/src/JsonapiResourceConfigListBuilder.php ------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- FOUND 0 ERRORS AND 4 WARNINGS AFFECTING 3 LINES ------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- 62 | WARNING | The deprecation-version 'jsonapi_extras:8.x-3.x' does not match the lower-case machine-name standard: drupal:n.n.n or project:n.x-n.n or project:n.x-n.n-label[n] | | or project:n.n.n or project:n.n.n-label[n] 62 | WARNING | The removal-version 'jsonapi_extras:8.x-4.x' does not match the lower-case machine-name standard: drupal:n.n.n or project:n.x-n.n or project:n.x-n.n-label[n] or | | project:n.n.n or project:n.n.n-label[n] 99 | WARNING | t() calls should be avoided in classes, use \Drupal\Core\StringTranslation\StringTranslationTrait and $this->t() instead 100 | WARNING | t() calls should be avoided in classes, use \Drupal\Core\StringTranslation\StringTranslationTrait and $this->t() instead ------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- Time: 641ms; Memory: 14MB
I do not feel confident fixing the remaining errors so I am submitting the MR in this state.
- Status changed to Needs work
almost 2 years ago 7:19am 9 February 2023 - ๐ฎ๐ณIndia rassoni Bangalore
Rashmisoni โ made their first commit to this issueโs fork.
- @rashmisoni opened merge request.
- Status changed to Needs review
almost 2 years ago 12:17pm 14 February 2023 - Assigned to indrapatil
- Status changed to Needs work
almost 2 years ago 9:27am 17 February 2023 - ๐ฎ๐ณIndia indrapatil Bangalore
I reviewed the patch, still i can see errors
phpcs --standard=Drupal,DrupalPractice --extensions=php,module,inc,install,test,profile,theme,css,info,txt,md,yml jsonapi_extras-3339527/
PHP Warning: Module "gd" is already loaded in Unknown on line 0FILE: /var/www/html/drupal8/web/modules/jsonapi_extras-3339527/src/EventSubscriber/JsonApiBuildSubscriber.php
-------------------------------------------------------------------------------------------------------------
FOUND 0 ERRORS AND 1 WARNING AFFECTING 1 LINE
-------------------------------------------------------------------------------------------------------------
46 | WARNING | \Drupal calls should be avoided in classes, use dependency injection instead
-------------------------------------------------------------------------------------------------------------FILE: /var/www/html/drupal8/web/modules/jsonapi_extras-3339527/src/EventSubscriber/ConfigSubscriber.php
-------------------------------------------------------------------------------------------------------
FOUND 0 ERRORS AND 1 WARNING AFFECTING 1 LINE
-------------------------------------------------------------------------------------------------------
53 | WARNING | \Drupal calls should be avoided in classes, use dependency injection instead
-------------------------------------------------------------------------------------------------------FILE: /var/www/html/drupal8/web/modules/jsonapi_extras-3339527/src/Form/JsonapiResourceConfigForm.php
------------------------------------------------------------------------------------------------------------------------------------------
FOUND 0 ERRORS AND 1 WARNING AFFECTING 1 LINE
------------------------------------------------------------------------------------------------------------------------------------------
417 | WARNING | t() calls should be avoided in classes, use \Drupal\Core\StringTranslation\StringTranslationTrait and $this->t() instead
------------------------------------------------------------------------------------------------------------------------------------------FILE: /var/www/html/drupal8/web/modules/jsonapi_extras-3339527/src/JsonapiResourceConfigListBuilder.php
------------------------------------------------------------------------------------------------------------------------------------------
FOUND 0 ERRORS AND 2 WARNINGS AFFECTING 2 LINES
------------------------------------------------------------------------------------------------------------------------------------------
99 | WARNING | t() calls should be avoided in classes, use \Drupal\Core\StringTranslation\StringTranslationTrait and $this->t() instead
100 | WARNING | t() calls should be avoided in classes, use \Drupal\Core\StringTranslation\StringTranslationTrait and $this->t() instead
------------------------------------------------------------------------------------------------------------------------------------------FILE: /var/www/html/drupal8/web/modules/jsonapi_extras-3339527/modules/jsonapi_defaults/src/Controller/EntityResource.php
-------------------------------------------------------------------------------------------------------------------------
FOUND 0 ERRORS AND 1 WARNING AFFECTING 1 LINE
-------------------------------------------------------------------------------------------------------------------------
191 | WARNING | Redeclaration of function parameter $arr as variable.
-------------------------------------------------------------------------------------------------------------------------Time: 1.89 secs; Memory: 18MB
I will work on it.
- ๐ฏ๐ตJapan ptmkenny
@Indra patil The patch does not need to fix all coding standards errors; if the patch fixes errors in a valid way, I think the patch can pass review with a note that "this does not fix all errors."
In particular, changing code to use dependency injection in a coding standards patch increases the chance that something might get broken, so I think it is better to handle DI in a separate patch in a separate issue.
- Status changed to Needs review
almost 2 years ago 10:54am 17 February 2023 - ๐ฎ๐ณIndia indrapatil Bangalore
@ptmkenny thank you
I totally agree I fixed some of the issues and still, this below error is pending this Need to create a new ticket.
phpcs --standard=Drupal,DrupalPractice --extensions=php,module,inc,install,test,profile,theme,css,info,txt,md,yml jsonapi_extras-3339527/FILE: /var/www/html/drupal8/web/modules/jsonapi_extras-3339527/src/EventSubscriber/ConfigSubscriber.php
-------------------------------------------------------------------------------------------------------
FOUND 0 ERRORS AND 1 WARNING AFFECTING 1 LINE
-------------------------------------------------------------------------------------------------------
53 | WARNING | \Drupal calls should be avoided in classes, use dependency injection instead
-------------------------------------------------------------------------------------------------------Time: 620ms; Memory: 18MB
- Issue was unassigned.
- Assigned to RohitRawat676
- Status changed to Active
over 1 year ago 7:37am 27 June 2023 - Issue was unassigned.
- Status changed to Needs review
over 1 year ago 7:39am 27 June 2023 - Open on Drupal.org โCore: 10.1.x + Environment: PHP 8.1 & MySQL 5.7last update
over 1 year ago Waiting for branch to pass - last update
over 1 year ago Composer require-dev failure - last update
about 1 year ago 17 pass - ๐ณ๐ฑNetherlands bbrala Netherlands
Thanks! I've updated a few more parts of the patch to make it complete. Lets see if ci doesnt fail.
-
bbrala โ
committed e9222988 on 8.x-3.x
Issue #3339527 by Rassoni, bbrala, ptmkenny, Indrapatil, RohitRawat676,...
-
bbrala โ
committed e9222988 on 8.x-3.x
- Status changed to Fixed
about 1 year ago 1:49pm 31 August 2023 Automatically closed - issue fixed for 2 weeks with no activity.