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.When creating an exposed filter, the description states:
" This will appear in the URL after the ? to identify this filter. Cannot be blank. Only letters, digits and the dot ("."), hyphen ("-"), underscore ("_"), and tilde ("~") characters are allowed. "
Although the character "^" (and possibly others?) are also allowed.
Is this wanted behaviour?
No, due to a typo in the regex the characters A-z are now allowed, this matches a character in the range "A" to "z" (char code 65 to 122). This should just be A-Z. Currently extra allowed characters by this part of the regex are [\]^_`, with the proposed changed, only the underscore would still be allowed from that list since that is whitelisted separately already.
Release note snippet:
Validation for allowed characters in Views filter identifiers was more lenient than intended. If any of your views currently uses a character in the identifier that is other than letters, digits, the dot (.), hyphen (-), underscore (_), or tilde (~), you will need to reconfigure the exposed filters without them. Read the change record on how Views exposed filters identifiers are now validated correctly for more information.
| Comment | File | Size | Author |
|---|---|---|---|
| #19 | 2731333-19.patch | 1.77 KB | jofitz |
| #6 | 2731333-06.patch | 1.74 KB | jhedstrom |
| #6 | 2731333-06-TEST-ONLY.patch | 1.03 KB | jhedstrom |
Comments
Comment #2
jhedstromThis is currently explicitly allowed in the validator, but that is probably a mistake:
From
\Drupal\views\Plugin\views\filter\FilterPluginBase:Simply removing that character from the regex doesn't work though for some reason.
Comment #3
jhedstromer, nevermind, the
^there is supposed to match any character except those specified.Comment #4
jhedstromApparently there is a typo in the current one (
A-zvsA-Zthat makes the exclusion behave very oddly).This should fix the issue, and there's a test to demonstrate the current fails.
Comment #5
jhedstromOops, I accidentally removed the
~.Comment #6
jhedstromIgnore patches from #5, they accidentally include some composer changes.
Comment #11
StryKaizerNice catch on that regex typo, patch fixes the issue, setting rtbc
Comment #12
alexpottWhat about existing bad values? Do we just accept that they have to be fixed if someone saves the view again?
Comment #13
alexpottSetting to needs review to get an answer to #12 - i think it might be fine it is still worth having a think about before committing this.
Comment #14
StryKaizerFollowing characters match current regex (Basicly every character between A and z in the ASCII table, see http://www.ascii-code.com/).
[ \ ] ^ `_also matches, but we support underscores anyway.Since writing an update path can break links, I'd accept people have to fix their alias next time they save the view.
Comment #18
borisson_This doesn't apply anymore.
Comment #19
jofitz CreditAttribution: jofitz at ComputerMinds commentedRe-rolled.
In #14 @StryKaizer answered the question from #12 so if this re-roll is correct this can be returned to RTBC.
Comment #23
Manuel Garcia CreditAttribution: Manuel Garcia as a volunteer and at Appnovation for Pfizer, Inc. commentedReroll looks good, and still applies to 8.8.x
Re #12:
Essentially I agree with #14. An upgrade path would not be possible without breaking links inside text fields, and those links would still work after this change since this only takes place while validating views configuration changes being saved. Site builders will see the problem when they try to save views configuration, at which point it would (should) become clear to them that they should change any links to these pages accordingly.
We could try to warn them perhaps via a CR, though this is probably an uncommon case I'm not sure it's worth the effort?
Comment #24
LendudeUpdated the IS a bit to make it clearer which characters are now allowed that will be disallowed after this change.
If we are not doing an upgrade path (fine by me), shouldn't we at least do a CR so that people can have some indication of what is going on when they run into this?
Comment #25
Manuel Garcia CreditAttribution: Manuel Garcia as a volunteer and at Appnovation for Pfizer, Inc. commentedCreated the CR: https://www.drupal.org/node/3040204
Comment #26
larowlanThis will warrant pointing out in release notes - can we add a release notes snippet to the issue summary
Comment #27
LendudeAdded a snippet
Comment #28
alexpottCrediting @StryKaizer for filing the issue. @Manuel Garcia, @Lendude, myself, and @larowlan for issue review and management.
Committed d3a1192 and pushed to 8.8.x. Thanks!
Comment #29
alexpottComment #32
xjmComment #33
xjmAnd adding a link to the CR.