Problem/Motivation

Fix all dependency injection problems in this module.

Steps to reproduce

Run phpcs

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

Proposed resolution

Send a patch
Test it
Commit

Remaining tasks

-

User interface changes

-

API changes

-

Data model changes

Comments

tmaiochi created an issue. See original summary.

tmaiochi’s picture

Working on it.

tmaiochi’s picture

Assigned: tmaiochi » Unassigned
Status: Active » Needs review
StatusFileSize
new9.08 KB

I fixed all dependency injection errors in PHPCS. There are other PHPCS errors and warnings that will be fixed in other issues.

victoria-marina’s picture

Assigned: Unassigned » victoria-marina

I'll review it.

victoria-marina’s picture

Assigned: victoria-marina » Unassigned
Status: Needs review » Needs work

@tmaiochi as the class UpgradeStatusCommands is a service, it needs to have the new injected dependency as an argument on the .services file.

tmaiochi’s picture

Status: Needs work » Needs review
StatusFileSize
new9.41 KB

@victoria-marina Thanks, I forgot to change there. This is a new patch with this change.

lucienchalom’s picture

Status: Needs review » Needs work

Hello, I reviewed and found that on file UpgradeStatusForm.php, because of the injection of the dependency of users Roles, the use statement of Roles is no longer used, so we have a new phpcs error for unused use statement.

Besides that, everything looks good and the module is working as far as I can test it.

tmaiochi’s picture

StatusFileSize
new9.46 KB

Thanks for your review @lucienchalom, I removed this unused use statement in this new patch.

tmaiochi’s picture

Status: Needs work » Needs review
lucienchalom’s picture

Status: Needs review » Reviewed & tested by the community

Thank you!

With that patch, the dependency injections were fixed, no new errors were added, everything looks good and the module is working as far as I can test it.
Moving to RTBC

gábor hojtsy’s picture

Title: Dependency Injection » Fix missing dependency injections
Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs manual testing

Thanks, this looks good other than these two IMHO:

  1. +++ b/src/Commands/UpgradeStatusCommands.php
    @@ -43,6 +44,13 @@ class UpgradeStatusCommands extends DrushCommands {
    +   * The key value.
    
    @@ -54,16 +62,20 @@ class UpgradeStatusCommands extends DrushCommands {
    +   *   The key value.
    

    I would say this is "The key-value storage" or somesuch. What is the comment for such properties elsewhere in this module or core?

  2. +++ b/src/Form/UpgradeStatusForm.php
    @@ -120,6 +122,27 @@ class UpgradeStatusForm extends FormBase {
    +   * The users permission.
    

    Is this the current user's permissions? That would be clearer.

Also as the drush command does not have automated test coverage, did you manually test that it still works?

tmaiochi’s picture

Status: Needs work » Needs review
StatusFileSize
new9.61 KB
new751 bytes

Hey @Gábor Hojtsy, I changed the documentation for these ones. About the drush command I tested manually and work fine.

lucienchalom’s picture

Status: Needs review » Reviewed & tested by the community
StatusFileSize
new69.25 KB

"The key value." was changed to "Defines the key/value store factory interface."
which I agree is better explained.

For some reason, the User permission is not showing on the interdiff? but was changed to "Defines an interface to list available permissions."

I tested in the interface, but now I also tested the dush command for checkstyle.
Thank you for the reminder!

gábor hojtsy’s picture

Version: 8.x-3.x-dev » 4.x-dev
Status: Reviewed & tested by the community » Needs work
Issue tags: -Needs manual testing

Sorry for not integrating this when it was done. Unfortunately definitely does not apply anymore. Also 3.x is not supported anymore.

Drush commans now have some automated test coverage so that will not need to be manually tested.

benstallings’s picture

Status: Needs work » Closed (outdated)

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

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

Maintainers, please credit people who helped resolve this issue.

gábor hojtsy’s picture

Status: Closed (outdated) » Closed (duplicate)

I did add all credits to #3459216: Fix all phpcs issues though, so your efforts are not uncredited. Thanks all!