We use dependency injection of a logger channel in a lot of places, it might make sense to have a trait for that.
What this would especially allow us to fix is the DI-circumventing calls to watchdog_exception() – we could just copy that code to a logException() (e.g.) method on the trait, which uses the injected channel, and then use that instead.

Comments

drunken monkey created an issue. See original summary.

drholera’s picture

Checking.

drholera’s picture

Status: Active » Needs review
drholera’s picture

StatusFileSize
new23.57 KB

In provided patch was created LoggerTrait with the logException() method. This method uses the injected channel. Also in module changed watchdog_exception() function to the method in this trait.

Status: Needs review » Needs work

The last submitted patch, 4: 2753763-2--added_logger_trait.patch, failed testing.

drholera’s picture

StatusFileSize
new24.71 KB

Fix patch from PhpStorm-created to correct git-created one.

drholera’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 6: 2753763-6--added_logger_trait.patch, failed testing.

The last submitted patch, 6: 2753763-6--added_logger_trait.patch, failed testing.

drholera’s picture

Cannot reproduce tests fails on the local environment. Can anyone test #6 patch too?

a.romantsov’s picture

Applied patch #6 on D8.1.8 - no fails.

The last submitted patch, 6: 2753763-6--added_logger_trait.patch, failed testing.

borisson_’s picture

So I ran tests again on the bot w/ the default branch (8.3.x) and this produces 9 fails, I also ran them with 8.1.x and that patch is green. We'll have to figure out how to make the patch green for all versions of d8.

drholera’s picture

StatusFileSize
new664.91 KB

I ran tests locally in the 8.3.x core branch and search_api 8.x.1.x branch. All tests passed well. Need to understand why tests fail here. Tested on php 5.5, php 5.6 and php7.

mpp’s picture

See https://www.drupal.org/node/2784849#comment-11533091

Why is 8.3 the default branch for testing if 8.1 is currently the default branch?

drunken monkey’s picture

Thanks for the patch, great job!
However, while this correctly implements the second part of the proposal, it's missing the first part – i.e., implementing dependency injection in this trait and replacing all our current code for logger dependency injection with that (except for the necessary code in the create(), of course).
(This also means that logException() can't be static.)

drunken monkey’s picture

Issue tags: +release target
drunken monkey’s picture

Status: Needs work » Needs review
StatusFileSize
new37.16 KB
new40.98 KB

Here is my take on this, please review!

I've refrained from including a call to the setter in the various create() methods, though. I have been doing this religiously till now, and also argued for it over in #2311039: Implement proper dependency injection where possible, but in the end I don't really see why we need it. I thought it's the "clean" way to do it, but in the end everyone does it differently anyways, and I can't really see any benefit in having it there.
After all, in tests we can still just call setLogger() if we need to – why would we need the "detour" via create() for that? We usually don't even use that in tests.
Also, looking at some of our classes closely, if we really would want to keep to the rule of setting all the dependencies in create(), it would significantly bloat up some of them – for no apparent gain, at least to me.

Does anyone have any arguments why we should still include that (or why some other way of DI is better)?
If not, I guess we could even talk about removing such setter calls from all classes where we currently have them, for consistency's sake – maybe #2311039: Implement proper dependency injection where possible would be the place for that.

One question I then have, though, is why we have create() methods in the Core framework?
I guess just for the DI variant where you pass all dependencies in the constructor – but I can't really see the appeal of that, either.

Status: Needs review » Needs work

The last submitted patch, 18: 2753763-18--logger_trait.patch, failed testing.

drunken monkey’s picture

Status: Needs work » Needs review
StatusFileSize
new9.49 KB
new40.08 KB
borisson_’s picture

Status: Needs review » Reviewed & tested by the community

I think the create method is only really useful when you're not using setters for DI. I like that because it reduces the available methods from a class and that makes the public API smaller. Because we're only using setter DI, we don't need to worry about the create method and these changes look good. Setting to RTBC.

drunken monkey’s picture

Status: Reviewed & tested by the community » Fixed

OK, thanks for your input on this!
Also thanks for reviewing – committed.

Status: Fixed » Closed (fixed)

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