GitLab CI reports PHP_CodeSniffer errors/warnings which should be fixed.

CommentFileSizeAuthor
#33 3255698-33.patch64.94 KBarpitk
#25 diff_3255698_15-25.txt3.21 KBandregp
#25 3255698-25.patch57.05 KBandregp
#15 diff-3255698-14-15.txt185 bytesandregp
#15 3255698-15.patch57.8 KBandregp
#14 diff-3255698-13-14.txt27.8 KBandregp
#14 phpcs_gutenberg_src_new_errors.txt66.1 KBandregp
#14 3255698-14.patch58.1 KBandregp
#13 diff-3255698-11-13.txt561 bytesandregp
#13 3255698-13.patch30.24 KBandregp
#11 3255698-11.patch30.29 KBandregp
#5 interdiff_3_5.txt15.13 KBandregp
#5 3255698_documentation.txt927 bytesandregp
#5 3255698-5.patch35.77 KBandregp
#3 3255698-3.patch21.16 KBandregp
phpcs_gutenberg_src.txt20.4 KBandregp

Issue fork gutenberg-3255698

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:

About issue forks

Comments

andregp created an issue. See original summary.

andregp’s picture

andregp commented

I'm working on this :)

andregp’s picture

andregp commented
StatusFileSize
new 3255698-3.patch21.16 KB

Here is a partial solution (this patch solves all errors/warnings besides the dependency injection ones). I'll talk with the maintainer to see if he's interested on fixing the dependency injection warnings too.

andregp’s picture

andregp commented

Talked with @marcofernandes on slack and he said I can keep with the fixes. So, I'll work on these dependency injection changes.

andregp’s picture

andregp commented
Assigned: andregp » Unassigned
Status: Active » Needs review
StatusFileSize
new 3255698-5.patch35.77 KB
new 3255698_documentation.txt927 bytes
new interdiff_3_5.txt15.13 KB

Finally I did it.
I replaced the code and made the necessary changes to solve all 14 warnings of dependency injection on the files:
src/Controller/SearchController.php
src/Controller/ReusableBlocksController.php
src/Controller/BlocksController.php
src/BlockProcessor/ReusableBlockProcessor.php
src/BlocksRendererHelper.php
src/Service/MediaService.php

I'm sending the patch to solve all issues, an inter-diff between #3 and #5 patches, as well a .txt file with some of the links/documents/patches that I used to learn how to replace and fix the calls that needed dependency injection.

andregp’s picture

andregp commented

Just a tip for whoever will review, to test the changes in each class I used the following paths:

  • src/Controller/SearchController.php -> [my_site]/editor/search
    • If the code is working a list of nodes should be displayed.
  • src/Controller/ReusableBlocksController.php -> [my_site]/editor/reusable-blocks/
    • Create some reusable blocks first. They will be shown here.
  • src/Controller/BlocksController.php -> [my_site]/editor/blocks/load_by_type/{content_type}
    • To be honest I couldn't find any valid content_type so all my searches returned blank, but looking into [my_site]/admin/reports/dblog I was able to check if the code was breaking because of my code or not (when providing a invalid content_type the "correct" error message should be: Warning: Invalid argument supplied for foreach() in Drupal\gutenberg\Controller\BlocksController->loadByType() (line 117 of /app/web/modules/contrib/gutenberg/src/Controller/BlocksController.php) This means the error is not on the previous lines of the function (the ones I actually changed).
  • src/BlocksRendererHelper.php -> [my_site]/admin/structure/views/view/reusable_blocks
    • Create some reusable blocks first. You must be able to see a preview of them in the bottom of this page.
  • src/Service/MediaService.php -> [my_site]/editor/media/dialog
    • If the code is working it will return some raw HTML code.
tmaiochi’s picture

tmaiochi commented
Assigned: Unassigned » tmaiochi
tmaiochi’s picture

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

Hey @andregp, the branch was updated recently, so I couldn't applied the patch. I think you'll need to do some updates in patch.

marcofernandes’s picture

marcofernandes commented

Yep, sorry about that. I had to merge the Gutenberg core library update.

andregp’s picture

andregp commented
Assigned: Unassigned » andregp

No problem, I'll work on a reroll.

andregp’s picture

andregp commented
Assigned: andregp » Unassigned
Status: Needs work » Needs review
StatusFileSize
new 3255698-11.patch30.29 KB

Here is it! :)

tmaiochi’s picture

tmaiochi commented
Assigned: Unassigned » tmaiochi

I'll review this.

andregp’s picture

andregp commented
Assigned: tmaiochi » Unassigned
StatusFileSize
new 3255698-13.patch30.24 KB
new diff-3255698-11-13.txt561 bytes

Sorry, fixing a typo on previous patch.

andregp’s picture

andregp commented
StatusFileSize
new 3255698-14.patch58.1 KB
new phpcs_gutenberg_src_new_errors.txt66.1 KB
new diff-3255698-13-14.txt27.8 KB

So, the patch #13 only solves the old phpcs errors. If we run the phpcs check in the src/ directory again (after applying the patch #13) we get more errors (as you can see on the txt file bellow).
Here is a patch that addresses all the errors (the "old" and "new" ones).

andregp’s picture

andregp commented
StatusFileSize
new 3255698-15.patch57.8 KB
new diff-3255698-14-15.txt185 bytes

I'm sorry I should review my patches more carefully before sending them here...
Another typo fixed (I hope it's the last one).

tmaiochi’s picture

tmaiochi commented
Status: Needs review » Reviewed & tested by the community

Steps performed:
(1) Installed module
(2) Reproduced the issue.
(3) Applied patch.
(4) Code review on changes.
(5) Tested again with patch, issue resolved.
The patch #15 resolve all phpcs messages, and the module still work well.

  • marcofernandes committed bf4410f on 8.x-2.x 
    Issue #3255698 by andregp, tmaiochi, marcofernandes: Fix PHPCS errors on...
marcofernandes’s picture

marcofernandes commented
Status: Reviewed & tested by the community » Fixed

Thanks andregp and tmaiochi.
Valeu, cara. 😇

marcofernandes’s picture

marcofernandes commented
Assigned: Unassigned » marcofernandes
Priority: Normal » Critical
Status: Fixed » Needs work

I missed this on my review:

-    if (in_array($media->getSource()->getPluginId(), [
+    if (i->getSource()->getPluginId(), [
       'file', 'image', 'video', 'audio',
-    ])) {
+    ]) {
+      )

And it's breaking the media browser.

marcofernandes’s picture

marcofernandes commented

Also, it seems, since we don't have a dependency with Media and Media Library, dependency injection for MediaLibraryUiBuilder will fail if those modules are disabled.

  • marcofernandes committed 3787812 on 8.x-2.x 
    Revert "Issue #3255698 by andregp, tmaiochi, marcofernandes: Fix PHPCS...
marcofernandes’s picture

marcofernandes commented
Assigned: marcofernandes » Unassigned
marcofernandes’s picture

marcofernandes commented
Priority: Critical » Normal
andregp’s picture

andregp commented
Assigned: Unassigned » andregp

@marcofernandes oh, sorry. I'll fix it...

andregp’s picture

andregp commented
Assigned: andregp » Unassigned
Status: Needs work » Needs review
StatusFileSize
new 3255698-25.patch57.05 KB
new diff_3255698_15-25.txt3.21 KB

Here is the new patch. I fixed the error on if (in_array($media->getSource()->getPluginId(), [. And reverted the dependency injection for MediaLibraryUiBuilder.
I also added a @codingStandardsIgnoreLine and a comment with this issue's link so people know why the dependency was left there.

There is also a new case of Drupal calls (which is accused by the phpcs) on src/Controller/SearchController.php, line 57 (84 after this patch), but I left it there as it's from a recent commit and also seems temporary (there is already a @todo tag to improve the code).

tmaiochi’s picture

tmaiochi commented
Assigned: Unassigned » tmaiochi

I'll do this review!

tmaiochi’s picture

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

Steps performed:
(1) Installed module
(2) Reproduced the issue.
(3) Applied patch.
(4) Code review on changes.
(5) Tested again with patch, issue resolved.
The patch fixed all PHPCS messages in src, except by a Drupal calls, but how was said in comment #25 seems temporary. And fixed the error on if (in_array($media->getSource()->getPluginId(), [.. The module is working fine as far as I can test.

avpaderno’s picture

avpaderno
he/him
Primary language Italian
Location Brescia, 🇮🇹 🇪🇺
commented
Status: Reviewed & tested by the community » Needs work
Issue tags: +Coding standards, +Needs issue summary update

The issue summary should always describe what the issue is trying to fix and, in the case, of coding standards issues, report which command has been used, which arguments have been used, and which report that command shown.

avpaderno’s picture

avpaderno
he/him
Primary language Italian
Location Brescia, 🇮🇹 🇪🇺
commented
Title: PHPCS errors on src/ directory » Fix the issues reported by phpcs
indrapatil’s picture

indrapatil commented
Issue summary: View changes
indrapatil’s picture

indrapatil commented
Status: Needs work » Needs review
avpaderno’s picture

avpaderno
he/him
Primary language Italian
Location Brescia, 🇮🇹 🇪🇺
commented
Issue tags: -Needs issue summary update
arpitk’s picture

arpitk commented
StatusFileSize
new 3255698-33.patch64.94 KB

Worked on fixing remaining issue after patch #25. fixed Drupal calls. Please review.

Thanks!

yashaswi18’s picture

yashaswi18 commented

Tried applying the patch provided in #33, got the following error :

git apply 3255698-33.patch -v
Checking patch gutenberg.services.yml...
Checking patch src/Ajax/UpdateMediaEntitiesCommand.php...
Checking patch src/BlockProcessor/DuotoneProcessor.php...
Checking patch src/BlockProcessor/OEmbedProcessor.php...
Checking patch src/BlockProcessor/ReusableBlockProcessor.php...
Checking patch src/BlocksRendererHelper.php...
Checking patch src/Controller/BlocksController.php...
Checking patch src/Controller/MediaController.php...
Checking patch src/Controller/ReusableBlocksController.php...
Checking patch src/Controller/SearchController.php...
error: while searching for:
      return new JsonResponse([]);
    }

    $query = \Drupal::entityQuery('node');
    $query->condition('title', $search, 'CONTAINS')
      ->condition('status', 1)
      ->sort('created', 'DESC')

error: patch failed: src/Controller/SearchController.php:35
error: src/Controller/SearchController.php: patch does not apply
Checking patch src/Controller/UtilsController.php...
Checking patch src/DataProvider/BaseDataProvider.php...
Checking patch src/DataProvider/FileEntityDataProvider.php...
Checking patch src/DataProvider/MediaEntityDataProvider.php...
Checking patch src/Discovery/BlockJsonDiscovery.php...
Checking patch src/Form/BlockSettingsForm.php...
Checking patch src/GutenbergElementInfoAlter.php...
Checking patch src/GutenbergLibraryManager.php...
Hunk #3 succeeded at 254 (offset 7 lines).
Checking patch src/GutenbergMediaLibraryUiBuilder.php...
Checking patch src/MappingFieldsHelper.php...
Checking patch src/MediaSelectionProcessor/MediaSelectionProcessorInterface.php...
Checking patch src/OEmbedResolver.php...
Checking patch src/Parser/BlockParser.php...
Checking patch src/Parser/BlockParserFrame.php...
Checking patch src/Plugin/Filter/GutenbergFilter.php...
Checking patch src/Plugin/GutenbergPlugin/DrupalImage.php...
Checking patch src/ScanDir.php...
Checking patch src/Service/MediaService.php...
Hunk #8 succeeded at 476 (offset 2 lines).
Checking patch src/TinyColor.php...
nikolay shapovalov’s picture

nikolay shapovalov commented
Issue summary: View changes

I think no changes to vendor folder needs to be made
Please use command and ignore vendor folder.

phpcs --standard=Drupal,DrupalPractice --extensions=php,inc,module,install,info,test,profile,theme modules/contrib/gutenberg/ --ignore=*/vendor/*

I update IS.

nikolay shapovalov’s picture

nikolay shapovalov commented
Status: Needs review » Needs work
Issue tags: +Needs reroll

I suggest to move to MR workflow to be able to run Gitlab CI phpcs check.
And the status of this task will be transparent.

Set to NW, because patch #33 is not applicable.

pray_12 made their first commit to this issue’s fork.

jeremy1606 opened merge request !226

jeremy1606 made their first commit to this issue’s fork.

avpaderno opened merge request !227

avpaderno’s picture

avpaderno
he/him
Primary language Italian
Location Brescia, 🇮🇹 🇪🇺
commented
Title: Fix the issues reported by phpcs » Fix the issues reported by PHP_CodeSniffer
Issue summary: View changes
Issue tags: -Needs reroll

avpaderno changed the visibility of the branch 3255698-gitlab-ci-reports to hidden.

avpaderno’s picture

avpaderno
he/him
Primary language Italian
Location Brescia, 🇮🇹 🇪🇺
commented

The issue summary has been updated to link to the GitLab CI report for PHP_CodeSniffer. It lists all the PHP_CodeSniffer errors/warnings that should be fixed.

avpaderno’s picture

avpaderno
he/him
Primary language Italian
Location Brescia, 🇮🇹 🇪🇺
commented
Issue tags: +Needs issue summary update

Furthermore, looking at #3255691: [META] PHPCS errors on code, which is linked as parent issue, it seems it has been decided to fix the PHP_CodeSniffer errors/warnings in batches, while this issue seems to fix all the the PHP_CodeSniffer errors/warnings at once.
I guess the issue summary needs to be updated.

I would rather fix all the PHP_CodeSniffer errors/warnings in a single issue, since that would mean check the GitLab CI report only once, instead of reading it multiple times to just fix a sub-set of all the reported errors/warnings.