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
This issue was fixed on #2417333: Add support for user-path: scheme to Url class but no test were added for dblog. This issue add the tests for that fix.
Proposed resolution
Add the tests for the fix defined on #2417333: Add support for user-path: scheme to Url class.
Remaining tasks
None
User interface changes
None
API changes
None
Data model changes
None
Original report by @eosrei
The dblog detail pages contain links to Location and Referrer, but they are broken.
Example on fresh install: /admin/reports/dblog/event/24
Type user
Date Wednesday, November 19, 2014 - 17:27
User Anonymous (not verified)
Location http://d8.local/core/install.php/?langcode=en&profile=standard
Referrer http://d8.local/core/install.php?langcode=en&profile=standard
Message Session opened for root.
Severity Notice
Hostname 127.0.0.1
Operations
In the example above:
http://d8.local/core/install.php/?langcode=en&profile=standard
links to:
http://d8.local/http%3A//d8.local/core/install.php/%3Flangcode%3Den%26profile%3Dstandard
The attached patch removes the extra 'base://':
$this->l($dblog->location, $dblog->location ? Url::fromUri('base://' . $dblog->location) : Url::fromRoute('<none>')),
Beta phase evaluation
| Issue category | Bug |
|---|---|
| Unfrozen changes | Unfrozen because it only changes a broken UI in the dblog module |
| Comment | File | Size | Author |
|---|---|---|---|
| #23 | interdiff-2378617-17-22.txt | 707 bytes | dagmar |
| #22 | dblog-detail-links-2378617-22.patch | 1.69 KB | dagmar |
| #17 | dblog-detail-links-2378617-16.patch | 1.68 KB | mgifford |
| #16 | dblog-detail-links-2378617-16.patch | 1.68 KB | jhedstrom |
| #12 | dblog-detail-links-2378617-12-TEST-ONLY.patch | 1.68 KB | jhedstrom |
Comments
Comment #1
13rac1 CreditAttribution: 13rac1 commentedComment #2
jhedstromI looked into why there isn't a test for this already. The issue is that the link will change based on where/how the test is run (from the UI, they'll be batch page urls, from the script or drush I'm not sure what they'd be set to, but they'd be different).
The attached patch fixes the issue in manual testing.
I also added a beta phase evaluation to the summary.
Comment #5
jsobiecki CreditAttribution: jsobiecki commentedI'm not sure why issue is in "Needs work" state. After re-testing all tests are green.
Patch applies cleanly (at least for today) and fixes visible problem.
Changes are quite minor, and (IMHO) make sense.
+1 From me.
Comment #6
jhedstromThis is back to RTBC--I'd missed that the testbot falsely marked this as needs work.
Comment #7
alexpottThis looks very testable.
Comment #8
jhedstromRe: #7 you're totally right, not sure what I was thinking in #2.
Attached is a patch with tests for these links.
Note that I didn't use
assertLinkByHref()since that was passing without the fix since it only looks for links that *contain* rather than match exactly.Comment #11
jhedstromInteresting, so when the tests are run via run-tests.sh, the referer/location links are empty as mentioned in #2. I'll pursue a path of just injecting some fake data into the watchdog table that will be independent of testing environment.
Comment #12
jhedstromI added a separate test method for verifying the detail page. There is a bit of overlap with the existing
verifyReports()method, but as mentioned above, relying on the actual values in the report doesn't work since they change based on how the test is run. This test injects known data and tests against that.Comment #16
jhedstromReroll of #12.
Comment #17
mgiffordRe-uploading for bots.
Comment #18
alexpottThe fact this is a test only patch implies this has been fixed by another issue and the issue summary is out of date... did the issue that fixed it add a test or is a test only issue relevant?
Comment #19
jhedstromThe issue that fixed it is #2417333: Add support for user-path: scheme to Url class, and, as far as I can tell, did not add tests for the db log, so this could still be relevant.
Comment #20
mgiffordComment #22
dagmarThis patch looks good. I updated the issue summary and just wrapped a long comment line to 80 characters.
Comment #23
dagmarComment #24
dagmarMoving to RTBC since #22 is almost identical to #17. The issue summary has been updated to reflect why this patch only provides tests.
Comment #25
alexpottComment #26
alexpottCommitted e598c06 and pushed to 8.1.x and 8.2.x. Thanks!