Problem/Motivation

There are issues reported by phpcs for this module.

FILE: src/FormAlter.php
----------------------------------------------------------------------------------------------
FOUND 0 ERRORS AND 2 WARNINGS AFFECTING 2 LINES
----------------------------------------------------------------------------------------------
 231 | WARNING | \Drupal calls should be avoided in classes, use dependency injection instead
 248 | WARNING | \Drupal calls should be avoided in classes, use dependency injection instead
----------------------------------------------------------------------------------------------

Time: 305ms; Memory: 12MB

Steps to reproduce

Run phpcs --standard="Drupal,DrupalPractice" --extensions=php,module,inc,install,test,profile,theme,css,info,yml .

Proposed resolution

Provide patch file that addresses these issues

CommentFileSizeAuthor
#24 ntity_browser_block_layout-dependency_injection-3345859-24.patch2.98 KBliam morland
#23 entity_browser_block_layout-dependency_injection-3345859-23.patch2.98 KBliam morland
#22 entity_browser_block_layout-dependency_injection-3345859-22.patch5.08 KBliam morland
#21 entity_browser_block_layout-dependency_injection-3345859-21.patch3.05 KBliam morland
#19 DI_entity_browser_block_layout-3345859-19.patch2.6 KBpriyanka_chauhan31
#17 Dependency_injection_entity_browser_block_layout-3345859-17.patch2.58 KBpriyanka_chauhan31
#9 3345859-2-before.png282.15 KBrajneeshb
#9 3345859-2-after.png280.49 KBrajneeshb
#3 3345859--entity_browser_block_layout--phpcs-fixes-2.patch3.9 KBdavidburns
#2 3345859--entity_browser_block_layout--phpcs-fixes.patch2.9 KBdavidburns

Issue fork entity_browser_block_layout-3345859

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

davidburns created an issue. See original summary.

davidburns’s picture

davidburns
he/him
Primary language English
Location Philadelphia
commented
StatusFileSize
new 3345859--entity_browser_block_layout--phpcs-fixes.patch2.9 KB
davidburns’s picture

davidburns
he/him
Primary language English
Location Philadelphia
commented
StatusFileSize
new 3345859--entity_browser_block_layout--phpcs-fixes-2.patch3.9 KB

Fixed a couple of other PHPCS things that popped up after applying that patch.
Here's the updated version that has everything passing.

[test:phpstan]  ------ -----------------------------------------------------------------------------------
[test:phpstan]   Line   modules/custom/entity_browser_block_layout/src/FormAlter.php
[test:phpstan]  ------ -----------------------------------------------------------------------------------
[test:phpstan]   77     Unsafe usage of new static().
[test:phpstan]          💡 See: https://phpstan.org/blog/solving-phpstan-error-unsafe-usage-of-new-static
[test:phpstan]  ------ -----------------------------------------------------------------------------------
avpaderno’s picture

avpaderno
he/him
Primary language Italian
Location Brescia, 🇮🇹 🇪🇺
commented
Title: Fix PHPCS Errors » Fix the issues reported by phpcs
Status: Active » 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.

(The patches fail to apply too.)

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

hardikpandya’s picture

hardikpandya commented
Title: Fix the issues reported by phpcs » Fix Drupal Coding Standards | phpcs
Assigned: davidburns » hardikpandya
Issue summary: View changes

hardikpandya opened merge request !2

hardikpandya’s picture

hardikpandya commented
Assigned: hardikpandya » Unassigned
Status: Needs work » Needs review
rajneeshb’s picture

rajneeshb commented
StatusFileSize
new 3345859-2-after.png280.49 KB
new 3345859-2-before.png282.15 KB

Reviewed MR 2, Looks good, +1RTBC

Attaching screenshots for the reference.

avpaderno’s picture

avpaderno
he/him
Primary language Italian
Location Brescia, 🇮🇹 🇪🇺
commented
Version: 1.0.0 » 1.0.x-dev
Priority: Normal » Minor
Issue tags: -Needs issue summary update
liam morland’s picture

liam morland
Primary language English
Location Ontario, CA 🇨🇦
commented
Status: Needs review » Needs work

This will need a re-roll because of #3347720: Use constructor property promotion. Please use constructor property promotion for the new properties.

nikhil_110’s picture

nikhil_110 commented
Assigned: Unassigned » nikhil_110

i try to resolve this issue

avpaderno’s picture

avpaderno
he/him
Primary language Italian
Location Brescia, 🇮🇹 🇪🇺
commented
Title: Fix Drupal Coding Standards | phpcs » Fix the issues reported by phpcs
nikhil_110’s picture

nikhil_110 commented
Assigned: nikhil_110 » Unassigned
liam morland’s picture

liam morland
Primary language English
Location Ontario, CA 🇨🇦
commented
Title: Fix the issues reported by phpcs » Use dependency injection
priyanka_chauhan31’s picture

priyanka_chauhan31 commented
Assigned: Unassigned » priyanka_chauhan31

Hi, Working on this issue.

priyanka_chauhan31’s picture

priyanka_chauhan31 commented
Assigned: priyanka_chauhan31 » Unassigned
Status: Needs work » Needs review
StatusFileSize
new Dependency_injection_entity_browser_block_layout-3345859-17.patch2.58 KB

Hi, I tried to solve the issue. Here is the patch attached for the same.

liam morland’s picture

liam morland
Primary language English
Location Ontario, CA 🇨🇦
commented
Status: Needs review » Needs work

On PHP 8.2, I expect there will be deprecation notices because of the use of dynamic properties. $this->destination and $this->renderer need to be declared. This can be done like it is for $entityTypeManager by adding a keyword like protected in the constructor.

priyanka_chauhan31’s picture

priyanka_chauhan31 commented
Status: Needs work » Needs review
StatusFileSize
new DI_entity_browser_block_layout-3345859-19.patch2.6 KB

Hi, I have added protected keyword in constructor as said. Please review my new patch.

liam morland’s picture

liam morland
Primary language English
Location Ontario, CA 🇨🇦
commented

Thanks @priyanka_chauhan31. After adding protected, you don't need to assign the values; that happens automatically. Please test the merge request.

liam morland’s picture

liam morland
Primary language English
Location Ontario, CA 🇨🇦
commented
StatusFileSize
new entity_browser_block_layout-dependency_injection-3345859-21.patch3.05 KB

MR as a patch.

liam morland’s picture

liam morland
Primary language English
Location Ontario, CA 🇨🇦
commented
StatusFileSize
new entity_browser_block_layout-dependency_injection-3345859-22.patch5.08 KB

MR as a patch.

liam morland’s picture

liam morland
Primary language English
Location Ontario, CA 🇨🇦
commented
StatusFileSize
new entity_browser_block_layout-dependency_injection-3345859-23.patch2.98 KB
liam morland’s picture

liam morland
Primary language English
Location Ontario, CA 🇨🇦
commented
StatusFileSize
new ntity_browser_block_layout-dependency_injection-3345859-24.patch2.98 KB

  • Liam Morland committed 488f7ff9 on 1.0.x 
    Issue #3345859: Remove unnecessary injection of string_translation...

  • Liam Morland committed 5aae61ab on 1.0.x 
    Issue #3345859 by davidburns, hardikpandya, priyanka_chauhan31: Use...
liam morland’s picture

liam morland
Primary language English
Location Ontario, CA 🇨🇦
commented
Status: Needs review » Fixed

Status: Fixed » Closed (fixed)

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