It is a good practice to use dependency injection. Please note that ShieldSettingsForm is conditionally using plugin.manager.key.key_type service so dependency injection should take care of it.

It is follow up of https://www.drupal.org/project/shield/issues/2996397#comment-13579073

CommentFileSizeAuthor
#8 shield-3132344-7-di.patch6.3 KBgeek-merlin
#7 shield-3132344-7-di.patch0 bytesgeek-merlin
#3 shield-dependency-injection-3132344-3.patch6.59 KBvbouchet
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

vbouchet created an issue. See original summary.

vbouchet’s picture

vbouchet
Location Toulouse
CreditAttribution: vbouchet at Acquia commented
Issue summary: View changes
vbouchet’s picture

vbouchet
Location Toulouse
CreditAttribution: vbouchet at Acquia commented
FileSize
shield-dependency-injection-3132344-3.patch6.59 KB

Please find a patch.

vbouchet’s picture

vbouchet
Location Toulouse
CreditAttribution: vbouchet at Acquia commented
Status: Active » Needs review

Status: Needs review » Needs work

The last submitted patch, 3: shield-dependency-injection-3132344-3.patch, failed testing. View results

vbouchet’s picture

vbouchet
Location Toulouse
CreditAttribution: vbouchet at Acquia commented
Status: Needs work » Needs review

Not sure why CI moves to "Need work" given it is a success.

geek-merlin’s picture

geek-merlin
they/omg
Primary language German
Location Freiburg, Germany
CreditAttribution: geek-merlin at Geeks4Change, MachbarMacher commented
FileSize
shield-3132344-7-di.patch0 bytes

Thanks! I think we can simplify this a lot with nullable constructor injection, let's see.

Also i committed #2953625: Add tests so i hope the bot will be more friendly now.

geek-merlin’s picture

geek-merlin
they/omg
Primary language German
Location Freiburg, Germany
CreditAttribution: geek-merlin at Geeks4Change, MachbarMacher commented
FileSize
shield-3132344-7-di.patch6.3 KB

ANd here's the patch.

vbouchet’s picture

vbouchet
Location Toulouse
CreditAttribution: vbouchet at Acquia commented
Status: Needs review » Reviewed & tested by the community

Thanks for the follow up and tip ;-) Looks all good on my side.

geek-merlin’s picture

geek-merlin
they/omg
Primary language German
Location Freiburg, Germany
CreditAttribution: geek-merlin at Geeks4Change, MachbarMacher commented
Status: Reviewed & tested by the community » Fixed

Thanks! Committed and pushed.

  • geek-merlin committed f0eaf1e on 8.x-1.x 
    Issue #3132344 by geek-merlin, vbouchet: Dependency injection

Status: Fixed » Closed (fixed)

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