I just read this article and it describes something as problematic which we do in serveral classes (e.g., the DB backend plugin): inside the static create() method, set $logger = $container->get('logger.factory')->get('search_api');.
Apparently, this can lead to problems in vaguely described (probably not even fully understood) circumstances, potentially involving AJAX (forms). The "proper" way of doing this is described in the handbook.
Questions:
- Does anyone know whether this really is a problem?
- Relatedly: Should we implement the "proper" way for this that's mentioned in the handbook?
Once decided, this is more or less a novice issue: just add the logger channels to the .services.yml files and then use them instead of the logger factory in constructors. (To find out which classes might use the wrong workflow: git grep -l 'logger.factory.*get.*search_api'.)
| Comment | File | Size | Author |
|---|---|---|---|
| #11 | add_a_search_api_logger-2716487-11.patch | 1.66 KB | anicky |
| #6 | 2716487-6--logger_channel_services.patch | 11.28 KB | drunken monkey |
Comments
Comment #2
borisson_I'm not sure if this is a problem but it does look like a nicer way. So I'm all for this.
Comment #3
anicky commentedIt seems to be a proper way for me too. I can handle this issue.
Comment #4
drunken monkeyOK, then let's do this. Thanks!
Comment #5
anicky commentedComment #6
drunken monkeyGreat job, thanks a lot!
Already looks quite good, and seems like it should work. (Unfortunately, I don't think we're testing logging in any way at the moment, so it's hard to tell. But some quick manual testing worked fine, too.)
I just did a bit of clean-up and also replaced all
\Drupal::logger()calls. Please see the attached patch.Also removed a
@todocomment and instead created a proper issue for it: #2753763: Add a logging trait which can also log exceptions.If you're fine with these additional changes, I can commit it.
Comment #7
borisson_Comment #8
chandeepkhosa commentedComment #10
drunken monkeyOK, thanks for reviewing!
Committed.
Thanks again, Anicky!
Comment #11
anicky commentedSorry about all the
\Drupal::logger()calls. I only searchedlogger.factory...It seems that you forgot some
setLoggerin your refactoring. I made a patch based on the latest, which was committed.Comment #12
drunken monkeyNo problem at all, just noticed them myself – otherwise I'd have mentioned them in the IS.
Whether I inlined the container call was just based on whether there were other calls in the method – if all others use three lines, it seems weird if just one uses a single line. (Unifying this to always use a single line would be a different issue.)
Also, your patch contains trailing whitespace in the empty lines. But I think we'll just leave it at that.
Thanks again, in any case!