If an URL contains '[' or ']' characters, _filter_url function don't work properly.
ex:
- http://mysite.fr/node/ognodeadd?type=asset&gids=1074 is recognized as an URL
- http://mysite.fr/node/ognodeadd?type=asset&gids[]=1074 is not recognized as an URL
See attached patch for correction.
Comment | File | Size | Author |
---|---|---|---|
#30 | issue-190466_3.patch | 2.38 KB | kscheirer |
#24 | issue-190466_2.patch | 2.45 KB | kscheirer |
#22 | issue-190466_1.patch | 2.48 KB | kscheirer |
#20 | issue-190466.patch | 2.28 KB | lilou |
#15 | filter_url.patch | 1.96 KB | kleinmp |
Comments
Comment #1
David Stosik CreditAttribution: David Stosik commentedHello,
I work with sdelbosc, I just want to add that you have to clean filter_cache table.
(and also, sdelbosc's post is a perfect example of what does not work).
Regards,
David
Comment #2
drummI think this patch has been reversed. The '][' characters should be added, not removed. These should be added at the end of the [...] of valid characters since ranges always go first. And the ']' probably needs to be escaped.
Comment #3
David Stosik CreditAttribution: David Stosik commentedHello,
Why did you put this bug report in 6.x-dev? I think this has to be corrected in a future 5.x release.
I modified the patch provided by sdelbosc to reflect the real modification. The two square brackets don't need to be escaped, as there is no special character inside a [] class, they are considered as simple characters (and it is a PERL rule that is verified when using preg_replace ;) ).
Please review this patch and tell me if it is correct.
Thanks,
David
Comment #4
drummIt is generally best to fix issues on the development branch, currently 6.x for bug fixes, and then backport as needed. If a patch needs work I generally push it to the best version to do work on. Since there is now a reviewable patch for 5.x, it can stay here.
Comment #5
David Stosik CreditAttribution: David Stosik commentedThanks! :)
Comment #6
drummSorry, I must have thought this was a backport or something. It should be resolved for the development branch first, and then backported as needed. This patch does apply fine against HEAD, so a new one is not needed now. The code looks okay. This might get more attention on HEAD since there are more people working on the development version.
Comment #7
catchThis needs a test with it - haven't looked in detail, but there's a lot of failing tests included in #276597: TestingParty08: filter.module so it may just be a case of using one of those and combining it with this patch.
Comment #8
kleinmp CreditAttribution: kleinmp commentedHere's the patch again, but it was patched against drupal 7.
Comment #9
catchThis still needs to be combined with the 'filter URL doesn't work with square brackets' test from http://drupal.org/files/issues/filter-failing-tests_php.txt
Comment #10
kleinmp CreditAttribution: kleinmp commentedThanks, catch. I added the 'filter URL doesn't work with square brackets' test into the patch.
Comment #11
kleinmp CreditAttribution: kleinmp commentedJust tested this patch using Simpletest and it failed. I fixed a minor spacing issue, so I'm reposting it.
Comment #12
kleinmp CreditAttribution: kleinmp commentedThis patch passed the test for me.
Comment #14
catchkleinmp - try re-rolling from the root of your Drupal directory.
Comment #15
kleinmp CreditAttribution: kleinmp commentedOk, here it is re-rolled from the root folder.
Comment #16
kleinmp CreditAttribution: kleinmp commentedForgot to change status.
Comment #17
catchWhen the test bot is happy I'll RTBC this.
Comment #19
sunFailing test seems to be obvious. (Added closing curly-brace does match any opening brace)
Can we add some more tests? (than just the case this issue is about?)
Comment #20
lilou CreditAttribution: lilou commentedReroll (delete curly-brace).
Comment #22
kscheirerLooks good, all tests pass. Rerolled against HEAD, changed variable name to
$f
and moved test case totestUrlFilter()
since that's where the other tests for _filter_url() are.Comment #24
kscheirerrerolled against HEAD
Comment #26
cburschkaLet's have an inline comment explaining the case - "test for correct treatment of square brackets".
(Maybe some other special characters that should be accepted could also be placed into this test URL...)
This review is powered by Dreditor.
Comment #27
sunComment #28
kscheirerThe test case already states 'Converting URLs -- addresses with square brackets.' I don't think we need to duplicate that as a comment.
Comment #29
kscheirer#24: issue-190466_2.patch queued for re-testing.
Comment #30
kscheirerBah, we don't add the title to the link anymore since #784790: URL filter displays URL as link title, so rolled a new patch. With comment. Coming up on 3 years, I think this one is ready to go in!
Comment #31
meeli CreditAttribution: meeli commentedI looked at the latest dev for 8 and it looks like there are tests for absolute URLs with square brackets. This means that you can put square brackets in all your URLs and Drupal will no longer remove them!
The test is in /core/modules/filter/lib/Drupal/filter/Tests/FilterUnitTest.php.
And this issue is now only for D7.
Because this is a bug report we need tests because patch in #30 has no tests.
Also, the patch in #30 needs a reroll.
Review bonus #2094585: [policy, no patch] Core review bonus for #1898464: toolbar.module - Convert theme_ functions to Twig
Comment #32
Sivaji_Ganesh_Jojodae CreditAttribution: Sivaji_Ganesh_Jojodae commentedI think this issue is not valid for D7 anymore.
We have different code to handle absolute URLs in filter.module
In filter.test, there is a test case for checking url with [], snip below,