Problem/Motivation

It is recommended to use dependency injection instead of calls to \Drupal:: in OO-code.

This project fails to do so. While the current code works, it is the recommended way to do things.

Proposed resolution

Identify the code that need to be replaced and fix them.

CommentFileSizeAuthor
#4 content_access-use_dependency_injection-3170415-4.patch1.22 KBgisle
#2 Use_dependency_injection_where_possible-3170415-2.patch8.9 KBGnanagowthaman sankar
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

gisle created an issue. See original summary.

Gnanagowthaman sankar’s picture

Gnanagowthaman sankar CreditAttribution: Gnanagowthaman sankar as a volunteer and at UniMity Solutions Pvt Limited commented
Assigned: Unassigned » Gnanagowthaman sankar
Status: Active » Needs review
Issue tags: +Drupal India Association, +DIACWSep2020
FileSize
Use_dependency_injection_where_possible-3170415-2.patch8.9 KB

Hi,

I had attached the patch please review and let me know for any changes.

Thanks & regards,
Gnanagowthaman Sankar

Status: Needs review » Needs work

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

gisle’s picture

gisle
he/him
Primary language Norwegian Bokmål
Location Norway
CreditAttribution: gisle at Hannemyr Nye Medier AS commented
Assigned: Gnanagowthaman sankar » Unassigned
Category: Plan » Task
Issue summary: View changes
Status: Needs work » Needs review
Issue tags: -Drupal India Association, -DIACWSep2020
FileSize
content_access-use_dependency_injection-3170415-4.patch1.22 KB

Patch failed testing. To me, it looks like the patch in #2 was made against a very outdated version of the project.

Made a new patch aginst HEAD.

gisle’s picture

gisle
he/him
Primary language Norwegian Bokmål
Location Norway
CreditAttribution: gisle at Hannemyr Nye Medier AS commented
Status: Needs review » Fixed

Status: Fixed » Closed (fixed)

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

TR’s picture

TR CreditAttribution: TR commented

This commit (41767c3) needs to be reverted.

Everything about this commit is wrong. It just broke things, it didn't fix anything. And specifically it didn't *inject* anything - $entityTypeManager is defined but it is not set anywhere, so it is always NULL. That can never work. And $configFactory is never even defined and also never set. Again, this will always be NULL.

Blindly following recommendations for dependency injection without understanding how to actually inject dependencies and without understanding how traits are used for code reuse is sheer folly. All this did was break the code.

gisle’s picture

gisle
he/him
Primary language Norwegian Bokmål
Location Norway
CreditAttribution: gisle at Hannemyr Nye Medier AS commented
Status: Closed (fixed) » Needs work

I'll take your word for it. Any additional advice you can offer shall be appreciated.

TR’s picture

TR CreditAttribution: TR commented

I opened #3226616: Revert commit from issue #3170415 because I didn't have permission to reopen this current issue and I didn't know if anyone would see my comment on this closed issue. We don't need both open.

My advice is to do a git revert 41767c3, which will commit the inverse of 41767c3. The result will be like the above patch never happened. That is what we want, because everything in the patch is wrong.

There are two uses of \Drupal:: in the trait that this will restore, but because that trait needs to be refactored and eliminated (these really should be methods on the plugin base class, and NOT be in a trait), those two uses can be handled in the refactoring issue as appropriate.

There's a lot of work needed to get the Rules integration working - it was never really ported from D7 properly. This was just the first thing I noticed immediately when looking at the code for the trait and trying to figure out what it was supposed to do. I will be opening a separate meta issue for all the other Rules-related things I'm finding.

  • gisle authored 94a646d on 8.x-1.x 
    Issue #3170415 by gisle: Rolled back to commit fcd3522
gisle’s picture

gisle
he/him
Primary language Norwegian Bokmål
Location Norway
CreditAttribution: gisle at Hannemyr Nye Medier AS commented
Status: Needs work » Needs review

It is now rolled back to commit fcd3522.

Revert "Issue #3170415 by Gnanagowthaman sankar, gisle: Use dependency injection"
This reverts commit 41767c3c6ea47a7545f9e1e7e8faddf0ec760c65.

Please review that this worked out OK.

I'm going to reapply those subsequent commits that was removed by this reversal, i.e.:

  • Issue #3226014 by TR, marco5775, gisle: Fixed Rules integration to use context_definitions
  • Issue #3199390 by CoderBrandon: Fix PHP notice in content_access_get_role_gid
  • By gisle: Fixed two mis-spellings
  • Issue #3178217 by gisle: Fixed exception when using the reset to defaults button

  • gisle authored 0ad5133 on 8.x-1.x 
    Issue #3170415 by gisle: Reapplied patches from 3178217, 3199390,...
TR’s picture

TR CreditAttribution: TR commented

It looks like you used git reset or something other than git revert. With git revert, it just should have committed the inverse patch, so no other commits should be affected and you shouldn't have to re-commit anything else.

But I think the end result of what you did is correct. I will check.

gisle’s picture

gisle
he/him
Primary language Norwegian Bokmål
Location Norway
CreditAttribution: gisle at Hannemyr Nye Medier AS commented
Status: Needs review » Closed (outdated)

No further comments. Closing as outdated.