Problem/Motivation
Found gazillions of errors in our watchdog. Debugging shows that a template fallback like this triggered ComponentLoader to log them:
{% include ['c4c:state-transition-help--' ~ workflowId, 'c4c:state-transition-help']
The offending code from \Drupal\Core\Template\Loader\ComponentLoader::exists:
public function exists($name): bool {
if (!preg_match('/^[a-zA-Z][a-zA-Z0-9:_-]*[a-zA-Z0-9]?$/', $name)) {
return FALSE;
}
try {
$this->pluginManager->find($name);
return TRUE;
}
catch (ComponentNotFoundException $e) {
Error::logException($this->logger, $e);
return FALSE;
}
}
Steps to reproduce
See above.
Proposed resolution
Drop the Error::logException line without replacement.
Simply checking the existence of a component should not flood the dblog.
As a bonus, remove the internal findTemplate() function which is unused.
Remaining tasks
Code, review, commit.
User interface changes
None.
Introduced terminology
None.
API changes
None.
Data model changes
None.
Release notes snippet
None.
| Comment | File | Size | Author |
|---|
Issue fork drupal-3488260
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:
Comments
Comment #2
geek-merlinFull error and trace:
Comment #4
kensae commented@geek-merlin I discovered the same issue (https://www.drupal.org/project/drupal/issues/3530032)
Your solution also fixes the issue.
Comment #5
geek-merlinSetting NW to update the tests.
Comment #6
geek-merlinSee related issue.
Comment #7
geek-merlinPatch is trivial and removes a log that should not be logged. I suppose it does not need a test, as similar patches do not have them.
Tests are green. Setting rtbc per #4.
Comment #8
geek-merlinComment #9
longwaveThis was the only use of
$this->loggerso that can be dropped from the constructor as well.Comment #10
longwaveRemoved the logger from the constructor and the test.
Also noticed that
findTemplate()isn't part of the interface and appears to be never be called, so I removed that as well.Comment #11
dcam commentedI'm fairly certain the parameter doesn't need to be deprecated. But I don't know if this needs a change record or not as an API change. Something to consider. Otherwise, the changes are simple enough. I'll RTBC it.
I updated the issue summary with the info about removing
findTemplate().Comment #12
quietone commentedA better title?
Comment #14
needs-review-queue-bot commentedThe Needs Review Queue Bot tested this issue. It fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".
This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.
Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.
Comment #15
dcam commentedPost-bot-rebellion rebase
Comment #16
nod_Not sure about this. What if you expect the template to be loaded but it's not, how do you figure that out as a frontend dev? In the use case from the issue summary it makes sense that this is noisy but it also removes it from regular includes.
Comment #17
pdureau commentedIndeed, in this very specific case, using
{% include %}tag (instead of the recommendedinclude()function) with a list of components (that are checked one by one for template existence), we don't want to log errors if we don't find a template.I guess the original authors of
ComponentLoaderdidn't think about this case, whcih is rarely met.I don't have numbers to prove my point but I am not sure most frontend devs are working with a logged account and are watching the Drupal logs ;)
So, I am in favor of merging this change. Can I do it soon or we still need to discuss ?
Comment #18
nod_go for it
Comment #19
pdureau commentedI will merge soon, i just do a last rebase to make the pipleine run a last time on up-to-date codebase.
Comment #21
pdureau commentedCommitted 791670b and pushed to main. Thanks!