Running phpcs --standard=Drupal,DrupalPractice --extensions=php,module,inc,install,test,profile,theme,css,info,txt,md,yml,twig shows the following warnings/errors that needs to be fixed.

FILE: ./entity_browser_8.x-2.x/tests/fixtures/update/entity_browser.update-hook-test.php
  4 | ERROR | [x] Doc comment short description must be on the first line
 85 | ERROR | [ ] unserialize() is insecure unless allowed classes are limited. Use a safe format like JSON or use the
    |       |     allowed_classes option.

FILE: ./entity_browser_8.x-2.x/tests/modules/entity_browser_test/entity_browser_test.routing.yml
 7 | WARNING | Open page callback found, please add a comment before the line why there is no access restriction

FILE: ./entity_browser_8.x-2.x/tests/modules/entity_browser_test/src/Form/FormElementTest.php
 33 | WARNING | \Drupal calls should be avoided in classes, use dependency injection instead
 37 | WARNING | \Drupal calls should be avoided in classes, use dependency injection instead

FILE: ./entity_browser_8.x-2.x/tests/modules/entity_browser_test/src/Plugin/EntityBrowser/Widget/DummyWidget.php
 63 | WARNING | \Drupal calls should be avoided in classes, use dependency injection instead

FILE: ./entity_browser_8.x-2.x/tests/src/Kernel/Extension/EntityBrowserTest.php
 266 | WARNING | t() calls should be avoided in classes, use \Drupal\Core\StringTranslation\StringTranslationTrait and $this->t()
     |         | instead
 461 | WARNING | t() calls should be avoided in classes, use \Drupal\Core\StringTranslation\StringTranslationTrait and $this->t()
     |         | instead
 468 | WARNING | t() calls should be avoided in classes, use \Drupal\Core\StringTranslation\StringTranslationTrait and $this->t()
     |         | instead

FILE: ./entity_browser_8.x-2.x/tests/src/Kernel/Plugin/FieldWidgetDisplayTest.php
 70 | ERROR | The array declaration extends to column 97 (the limit is 80). The array content should be split up over multiple
    |       | lines

FILE: ./entity_browser_8.x-2.x/tests/src/FunctionalJavascript/ConfigurationTest.php
 202 | WARNING | Unused variable $entity_type.

FILE: ./entity_browser_8.x-2.x/tests/src/FunctionalJavascript/InlineEntityFormTest.php
 424 | ERROR | [x] list(...) is forbidden, use [...] instead.

FILE: ./entity_browser_8.x-2.x/tests/src/FunctionalJavascript/FieldWidgetConfigTest.php
 153 | WARNING | Line exceeds 80 characters; contains 104 characters

FILE: ./entity_browser_8.x-2.x/tests/src/FunctionalJavascript/EntityReferenceWidgetTest.php
 590 | ERROR | [x] list(...) is forbidden, use [...] instead.

FILE: ./entity_browser_8.x-2.x/tests/src/FunctionalJavascript/ParagraphsTest.php
 50 | ERROR | The array declaration extends to column 84 (the limit is 80). The array content should be split up over multiple
    |       | lines
 51 | ERROR | The array declaration extends to column 98 (the limit is 80). The array content should be split up over multiple
    |       | lines

FILE: ./entity_browser_8.x-2.x/tests/src/FunctionalJavascript/EntityBrowserTest.php
  44 | WARNING | [x] '@TODO Test the edit button.' should match the format '@todo Fix problem X here.'
 142 | WARNING | [ ] Unused variable $image2.
 186 | WARNING | [ ] Line exceeds 80 characters; contains 96 characters

FILE: ./entity_browser_8.x-2.x/tests/src/FunctionalJavascript/CardinalityTest.php
 230 | ERROR   | The array declaration extends to column 84 (the limit is 80). The array content should be split up over multiple
     |         | lines
 238 | WARNING | Line exceeds 80 characters; contains 82 characters
 306 | ERROR   | The array declaration extends to column 84 (the limit is 80). The array content should be split up over multiple
     |         | lines
 315 | WARNING | Line exceeds 80 characters; contains 82 characters

FILE: ./entity_browser_8.x-2.x/tests/src/FunctionalJavascript/EntityBrowserWebDriverTestBase.php
 254 | WARNING | Line exceeds 80 characters; contains 83 characters
 288 | WARNING | Line exceeds 80 characters; contains 86 characters

FILE: ./entity_browser_8.x-2.x/tests/src/FunctionalJavascript/PluginsTest.php
 228 | WARNING | [x] 'TODO test if entities were selected. Will most likely need a custom event' should match the format '@todo Fix
     |         |     problem X here.'

FILE: ./entity_browser_8.x-2.x/tests/src/Functional/EntityBrowserUITest.php
 37 | ERROR | Public method name "EntityBrowserUITest::testEntityBrowserUI" is not in lowerCamel format

FILE: ./entity_browser_8.x-2.x/tests/src/Functional/FormElementTest.php
 73 | ERROR | The array declaration extends to column 151 (the limit is 80). The array content should be split up over multiple
    |       | lines
 73 | ERROR | The array declaration extends to column 150 (the limit is 80). The array content should be split up over multiple
    |       | lines

FILE: ./entity_browser_8.x-2.x/entity_browser.install
 70 | WARNING | Unused variable $entity_type_name.
 83 | WARNING | Unused variable $display_name.
 85 | WARNING | Unused variable $field_name.

FILE: ./entity_browser_8.x-2.x/entity_browser.views.inc
 14 | WARNING | Unused variable $entity_type_name.

FILE: ./entity_browser_8.x-2.x/modules/entity_form/entity_browser_entity_form.module
  32 | WARNING | [x] 'TODO - 'default' might become configurable or something else in the future.' should match the format '@todo
     |         |     Fix problem X here.'
  83 | WARNING | [x] 'TODO see if we can get away without overriding entire IEF function.' should match the format '@todo Fix
     |         |     problem X here.'
 106 | WARNING | [ ] Unused variable $key.
 121 | WARNING | [x] 'TODO see if we can get away without overriding entire IEF function.' should match the format '@todo Fix
     |         |     problem X here.'
 158 | ERROR   | [ ] The array declaration extends to column 107 (the limit is 80). The array content should be split up over
     |         |     multiple lines

FILE: ./entity_browser_8.x-2.x/modules/entity_form/src/Plugin/EntityBrowser/Widget/EntityForm.php
 155 | ERROR | The array declaration extends to column 147 (the limit is 80). The array content should be split up over multiple
     |       | lines
 156 | ERROR | The array declaration extends to column 89 (the limit is 80). The array content should be split up over multiple
     |       | lines
 156 | ERROR | The array declaration extends to column 166 (the limit is 80). The array content should be split up over multiple
     |       | lines

FILE: ./entity_browser_8.x-2.x/src/Ajax/ValueUpdatedCommand.php
 17 | ERROR | Class property $details_id should use lowerCamel naming without underscores

FILE: ./entity_browser_8.x-2.x/src/Ajax/SelectEntitiesCommand.php
 9 | ERROR | Doc comment short description must be on a single line, further text should be a separate paragraph

FILE: ./entity_browser_8.x-2.x/src/WidgetInterface.php
 78 | WARNING | Line exceeds 80 characters; contains 83 characters

FILE: ./entity_browser_8.x-2.x/src/WidgetBase.php
 117 | ERROR | The array declaration extends to column 86 (the limit is 80). The array content should be split up over multiple
     |       | lines
 332 | ERROR | Parameter $form_state is not described in comment
 339 | ERROR | The array declaration extends to column 82 (the limit is 80). The array content should be split up over multiple
     |       | lines
 359 | ERROR | Doc comment short description must be on a single line, further text should be a separate paragraph

FILE: ./entity_browser_8.x-2.x/src/DisplayBase.php
 179 | ERROR | The array declaration extends to column 93 (the limit is 80). The array content should be split up over multiple
     |       | lines

FILE: ./entity_browser_8.x-2.x/src/SelectionDisplayBase.php
 133 | ERROR | The trigger_error message 'checkPreselectionSupport method is deprecated. Use supportsPreselection instead.' does
     |       | not match the relaxed standard format: %thing% is deprecated in %deprecation-version% any free text
     |       | %removal-version%. %extra-info%. See %cr-link%

FILE: ./entity_browser_8.x-2.x/src/Element/EntityBrowserElement.php
 257 | ERROR | [x] list(...) is forbidden, use [...] instead.

FILE: ./entity_browser_8.x-2.x/src/EntityBrowserInterface.php
 53 | ERROR | Parameter $widget_selector is not described in comment
 56 | ERROR | Doc comment for parameter $display does not match actual variable name $widget_selector
 64 | ERROR | Parameter $selection_display is not described in comment
 67 | ERROR | Doc comment for parameter $display does not match actual variable name $selection_display

FILE: ./entity_browser_8.x-2.x/src/WidgetSelectorBase.php
 28 | ERROR | [ ] Class property $widgets_ids should use lowerCamel naming without underscores
 43 | ERROR | [x] Use null coalesce operator instead of ternary operator.

FILE: ./entity_browser_8.x-2.x/src/Form/EntityBrowserForm.php
 131 | ERROR | The array declaration extends to column 100 (the limit is 80). The array content should be split up over multiple
     |       | lines
 152 | ERROR | The array declaration extends to column 109 (the limit is 80). The array content should be split up over multiple
     |       | lines

FILE: ./entity_browser_8.x-2.x/src/Form/WidgetsConfig.php
 164 | WARNING | t() calls should be avoided in classes, use \Drupal\Core\StringTranslation\StringTranslationTrait and $this->t()
     |         | instead

FILE: ./entity_browser_8.x-2.x/src/Form/EntityBrowserEditForm.php
  17 | WARNING | The class short comment should describe what the class does and not simply repeat the class name
 340 | WARNING | Unused variable $plugin_id.

FILE: ./entity_browser_8.x-2.x/src/RouteSubscriber.php
 50 | WARNING | [x] The variable name should be defined after the type
 66 | ERROR   | [ ] Protected method name "RouteSubscriber::getBrowserIDsWithRoute" is not in lowerCamel format

FILE: ./entity_browser_8.x-2.x/src/Events/EventBase.php
 45 | ERROR | Public method name "EventBase::getBrowserID" is not in lowerCamel format
 55 | ERROR | Public method name "EventBase::getBrowserInstanceUUID" is not in lowerCamel format

FILE: ./entity_browser_8.x-2.x/src/Events/Events.php
 12 | ERROR   | Doc comment short description must be on a single line, further text should be a separate paragraph
 19 | WARNING | Line exceeds 80 characters; contains 82 characters
 20 | WARNING | Line exceeds 80 characters; contains 85 characters
 20 | ERROR   | Doc comment short description must be on a single line, further text should be a separate paragraph
 27 | WARNING | Line exceeds 80 characters; contains 82 characters
 29 | ERROR   | Doc comment short description must be on a single line, further text should be a separate paragraph
 36 | WARNING | Line exceeds 80 characters; contains 81 characters
 37 | ERROR   | Doc comment short description must be on a single line, further text should be a separate paragraph

FILE: ./entity_browser_8.x-2.x/src/Events/EntitySelectionEvent.php
 35 | ERROR | Description for the @return value is missing

FILE: ./entity_browser_8.x-2.x/src/SelectionDisplayInterface.php
 69 | ERROR | The text '@deprecated Use ::supportsPreselection instead.' does not match the standard format: @deprecated in
    |       | %deprecation-version% and is removed from %removal-version%. %extra-info%.
 69 | ERROR | Each @deprecated tag must have a @see tag immediately following it

FILE: ./entity_browser_8.x-2.x/src/Plugin/Field/FieldWidget/FileBrowserWidget.php
 101 | ERROR | [ ] Doc comment for parameter $mime_type_guesser does not match actual variable name 
 370 | ERROR | [ ] The array declaration extends to column 133 (the limit is 80). The array content should be split up over
     |       |     multiple lines
 389 | ERROR | [ ] The array declaration extends to column 103 (the limit is 80). The array content should be split up over
     |       |     multiple lines
 406 | ERROR | [ ] The array declaration extends to column 103 (the limit is 80). The array content should be split up over
     |       |     multiple lines
 505 | ERROR | [x] Use null coalesce operator instead of ternary operator.
 511 | ERROR | [ ] The array declaration extends to column 129 (the limit is 80). The array content should be split up over
     |       |     multiple lines

FILE: ./entity_browser_8.x-2.x/src/Plugin/Field/FieldWidget/EntityReferenceBrowserWidget.php
 443 | ERROR   | [ ] The array declaration extends to column 89 (the limit is 80). The array content should be split up over
     |         |     multiple lines
 530 | WARNING | [ ] Unused variable $row_id.
 610 | ERROR   | [ ] The array declaration extends to column 97 (the limit is 80). The array content should be split up over
     |         |     multiple lines
 810 | ERROR   | [ ] The array declaration extends to column 81 (the limit is 80). The array content should be split up over
     |         |     multiple lines
 919 | WARNING | [x] 'TODO Figure out how to avoid using raw user input.' should match the format '@todo Fix problem X here.'

FILE: ./entity_browser_8.x-2.x/src/Plugin/views/display/EntityBrowser.php
 133 | ERROR | [x] Use null coalesce operator instead of ternary operator.

FILE: ./entity_browser_8.x-2.x/src/Plugin/views/filter/ContextualBundle.php
 53 | ERROR | Parameter $selection_storage is not described in comment

FILE: ./entity_browser_8.x-2.x/src/Plugin/EntityBrowser/Widget/View.php
  93 | WARNING | [x] 'TODO - do we need better error handling for view and view_display (in' should match the format '@todo Fix
     |         |     problem X here.'
 108 | ERROR   | [x] Use null coalesce operator instead of ternary operator.
 134 | ERROR   | [ ] The array declaration extends to column 149 (the limit is 80). The array content should be split up over
     |         |     multiple lines
 135 | ERROR   | [ ] The array declaration extends to column 130 (the limit is 80). The array content should be split up over
     |         |     multiple lines
 136 | ERROR   | [ ] The array declaration extends to column 127 (the limit is 80). The array content should be split up over
     |         |     multiple lines
 141 | WARNING | [ ] \Drupal calls should be avoided in classes, use dependency injection instead
 232 | ERROR   | [x] list(...) is forbidden, use [...] instead.
 260 | ERROR   | [x] list(...) is forbidden, use [...] instead.
 262 | ERROR   | [ ] The array declaration extends to column 170 (the limit is 80). The array content should be split up over
     |         |     multiple lines
 285 | ERROR   | [x] list(...) is forbidden, use [...] instead.
 297 | WARNING | [ ] ViewEntity::load calls should be avoided in classes, use dependency injection instead

FILE: ./entity_browser_8.x-2.x/src/Plugin/EntityBrowser/Widget/Upload.php
 101 | ERROR   | The array declaration extends to column 104 (the limit is 80). The array content should be split up over multiple
     |         | lines
 102 | ERROR   | The array declaration extends to column 99 (the limit is 80). The array content should be split up over multiple
     |         | lines
 102 | ERROR   | The array declaration extends to column 177 (the limit is 80). The array content should be split up over multiple
     |         | lines
 157 | WARNING | Line exceeds 80 characters; contains 81 characters

FILE: ./entity_browser_8.x-2.x/src/Plugin/EntityBrowser/SelectionDisplay/View.php
  41 | WARNING | [x] 'TODO - do we need better error handling for view and view_display' should match the format '@todo Fix problem
     |         |     X here.'
  52 | WARNING | [x] 'TODO - if there are entities that are selected multiple times this' should match the format '@todo Fix
     |         |     problem X here.'
  55 | ERROR   | [ ] The array declaration extends to column 81 (the limit is 80). The array content should be split up over
     |         |     multiple lines
  93 | ERROR   | [ ] The array declaration extends to column 146 (the limit is 80). The array content should be split up over
     |         |     multiple lines
 116 | ERROR   | [x] list(...) is forbidden, use [...] instead.

FILE: ./entity_browser_8.x-2.x/src/Plugin/EntityBrowser/SelectionDisplay/MultiStepDisplay.php
  94 | ERROR | The array declaration extends to column 81 (the limit is 80). The array content should be split up over multiple
     |       | lines
 205 | ERROR | The array declaration extends to column 86 (the limit is 80). The array content should be split up over multiple
     |       | lines
 374 | ERROR | The array declaration extends to column 82 (the limit is 80). The array content should be split up over multiple
     |       | lines
 401 | ERROR | The array declaration extends to column 83 (the limit is 80). The array content should be split up over multiple
     |       | lines

FILE: ./entity_browser_8.x-2.x/src/Plugin/EntityBrowser/FieldWidgetDisplay/RenderedEntity.php
 83 | ERROR | [x] Expected 1 space after "?"; 2 found
 84 | ERROR | [x] Expected 1 space after ":"; 2 found

FILE: ./entity_browser_8.x-2.x/src/Plugin/EntityBrowser/FieldWidgetDisplay/ImageThumbnail.php
 74 | ERROR | [x] Expected 1 space after "?"; 2 found
 75 | ERROR | [x] Expected 1 space after ":"; 2 found

FILE: ./entity_browser_8.x-2.x/src/Plugin/EntityBrowser/Display/Standalone.php
 48 | WARNING | [ ] Possible useless method overriding detected
 50 | WARNING | [x] '@TODO Implement it.' should match the format '@todo Fix problem X here.'
 57 | WARNING | [x] '@TODO Implement it.' should match the format '@todo Fix problem X here.'

FILE: ./entity_browser_8.x-2.x/src/Entity/EntityBrowser.php
 323 | WARNING | [x] 'TODO - this doesn't make much sense. Refactor.' should match the format '@todo Fix problem X here.'
 332 | WARNING | [x] 'TODO - this doesn't make much sense. Refactor.' should match the format '@todo Fix problem X here.'
 386 | WARNING | [x] 'TODO: Allow displays to define more than just path.' should match the format '@todo Fix problem X here.'

FILE: ./entity_browser_8.x-2.x/src/Controllers/EntityBrowserController.php
 77 | ERROR | The array declaration extends to column 194 (the limit is 80). The array content should be split up over multiple
    |       | lines

FILE: ./entity_browser_8.x-2.x/src/Controllers/EntityBrowserListBuilder.php
 33 | WARNING | [x] Inline @var declarations should use the /** */ delimiters

FILE: ./entity_browser_8.x-2.x/src/Controllers/EntityBrowserFormController.php
 107 | WARNING | [x] Inline @var declarations should use the /** */ delimiters
 109 | WARNING | [x] Inline @var declarations should use the /** */ delimiters

FILE: ./entity_browser_8.x-2.x/entity_browser.module
  81 | ERROR | [ ] The array declaration extends to column 198 (the limit is 80). The array content should be split up over
     |       |     multiple lines
 148 | ERROR | [x] list(...) is forbidden, use [...] instead.
 166 | ERROR | [x] list(...) is forbidden, use [...] instead.

FILE: ./entity_browser_8.x-2.x/entity_browser.api.php
 14 | WARNING | Line exceeds 80 characters; contains 92 characters
 24 | WARNING | Line exceeds 80 characters; contains 91 characters
 34 | WARNING | Line exceeds 80 characters; contains 88 characters
 44 | WARNING | Line exceeds 80 characters; contains 99 characters
 54 | WARNING | Line exceeds 80 characters; contains 103 characters
 65 | WARNING | Line exceeds 80 characters; contains 101 characters
CommentFileSizeAuthor
#37 enitity_browser.patch84.51 KBShreyas gowda
#34 phpcs-3036556-34.patch94.6 KBimustakim
#34 interdiff-30-34.txt23.81 KBimustakim
#30 phpcs-3036556-30.patch82.85 KBsakthi_dev
#24 3036556-Coding_standards_fixes-24.patch74.14 KBanabpv
#23 3036556-Coding_standards_fixes-23.patch74.14 KBanabpv
#22 3036556-Coding_standards_fixes-22.patch74.14 KBanabpv
#20 3036556-Coding_standards_fixes-20.patch74.14 KBanabpv
#18 3036556-18.patch38.22 KBguilhermevp
#16 interdiff_15-16.txt32.46 KBnmatja
#16 3036556-16.patch46.61 KBnmatja
#15 interdiff_11-15.txt8.69 KBnikitagupta
#15 3036556-15.patch25.33 KBnikitagupta
#11 3036556-11.patch22.17 KBotrolopezmas
#2 entity-browser-line-3036556.patch6.67 KBjigish.addweb

Issue fork entity_browser-3036556

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
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

jigish.addweb created an issue. See original summary.

jigish.addweb’s picture

jigish.addweb CreditAttribution: jigish.addweb at AddWeb Solution Pvt. Ltd. commented
FileSize
entity-browser-line-3036556.patch6.67 KB

Here's the patch that resolves the error that mentioned above.

oknate’s picture

oknate
Location Greater New York City Area
CreditAttribution: oknate as a volunteer and commented
Status: Active » Postponed (maintainer needs more info)

The patch changes many things that shouldn't be changed. Closing for now.

oknate’s picture

oknate
Location Greater New York City Area
CreditAttribution: oknate as a volunteer and commented
Title: README.txt does not follow best practices » Coding standards fixes
Assigned: jigish.addweb » Unassigned
Issue summary: View changes
Status: Postponed (maintainer needs more info) » Needs work
Issue tags: +Novice
oknate’s picture

oknate
Location Greater New York City Area
CreditAttribution: oknate as a volunteer and commented
Issue summary: View changes
oknate’s picture

oknate
Location Greater New York City Area
CreditAttribution: oknate as a volunteer and commented

Re-opening. I see a bunch of "Line exceeds 80 characters" warnings.

I haven't looked closely at the lines in question. I think these were missed because of the way I was linting the code, it was only fixing the ones it could automatically fix, using phpcbf, rather than just outputting a report using phpcs.

Once we fix these, I'll check the code base regularly with phpcs before committing code.

joshi.rohit100’s picture

joshi.rohit100
he/him
Primary language Hindi
Location Delhi, India
CreditAttribution: joshi.rohit100 at Acquia commented
Issue tags: +#DCD19

Adding for DCD-2019 code sprint.

willtryagain’s picture

willtryagain CreditAttribution: willtryagain commented
Assigned: Unassigned » willtryagain
willtryagain’s picture

willtryagain CreditAttribution: willtryagain commented
Assigned: willtryagain » Unassigned
sushyl’s picture

sushyl
Location Pune
CreditAttribution: sushyl at Axelerant commented
Issue tags: -#DCD19
otrolopezmas’s picture

otrolopezmas
He/Him
Primary language Spanish
CreditAttribution: otrolopezmas as a volunteer commented
Status: Needs work » Needs review
FileSize
3036556-11.patch22.17 KB

Ran codesniffer and fixed all issues I found. I solved some of the comment lines, that were longer than 80 character, reducing the first line to an anecdotal version and then inserting the full comment in the following lines, where it's allowed to have multiple lines.

Status: Needs review » Needs work

The last submitted patch, 11: 3036556-11.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

otrolopezmas’s picture

otrolopezmas
He/Him
Primary language Spanish
CreditAttribution: otrolopezmas as a volunteer commented

Tests are failing because of parent classes argument hints. Is there a way to fix this? Or this issue needs to be in standby until core is fixed?

nikitagupta’s picture

nikitagupta CreditAttribution: nikitagupta at Srijan | A Material+ Company for Drupal India Association commented
Assigned: Unassigned » nikitagupta
nikitagupta’s picture

nikitagupta CreditAttribution: nikitagupta at Srijan | A Material+ Company for Drupal India Association commented
Assigned: nikitagupta » Unassigned
Status: Needs work » Needs review
FileSize
3036556-15.patch25.33 KB
interdiff_11-15.txt8.69 KB

Update the patch #11 also fixed the test case.

nmatja’s picture

nmatja CreditAttribution: nmatja at Agiledrop - Your Trusted Drupal Teammates commented
FileSize
3036556-16.patch46.61 KB
interdiff_15-16.txt32.46 KB

Updated the patch #15.

guilhermevp’s picture

guilhermevp CreditAttribution: guilhermevp at CI&T commented
Assigned: Unassigned » guilhermevp
guilhermevp’s picture

guilhermevp CreditAttribution: guilhermevp at CI&T commented
Assigned: guilhermevp » Unassigned
FileSize
3036556-18.patch38.22 KB

Sending patch. Testing with phpcs will not return a clean state because some changes would result in worst readability. This patch has no interdiff, but is a sudo-reroll of #16

anabpv’s picture

anabpv
she/her
Location Portugal 🇵🇹
CreditAttribution: anabpv at CI&T commented
Assigned: Unassigned » anabpv

I will review this

anabpv’s picture

anabpv
she/her
Location Portugal 🇵🇹
CreditAttribution: anabpv at CI&T commented
Assigned: anabpv » Unassigned
FileSize
3036556-Coding_standards_fixes-20.patch74.14 KB

I ran Code Sniffer and Code Beautify and fixed:
- 80 characters lines
- Dependency injections
- Access restriction comment
- Unused variables
- t() calls avoidance
- trigger_error for the deprecated method
- Doc comments
- @see message

Awaiting for review.

Thank you!

tmaiochi’s picture

tmaiochi CreditAttribution: tmaiochi at CI&T commented
Assigned: Unassigned » tmaiochi

I'll review this.

anabpv’s picture

anabpv
she/her
Location Portugal 🇵🇹
CreditAttribution: anabpv at CI&T commented
FileSize
3036556-Coding_standards_fixes-22.patch74.14 KB

Apologies, I had a syntax error in the patch #20.
Hopefully this new one is correct.

Thank you

anabpv’s picture

anabpv
she/her
Location Portugal 🇵🇹
CreditAttribution: anabpv at CI&T commented
FileSize
3036556-Coding_standards_fixes-23.patch74.14 KB

Update of patch #22 (another syntax error)

anabpv’s picture

anabpv
she/her
Location Portugal 🇵🇹
CreditAttribution: anabpv at CI&T commented
FileSize
3036556-Coding_standards_fixes-24.patch74.14 KB

Update of patch #23 (another syntax error)

tmaiochi’s picture

tmaiochi CreditAttribution: tmaiochi at CI&T 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 #24 correctly applied all coding standards.

apaderno’s picture

apaderno
he/him
Primary language Italian
Location Brescia, 🇮🇹 🇪🇺
CreditAttribution: apaderno as a volunteer commented
Title: Coding standards fixes » Fix the issues reported by phpcs
Category: Bug report » Task
Priority: Normal » Minor
Issue tags: -Novice +Coding standards

I am not sure why only the Drupal ruleset is used, when there is also the DrupalPractice ruleset.

apaderno’s picture

apaderno
he/him
Primary language Italian
Location Brescia, 🇮🇹 🇪🇺
CreditAttribution: apaderno as a volunteer commented
Issue summary: View changes
Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs reroll
Kaustab_Roy’s picture

Kaustab_Roy CreditAttribution: Kaustab_Roy at Specbee commented
Assigned: Unassigned » Kaustab_Roy
apaderno’s picture

apaderno
he/him
Primary language Italian
Location Brescia, 🇮🇹 🇪🇺
CreditAttribution: apaderno as a volunteer commented
Assigned: Kaustab_Roy » Unassigned

Issues should be assigned to somebody who creates a patch or a MR in the next hours, not after more than 12 hours. This means blocking an issue for which somebody else could have worked on.

sakthi_dev’s picture

sakthi_dev CreditAttribution: sakthi_dev at Specbee for Drupal India Association commented
FileSize
phpcs-3036556-30.patch82.85 KB

Created a patch after verifying with DrupalPractice ruleset with . Please review.

apaderno’s picture

apaderno
he/him
Primary language Italian
Location Brescia, 🇮🇹 🇪🇺
CreditAttribution: apaderno as a volunteer commented
Issue tags: -Needs reroll 
  * @addtogroup hooks
- * @{

The last line is necessary and it should not be removed.

 /**
- * Alter the information provided in \Drupal\entity_browser\Annotation\EntityBrowserDisplay.
+ * Alter the information provided in.
+ *
+ * \Drupal\entity_browser\Annotation\EntityBrowserDisplay.

Short descriptions cannot be split like that.
Alter must use the third person singular.

- * TODO see if we can get away without overriding entire IEF function.
+ * @todo see if we can get away without overriding entire IEF function.

What follows @todo is a sentence: It starts with a capitalized word and ends with a period (preferable), a question mark, or an exclamation mark.

 /**
- * AJAX command to rerender a formatted text field without any transformation
- * filters.
+ * AJAX command to rerender a formatted text without transformation filters.
  */

I am not sure that removing field is correct. The existing comment is about a field, while the changed comment is about a formatted text (which is not a field).

   /**
-   * The ID for the details element.
+   * Details element ID.

That description is missing a definite article.

 /**
- * Class EntityBrowserEditForm.
+ * Entity browser edit form class.
  */
 class EntityBrowserEditForm extends EntityForm {

That description does not say what the class does.

-    if ($storage = $this->selectionStorage->get($form_state->get(['entity_browser', 'instance_uuid']))) {
+    if ($storage = $this->selectionStorage->get($form_state->get([
+      'entity_browser',
+      'instance_uuid',
+    ]))) {

As per Drupal coding standards, control structure conditions can be written on a single line, if they are more readable. In this case, the existing code is more readable.

-  /**
-   * {@inheritdoc}
-   */
-  public function displayEntityBrowser(array $element, FormStateInterface $form_state, array &$complete_form, array $persistent_data = []) {
-    parent::displayEntityBrowser($element, $form_state, $complete_form, $persistent_data);
-    // @TODO Implement it.
-  }
-
   /**
    * {@inheritdoc}
    */
   public function selectionCompleted(array $entities) {
-    // @TODO Implement it.
+    // @todo Implement it.
   }

The first method cannot be removed, exactly for the same reason the other one has not been removed: It contains a @todo tag.

+  /**
+   * Turns a render array into an HTML string.
+   *
+   * @var Drupal\Core\Render\RendererInterface
+   */
+  public $renderer;

That description says for what that property is used; it needs to say what that property contains.

+  /**
+   * Constructs a new View object.
+   *

The description for a constructor must start with Constructs a new followed by the class name (including its namespace), and end with object.

+   * @param string $plugin_id
+   *   The plugin_id for the plugin instance.

plugin_id is not an English word.
It is not necessary to say for the plugin instance, since that is a plugin ID.

+   * @param \Symfony\Component\EventDispatcher\EventDispatcherInterface $event_dispatcher
+   *   Event dispatcher service.

The description is missing a definite article.

+   * @param \Drupal\entity_browser\WidgetValidationManager $validation_manager
+   *   The Widget Validation Manager service.

Only the first word in the description must be capitalized.
There is no need to say service.

+   * @param Drupal\Core\Render\RendererInterface $renderer
+   *   Turns a render array into an HTML string.

That is not the description of what $render contains.

+    /*
+     * @deprecated and removed in entity_browser:8.x-2.2.
+     *   Use ::supportsPreselection instead.
+     *   @see
+     */

@deprecated must be added to the method documentation comment, not inside the method.

+   * @param \Drupal\Core\Form\FormStateInterface $form_state
+   *   Drupal form_state.

There is no need to say Drupal.
form_state is not an English word.

-  protected $widgets_ids;
+  protected $widgetsIds;
 
   /**
    * ID of the default widget.
@@ -40,7 +40,7 @@ abstract class WidgetSelectorBase extends PluginBase implements WidgetSelectorIn
   public function __construct($configuration, $plugin_id, $plugin_definition) {
     parent::__construct($configuration, $plugin_id, $plugin_definition);
     $this->setConfiguration($configuration);
-    $this->widget_ids = isset($this->configuration['widget_ids']) ? $this->configuration['widget_ids'] : [];
+    $this->widget_ids = isset($this->configuration['widget_ids']) ?? [];

Since the property name has been changed, $this->widget_ids is setting the wrong property.

roshni27’s picture

roshni27 CreditAttribution: roshni27 at Valuebound for Valuebound commented
Issue summary: View changes
imustakim’s picture

imustakim CreditAttribution: imustakim at Specbee for Drupal India Association commented
Assigned: Unassigned » imustakim
imustakim’s picture

imustakim CreditAttribution: imustakim at Specbee for Drupal India Association commented
Assigned: imustakim » Unassigned
Status: Needs work » Needs review
FileSize
interdiff-30-34.txt23.81 KB
phpcs-3036556-34.patch94.6 KB

As per suggestions by @apaderno, Changes have been made, and to address some of the concern mentioned in #31, only these errors are remaining.
Assuming these can be ignored. Rest of the errors are fixed.
Patch is updated, please review.

FILE: /Users/specbee/Sites/Projects/entity_browser/src/Form/EntityBrowserForm.php
---------------------------------------------------------------------------------------------------------------------------------------
FOUND 1 ERROR AFFECTING 1 LINE
---------------------------------------------------------------------------------------------------------------------------------------
 131 | ERROR | The array declaration extends to column 100 (the limit is 80). The array content should be split up over multiple lines
---------------------------------------------------------------------------------------------------------------------------------------


FILE: /Users/specbee/Sites/Projects/entity_browser/src/Plugin/EntityBrowser/Widget/View.php
-------------------------------------------------------------------------------------------
FOUND 0 ERRORS AND 1 WARNING AFFECTING 1 LINE
-------------------------------------------------------------------------------------------
 41 | WARNING | Line exceeds 80 characters; contains 85 characters
-------------------------------------------------------------------------------------------


FILE: /Users/specbee/Sites/Projects/entity_browser/src/Plugin/EntityBrowser/Display/Standalone.php
--------------------------------------------------------------------------------------------------
FOUND 0 ERRORS AND 1 WARNING AFFECTING 1 LINE
--------------------------------------------------------------------------------------------------
 48 | WARNING | Possible useless method overriding detected
--------------------------------------------------------------------------------------------------

Time: 3.16 secs; Memory: 18MB

Status: Needs review » Needs work

The last submitted patch, 34: phpcs-3036556-34.patch, failed testing. View results

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

Shreyas gowda’s picture

Shreyas gowda CreditAttribution: Shreyas gowda commented
FileSize
enitity_browser.patch84.51 KB

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

Shruu_rao’s picture

Shruu_rao CreditAttribution: Shruu_rao commented

Hi,
I tried applying #37 patch. The patch throws

error: corrupt patch at line 16

Please check.