Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
Problem/Motivation
The DbLogController::eventDetails() method has the following lines:
// ...
[
['data' => $this->t('Location'), 'header' => TRUE],
$this->l($dblog->location, $dblog->location ? Url::fromUri($dblog->location) : Url::fromRoute('<none>')),
],
[
['data' => $this->t('Referrer'), 'header' => TRUE],
$this->l($dblog->referer, $dblog->referer ? Url::fromUri($dblog->referer) : Url::fromRoute('<none>')),
],
// ...
This is forcing to render as links things that may not be valid links. Also, is calling l() method even if there is nothing to render. There are currently two different issues regarding this problem:
- #2858182: Exception thrown when viewing dblog entries created from requests containing non-routable URIs in referer header
- #2755497: InvalidArgumentException in dblog entries due to invalid location
Proposed resolution
- Create a new method to render things as links, plain text or nothing.
- Only create a link if there is something to link.
- Make sure the link is valid (@see #2858182: Exception thrown when viewing dblog entries created from requests containing non-routable URIs in referer header)
- Write tests (See examples in #2755497: InvalidArgumentException in dblog entries due to invalid location)
Remaining tasks
Write a patch.
User interface changes
None.
API changes
New protected method in DbLogController.
Data model changes
None.
Comment | File | Size | Author |
---|---|---|---|
#46 | 2868725-46.patch | 4.65 KB | Krzysztof Domański |
#46 | interdiff-42-46.txt | 1.77 KB | Krzysztof Domański |
#42 | 2868725-42.patch | 4.91 KB | Krzysztof Domański |
#42 | interdiff-39-42.txt | 3.79 KB | Krzysztof Domański |
Comments
Comment #2
cilefen CreditAttribution: cilefen commentedShall we postpone the other issues on this one?
Comment #3
dagmarI think we should close them as duplicated. But let's wait a few days so they don't get lost on the hidden duplicated queue.
Comment #4
cilefen CreditAttribution: cilefen commentedPosting my patch from the other issue.
Comment #5
dawehnerComment #6
xlin CreditAttribution: xlin as a volunteer commentedCombined patch from 2755497 and 2858182.
Comment #7
dawehnerIMHO this should document why we need this if here.
Comment #8
xlin CreditAttribution: xlin as a volunteer commentedAdded new method to format the referer URL.
Comment #9
dawehnerNeeds work for #7
Comment #10
dagmarThe approach of formatReferer should be abastracted in something that we can use also for location. Also the parameter $row doesn't seem accurate. We should accept a random string and make sure that it can be rendered as a link.
Comment #13
dagmarThe remaining things to change here are quite simple to implement. See #10
Comment #14
msankhala CreditAttribution: msankhala as a volunteer and at Srijan | A Material+ Company commentedThis method is not being used anymore.
Comment #15
msankhala CreditAttribution: msankhala as a volunteer and at Srijan | A Material+ Company commentedComment #16
msankhala CreditAttribution: msankhala as a volunteer and at Srijan | A Material+ Company commentedHere is updated patch as per suggestions in #10. Removed unused
protected function cleanReferer(Url $url)
from patch and updated test cases so that it does not require changes mentioned in #7.Comment #17
msankhala CreditAttribution: msankhala as a volunteer and at Srijan | A Material+ Company commentedBy mistake uploaded the same patch of #7. Here is updated patch.
Comment #19
joachim CreditAttribution: joachim commentedShould give context here to explain when the link might not be valid -- because it's an empty string? because the logger passed in just a plain text message? Something else?
Needs to say what you get back if it's not valid.
Comment #20
msankhala CreditAttribution: msankhala as a volunteer and at Srijan | A Material+ Company commentedUpdated method docblock and return type description as per #19
Comment #21
msankhala CreditAttribution: msankhala as a volunteer and at Srijan | A Material+ Company commentedComment #22
dagmarThanks @msankhala
You can just add the calls inside the array. No need to create two new variables.
This is not watchdog specific. I'm not a native english speaker so I may not be the best to define the docs here, but I expect to see something like: "Some log attributes like Referer or Location can be rendered as links if they are correctly formatted. This function returns a link version of an uri if it can be successfully render as a link. It fallback to the string version of the uri otherwise."
Return a Link object if the uri can be converted as a link. In case of emtpy uri, or invalid, fallback to plain string.
There are no test coverage at all for location in the DbLogTest class. Could you add a extra assertion in DbLogTest::testLogEventPage ?
Comment #23
msankhala CreditAttribution: msankhala as a volunteer and at Srijan | A Material+ Company commentedThanks, @dagmar for feedback.
and
I think these are explanatory enough. I'll try to add test for location in DbLogTest::testLogEventPage.
Comment #24
msankhala CreditAttribution: msankhala as a volunteer and at Srijan | A Material+ Company commentedComment #25
msankhala CreditAttribution: msankhala as a volunteer and at Srijan | A Material+ Company commentedHere is updated patch as per suggestion in #22
Comment #26
msankhala CreditAttribution: msankhala as a volunteer and at Srijan | A Material+ Company commentedDamn!! missed to hit upload button to upload patch before uploading interdiff. Here we go with both patch and interdiff.
Comment #27
msankhala CreditAttribution: msankhala as a volunteer and at Srijan | A Material+ Company commentedComment #28
msankhala CreditAttribution: msankhala as a volunteer and at Srijan | A Material+ Company commentedComment #29
dagmarThis issue is trying to solve two things:
But is currently including tests for the last one.
Take a look to https://www.drupal.org/project/drupal/issues/2858182#comment-11977909 and https://www.drupal.org/project/drupal/issues/2858182#comment-12033546
We should include in the DbLogTest a new method that create a log with an invalid referer and location and make sure they are returned as plain strings.
Comment #30
msankhala CreditAttribution: msankhala as a volunteer and at Srijan | A Material+ Company commentedComment #31
msankhala CreditAttribution: msankhala as a volunteer and at Srijan | A Material+ Company commentedUpdated patch as per #29
Comment #32
dagmarGreat! almost there. Just a few final comments.
This change is not necessary.
s/render/rendered
s/fallback/fallbacks
s/fallback/fallbacks
This assertion should check that the text of nth-child(4) matches with '/some/incorrect/url'
You could use a valid referer url here and ensure is a link.
Comment #33
msankhala CreditAttribution: msankhala as a volunteer and at Srijan | A Material+ Company commentedThis change is not necessary.
I think we can keep this change to maintain consistency.
$message
variable is also being used only once which can be replaced by$this->formatMessage($dblog)
.s/render/rendered - This is valid.
s/fallback/fallbacks - IDE gives typo error on this change.
s/fallback/fallbacks - IDE gives typo error on this change.
I'll update test case according to point 4 and 5.
Comment #34
msankhala CreditAttribution: msankhala as a volunteer and at Srijan | A Material+ Company commentedHere is updated patch.
Comment #35
msankhala CreditAttribution: msankhala as a volunteer and at Srijan | A Material+ Company commentedComment #36
dagmarThanks so much @msankhala. Excellent work.
Comment #37
alexpottThis change is not required for the change.
This can be a null so the typehint should be a
string|null
We should also test a log entry where these are not set at all. The test hints that it is doing a test where there is no request_uri or referer. But it doesn't appear to be.
This should be in a separate issue it is testing code that is not changed by the fix as far as I can see so it seems confusing.
Comment #39
Krzysztof DomańskiNew patch according to #37.
Comment #41
dagmarThanks! @Krzysztof Domański
I think this block of code can be replaced with the new generateLogEntries method.
Same here
Comment #42
Krzysztof DomańskiI used
generateLogEntries
method to inject fake log data andDatabase::getConnection()->query($query)->fetchField()
to get the last log.Comment #43
dagmarThanks @Krzysztof Domański
I've created the follow up requested by @alexpott in #37 here. #3039201: Follow up of #2868725. More test coverage in LoggerChannelTest.
Comment #44
Krzysztof DomańskiRe #41
I think we can change this also in the other tests. I've created issue.
#3039207: Use the generateLogEntries method in DBLogTest
Comment #45
alexpottLet's call this createLink and change the text to
Creates a Link object if the provided URI is valid.
This text is a bit verbose and repeats information from the one liner. And doesn't deal with the null situation. I'm not sure it is necessary at all.
Can return NULL no? Could say
fallback to the provided $uri.
also@return \Drupal\Core\Link|string|null
Comment #46
Krzysztof DomańskiRe #45.1 and #45.2. A new description is enough, so I have removed a long text.
Comment #47
dagmarThanks @Krzysztof Domański
@alexpott comments from #45 are addressed in #46.
Comment #48
alexpottCommitted and pushed 511bdacdd2 to 8.8.x and 2b5e56a9cc to 8.7.x. Thanks!