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
Path "" not prefixed with slash.
Validation message is shown if Pages textarea is empty. D7 version does not validate for "/" as this is a new feature in D8.
Proposed resolution
Fix the incorrect validation.
Remaining tasks
Review patch.
User interface changes
None.
API changes
None.
Data model changes
None.
Hello,
Thank you for this awesome module.
I was making tests and I emptied the specific page textarea and I got a validation error.
I will upload a patch.
| Comment | File | Size | Author |
|---|---|---|---|
| #20 | 2826956-20-reverted.patch | 3.54 KB | pfrenssen |
| #20 | 2826956-20.patch | 2.66 KB | pfrenssen |
| |||
| #2 | piwik-validate_specific_pages-2826956-2.patch | 1.17 KB | Grimreaper |
Comments
Comment #2
GrimreaperHere is the patch.
Thanks for the review.
Comment #3
GrimreaperComment #4
hass CreditAttribution: hass commentedComment #5
hass CreditAttribution: hass commentedComment #6
PolHi,
I worked on a patch where pages are saved in an array instead of a concatenated string.
Do you think it would be usefull for this project ? I've got a working POC.
Comment #7
pfrenssenSeems like 2 different issues, this is a straightforward bug fix. Saving the pages as an array would be a nice improvement but it's out of scope for this I think.
Comment #8
hass CreditAttribution: hass commentedBlock visibility was cloned from core and should work like core. If core now uses an array, we should also do this. If not, let‘s keep it as is, please.
Comment #9
hass CreditAttribution: hass commentedAside, can you verify if the „fix“ you are trying here, clones the block logic? Maybe block system has the same problem and there is a reason for this I currently cannot remember...
Comment #10
hass CreditAttribution: hass commentedComment #12
hass CreditAttribution: hass commentedComment #13
hass CreditAttribution: hass commentedoh, we need a test
Comment #14
GrimreaperThanks for the commit.
Comment #15
PolIn the meantime, I've checked in the D8 code, the Request Path condition plugin (core/modules/system/src/Plugin/Condition/RequestPath.php) and it's using a concat string to store its path, so, the idea to use array to store path is not valid in this context.
By the way, would it be a good idea to use the request path condition plugin in Matomo instead of basically redoing the same ?
Thanks.
Comment #16
pfrenssenAssigning to write the test.
Comment #17
pfrenssenHere is a test. I went straight for a modern PHPUnit test instead of adding to the existing browser tests, so that this doesn't impact the work on #2941830: Fix tests.
I have also provided a variant of the patch that has the original fix reverted, so that we can check that the test correctly detects the bug.
Comment #20
pfrenssenForgot to declare the required test group.
Comment #22
pfrenssenBack to needs review, the reverted patch failed as expected.
Comment #23
hass CreditAttribution: hass commentedYes, I just had not understood how in past... if you know how, let's do it.
Comment #24
hass CreditAttribution: hass commentedThe patch seems not testing the bug we fixed here... we should make sure this does not get introduced by fault again somedays in future.
Comment #25
pfrenssenIt's not so obvious, but the first part of the data provider is testing the bug from this issue:
This means that if nothing is filled in for the specific pages there should be no validation error. I've uploaded a version of the test that reverts the patch of this issue, and then the test fails.
Comment #27
hass CreditAttribution: hass commentedComment #28
PolOnly the test has been committed ?
Comment #29
hass CreditAttribution: hass commentedSee #11
Comment #30
PolOops ! Thanks !