Closed (fixed)
Project:
Search API
Version:
8.x-1.x-dev
Component:
General code
Priority:
Normal
Category:
Task
Assigned:
Unassigned
Issue tags:
Reporter:
Created:
22 Jun 2016 at 15:04 UTC
Updated:
1 Mar 2017 at 13:44 UTC
Jump to comment: Most recent, Most recent file
Comments
Comment #2
drholera commentedChecking.
Comment #3
drholera commentedComment #4
drholera commentedIn 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.
Comment #6
drholera commentedFix patch from PhpStorm-created to correct git-created one.
Comment #7
drholera commentedComment #10
drholera commentedCannot reproduce tests fails on the local environment. Can anyone test #6 patch too?
Comment #11
a.romantsov commentedApplied patch #6 on D8.1.8 - no fails.
Comment #13
borisson_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.
Comment #14
drholera commentedI 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.
Comment #15
mpp commentedSee 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?
Comment #16
drunken monkeyThanks 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.)Comment #17
drunken monkeyComment #18
drunken monkeyHere 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" viacreate()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.
Comment #20
drunken monkeyComment #21
borisson_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.
Comment #23
drunken monkeyOK, thanks for your input on this!
Also thanks for reviewing – committed.