Problem/Motivation

According to @mglaman having a get method in constructor is an anti pattern since the object is not lazy loaded anymore.

That is the case with ohdear_integration.settings service:

https://git.drupalcode.org/project/ohdear_integration/-/blob/2.x/ohdear_...

Proposed resolution

Should be replaced with config.factory service.

CommentFileSizeAuthor
#2 3403202-2.patch1.38 KBcasey

Issue fork ohdear_integration-3403202

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.

casey’s picture

casey commented
Status: Active » Needs review
StatusFileSize
new 3403202-2.patch1.38 KB

In development environments the api key is typically not provided. When the api key is missing currently every drush call triggers at least one " [error] OhDear site id is missing! Fix it ..."

useernamee’s picture

useernamee commented

@casey - good point - should we add some checkbox to tell the module that the site is not online? Or maybe we should just handle missing API key differently (e.g. we could move the functionality into submodule).

Are there any good examples from other modules, how they handle similar scenarios?

useernamee’s picture

useernamee commented
Status: Needs review » Needs work

I've checked the patch.
ohdear_integration.settings service should be removed as well from services.yml file.
Then from:
- https://git.drupalcode.org/project/ohdear_integration/-/blob/2.x/src/OhD...

Otherwise the patch looks good. It just need a few more things to take care of.

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

roderik opened merge request !19

roderik’s picture

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

I did the requested, and/but:

  • I didn't remove ohdear_integration.settings from the service file, because this is a published stable module and its users might be using the service. Instead, there's a deprecation message now. Maybe you can do a followup task for removing them in v3.
  • I also removed the logger service in the same way. If using a settings object/service is an antipattern, then a logger is an antipattern too. This made me remove the $logger class variable from various classes where it wasn't used.
  • fixed phpcs errors because there were few besides code I was already touching anyway.

I hope you don't mind long lines because I just replaced ohDearIntegrationSettings with configFactory->get('ohdear_integration.settings') on the same single line.

Tested, after a 'drush cr', to make sure I didn't do anything dumb:

  • executed getLogger() in OhDearIntegrationController and OhDearSdkService::getSiteId()

Did not test:

  • casey's change in OhDearSdkService::getSite() - because I have no Ohdear API key/settings handy. Reviewed the change, and it looks right / functionally-unchanged to me.
useernamee’s picture

useernamee commented

Reviewed and it looks good.

Thank you @casey & @roderik for you contributions.

  • useernamee committed 607c4b80 on 2.x authored by roderik 
    Issue #3403202 by roderik, casey, useernamee: Dependency injection anti-...
useernamee’s picture

useernamee commented
Status: Needs review » Fixed

Status: Fixed » Closed (fixed)

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