Problem/Motivation

There is an anti-pattern in dependency injection in Frontend redirect subscriber:

https://git.drupalcode.org/project/lupus_decoupled/-/blob/1.x/modules/lu...

The rest of the code should be checked for di antipatterns as well.

Proposed resolution


https://mglaman.dev/blog/dependency-injection-anti-patterns-drupal

Issue fork lupus_decoupled-3392681

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

useernamee created an issue. See original summary.

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

elber opened merge request !54

elber’s picture

elber
Primary language Portuguese, Brazil
Location Brazil
commented
Status: Active » Needs review

Hi please revise, Am I correct?

useernamee’s picture

useernamee commented
Status: Needs review » Needs work

Hello @elber,
thank you for your timely fix. It is on point. However, there are some cosmetic changes needed.

I've checked for other occurrences of anti-patterns and did not find any.

elber’s picture

elber
Primary language Portuguese, Brazil
Location Brazil
commented
Status: Needs work » Needs review

Please revise

useernamee’s picture

useernamee commented
Status: Needs review » Reviewed & tested by the community

Looks good, thank you @elber.

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

fago’s picture

fago
Primary language German
Location Vienna
commented
Status: Reviewed & tested by the community » Needs work

Thank you for the contribution!

Yes, we should load the config as late as possible, but it seems the PR converts one call wrong.
Then, we should avoid loading the config multiple time in a single page load, thus I think we need to introduce some sort of getter to load the config and statically cache it in a local variable.

useernamee’s picture

useernamee commented
Status: Needs work » Needs review

I've added a property and g(/s)etter method on BaseUrlProvided and make FrontendRedirectSubscriber use that instead.

useernamee’s picture

useernamee commented

re-rolled the PR to the latest 1.x version

roderik’s picture

roderik
Primary language Dutch
Location Amsterdam,NL / Budapest,HU
commented
Status: Needs review » Needs work

NW for above comments (implement or tell me they're not useful)

useernamee’s picture

useernamee commented
Status: Needs work » Needs review

Implemented code review recommendation. (I've missed that setting service exists already).

roderik’s picture

roderik
Primary language Dutch
Location Amsterdam,NL / Budapest,HU
commented
Status: Needs review » Reviewed & tested by the community
nikolay shapovalov’s picture

nikolay shapovalov commented
Status: Reviewed & tested by the community » Needs work

Thanks for your contribution, I made review for this MR, can you please check my feedback?

As I understand phpunit fails is not related to this changes, and there is already a task to fix it #3402425: Fix PHPCS and maybe other errors.

roderik’s picture

roderik
Primary language Dutch
Location Amsterdam,NL / Budapest,HU
commented

@zniki.ru you are correct, this issue is not related to phpunit failures. Only #3402425: Fix PHPCS and maybe other errors is related.

(I started that issue while seeing some failures, and someone said "yes you should fix them", but I didn't know the history. So I opened #3402425: Fix PHPCS and maybe other errors without looking further... but as things grew, that issue has now turned into a duplicate of #3395669: Make the gitlab-ci pipeline pass.)

useernamee’s picture

useernamee commented

Fix comments and order of params.

I don't think we should use property promotion since we don't have a dependency on php8 in the project's composer.json.

useernamee’s picture

useernamee commented
Status: Needs work » Needs review
roderik’s picture

roderik
Primary language Dutch
Location Amsterdam,NL / Budapest,HU
commented
Status: Needs review » Needs work

Needs work for last comment, but I'm fine with you merging this immediately after the change (provided the change is only removing that public getter method).

(Tested current code, to be sure of previous statement.)

useernamee’s picture

useernamee commented
Status: Needs work » Needs review
roderik’s picture

roderik
Primary language Dutch
Location Amsterdam,NL / Budapest,HU
commented
Status: Needs review » Reviewed & tested by the community
useernamee’s picture

useernamee commented

  • useernamee committed 385cdc3f on 1.x authored by elber 
    Issue #3392681 by useernamee, elber, fago, roderik, zniki.ru: Dependency...
useernamee’s picture

useernamee commented
Status: Reviewed & tested by the community » Fixed

merged & fixed

huh it was ride, thank you for staying along

Status: Fixed » Closed (fixed)

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