Upgrading from 7.x-1.3 to 7.x-1.4 broke several url links of the website for which I am responsible.
The fields links all belong to a field collection inside a node. I can no longer change the nodes with fields link. I had to disable URL validation.
When I'm editing the node, the url validation failed. It reports:
"The value content/dates-des-sessions provided for A lire aussi is not a valid URL."
I check the uri and the flag while the execution of the function link_validate_url. It fails when calling file_exists
with the argument set to "public://content/dates-des-sessions". Neither publication nor saving the draft are possible. Without validation I'm able to publish the node and there isn't any problem since the target exists.
This issue may be related to this one #2247261: URLs are not validated
Comment | File | Size | Author |
---|---|---|---|
#61 | link-revert_url_validation-2666912-61.patch | 8.43 KB | Den Tweed |
#55 | link-fix-internal-validation-2666912-55.patch | 568 bytes | cboyden |
| |||
#54 | link-revert-url-validation-2666912-54.patch | 9.42 KB | cboyden |
#51 | link-2666912-51.patch | 603 bytes | Pete B |
#46 | revert-url-validation-2666912-43.patch | 9.32 KB | jamaral86 |
Comments
Comment #2
stephmto CreditAttribution: stephmto commentedI made another test without using clean url (I put node/xx) while editing the node. The url become valid and I'm able to publish the node.
Comment #3
nickvanboven CreditAttribution: nickvanboven as a volunteer commentedWrong related isseu cant delete it
Comment #4
nickvanboven CreditAttribution: nickvanboven as a volunteer commentedComment #5
mathieuhelie CreditAttribution: mathieuhelie at Floe design + technologies commentedReleasing the 1.4 version of link with an entirely new layer of URL validation was not a backward-compatible change. The new validation logic breaks on multiple stable use cases and people are trying to patch in their own use-case in #2651742: always Not A Valid URL.
This was a new feature and should have been implemented as a beta version of a 2.0 branch of the link module, then an upgrade path should be planned for it to migrate our valid content to the new validation logic.
An alternative was proposed in #2651742: always Not A Valid URL to add an extra option for validation instead, which means the only change needed to current validation is with the labeling, to something like "validate as a URL", and the new feature as "validate as an accessible internal link".
Comment #6
mathieuhelie CreditAttribution: mathieuhelie at Floe design + technologies commentedThe attached patch reverts the new url validation logic and returns to simply verifying what type of URL has been provided.
Comment #7
mathieuhelie CreditAttribution: mathieuhelie at Floe design + technologies commentedComment #8
hugronaphor CreditAttribution: hugronaphor as a volunteer and at Acrosto for Dropsolid commentedWorth to mention that Recoverable fatal error when saving translation also is caused by this and the patch in comment #6 brings it back to normal.
Comment #9
stephmto CreditAttribution: stephmto commentedThank you, I used the patch and it's working now.
Comment #10
Marty2081 CreditAttribution: Marty2081 at LimoenGroen commentedApplying the patch from #6 to Link 7.x-1.4 works.
Comment #11
eelkeblokAdding the issue that added this functionality ([#2247261: URLs are not validated]) as a related issue.
I think it makes sense to "send this back to the drawing board", by first simply reverting it (which is what #6 seems to do). I'd say it is up for debate how to exactly implement it (i.e. 2.0 version, or make it configurable to use new or legacy validation), but existing data needs to be taken into serious consideration (which will need to be done if it is done as a 2.0 too) before attempting this one again. I'll be applying the patch to several sites in a minute, if the results are good I'll mark it RTBC.
Comment #12
drupov CreditAttribution: drupov commentedWorks for me too
Comment #13
aaron.ferris CreditAttribution: aaron.ferris commentedAnother +1 for the patch in #6.
Comment #14
cozzamara CreditAttribution: cozzamara commentedSame issue for me. I did not try any patch, and I wait a release updating. Please fix soon, thanks
Comment #15
millerrs CreditAttribution: millerrs commentedCan also confirm the patch from #6 works.
Comment #16
dalinOne thing to flag with this patch is that it removes several docblocks. Presumably these were added by the patch that we're undoing. So we should either keep those, or be sure to recreate them in whatever "back to the drawing board" comes out of this.
Comment #17
mallezieJust adding info.
I noticed (probably not complete) following behaviour changes. Are were valid urls in 1.2, and not in 1.4.
Link to /node/1 (with trailing slash #2850681: Regression: Allow leading slash in URL
Links to redirects (redirect module)
Language dependant links with path aliases. Link on english node to untranslated node.
Comment #18
RenrhafConfirming that #6 fixes the validation for internal links.
Just adding info too : there is a typo in the tests at 'Make sure link to specific article valiates as news.'
Comment #19
fengtanWe are also affected by #1323278: Recoverable fatal error when saving translation and patch 6 fixes the problem. A workaround is to disable the URL validation.
Comment #20
hugronaphor CreditAttribution: hugronaphor as a volunteer and at Acrosto for Dropsolid commentedThis is an old thread but, I guess instead of reverting the intended feature there should be a way to improve it. Maybe there is a better way!?
Comment #21
pefferen CreditAttribution: pefferen at Triquanta commentedAs stated in comment #5, this new feature is not backwards compatible and should only be implemented in a new major release. The patch in #6 works for our project.
Comment #22
jschrab CreditAttribution: jschrab commentedWhat more needs to be done to put this into 1.5? I can definitely confirm that I am seeing false positives on validation for multi-language sites and I would love to see this patch in a full non-dev release.
Comment #23
pelicani CreditAttribution: pelicani commentedWe installed this to get the link functionality back to how it worked before 1.4
Our client has documentation on the flow of the URL field that we don't want to update with the new validation.
And we don't want to disable validation.
The URLs are now working as before.
The only item I notice is when you post a node URL with the leading slash, the alias is not used.
i.e. /node/1 does not find it's alias but node/1 does find it's alias
Figure it should find its alias for either option.
Thank you all for this patch.
peace,
Michael
Comment #24
pifagorDear community. Please check again patch. Thank you.
Comment #25
John Cook CreditAttribution: John Cook at Creode commentedWhile I'm having the same issue with existing content not validating. I've taken to changing the content so that the validator would accept it.
I had one problem. The language was being sent to 'und' but the language of the linked to node was 'en' - this made the validation fail.
I created a new issue, #2952204: Validating with language code 'und' can fail, to address this.
Comment #26
fskreuz CreditAttribution: fskreuz commentedRewrote patch the best I could against dev.
Comment #27
fskreuz CreditAttribution: fskreuz at Portland Webworks commentedUgh, I hate it when IDEs generate patches. Ignore #26. Updated patch.
Comment #28
fskreuz CreditAttribution: fskreuz commentedComment #29
maticb CreditAttribution: maticb commentedThe patch in #28 works on 7.15, but fails to patch the link.validate.test
Comment #30
idebr CreditAttribution: idebr at iO commentedRerolled #28 against 7.x-1.x
Comment #32
dsnopekHere's #30 re-rolled against 7.x-1.5, so that it can be used in Drush .make files.
Comment #33
jamaral86 CreditAttribution: jamaral86 commentedPorted to work with the latest release per PSA-2019-02-19
Comment #34
sjerdoFixed missing comma in test.
Comment #36
jamaral86 CreditAttribution: jamaral86 commentedPorted to work with the latest release per PSA-2019-02-19
(missing last commit) from 1.x
Comment #37
jamaral86 CreditAttribution: jamaral86 commentedPorted to work with the latest release per PSA-2019-02-19
actually, I made a mistake on the source branch, I changed it to tag 7.x-1.6
Comment #38
sjerdoThe tests are still broken.
Comment #39
jamaral86 CreditAttribution: jamaral86 commentedfixed lint error
Comment #40
jamaral86 CreditAttribution: jamaral86 commentedfixed lint error
Comment #41
sjerdoComment #42
Alina Basarabeanu CreditAttribution: Alina Basarabeanu commentedUpdating the patch to work with 1.6 version
Comment #43
thetoast CreditAttribution: thetoast commented#42 patch worked for me. Thanks for posting alinaBasa.
Comment #44
alesr CreditAttribution: alesr commentedJust correct me if I'm wrong.
Link module doesn't have to be patched if you are not using any kind of Services/API modules?
Comment #45
thetoast CreditAttribution: thetoast commentedWell, if you haven't updated your Link module to a version where the validation breaks, and you don't use services, then you don't need to update your module, or apply the patch. Sound right?
Comment #46
jamaral86 CreditAttribution: jamaral86 commentedFixed lint error and also fixed tests
Comment #47
scottalan CreditAttribution: scottalan at Phase2 commentedComment #49
coconnor CreditAttribution: coconnor commentedNote for people still on 1.4 who have instituted the patch on #6...
If you're looking for a patch for the critical SA-CONTRIB-2019-020 issue, you can find one here:
https://www.drupal.org/project/link/issues/3034523
Comment #50
nileema.jadhav CreditAttribution: nileema.jadhav as a volunteer and at TATA Consultancy Services for Pfizer, Inc. commentedPatch mentioned #46 worked fine for me.
Comment #51
Pete B CreditAttribution: Pete B at Deeson commentedI just encountered this. I'm surprised it's been working for anyone unless I've missed something.
The first if statement says if it's an internal or external (absolute) link, then it immediately runs valid_url with the flag to say it should be an absolute url. Attached the patch I used which appears to have fixed it for me.
Comment #52
jim22 CreditAttribution: jim22 commentedThanks, Pete B. #51 worked for me.
Comment #53
Michelle#51 worked for me as well, but I think it's not formatted correctly because it asked me what file to patch when I tried to apply it.
Comment #54
cboyden CreditAttribution: cboyden at UC Berkeley Web Platform Services commentedThe patches in #46 and #51 no longer apply. It looks like they are both tackling the same problem, but one does it by reverting the changes to link validation introduced in 1.4, and the other fixes a problem in the logic that determines whether the URL is validated as internal or external.
Here's a re-roll of #46 against the current dev code.
Comment #55
cboyden CreditAttribution: cboyden at UC Berkeley Web Platform Services commentedAnd here's a re-roll of #51.
Comment #56
DamienMcKennaI'd like to request that we first confirm the test coverage covers the scenarios and APIs correctly, and decide from there on the best approach forwards. Thankfully @cboyden has provided test coverage in #54, so we should start there.
Comment #57
cboyden CreditAttribution: cboyden at UC Berkeley Web Platform Services commentedIt's possible that this issue was at least partially fixed by #2428185: Language prefix and relative links broken or another commit that was released in version 1.7. I've spun up a simplytest.me with Link 1.7 and Field Collection 1.1 and no patches. I am finding that relative links and nonexistent/malformed external links can be saved without validation errors in these cases:
What seems to fail validation is anything with a character that is not allowed in a URL, whether the path is relative, leading-slash, or absolute. Everything else sails through without complaint from the validator.
The logic that's changed by the patch in #51/55 seems like it should be causing some problems, but I'm unable to hit the problem in the UI.
More testing is needed for file URLs and nodes with more than one language.
Comment #58
cboyden CreditAttribution: cboyden at UC Berkeley Web Platform Services commentedComment #59
roderikPatch #55 does nothing. After a bit, I saw why: it's a re-roll of #51 but that has been effectively already applied in version 7.x-1.7 / #3041220: False positive in link_validate_url() function with relative URLs with fragments.
So, I guess I'll try to summarize what I see - that would leave for this issue:
I dropped by here with a specific case that could be reproduced / tested / fixed, so opened another related issue: #3268969: Non-standard URL schemes with more than just hostname fail validation.
Comment #60
Kevin.- CreditAttribution: Kevin.- at Synetic commentedRe-rolled patch mentioned in #46 against 7.x-1.11.
Comment #61
Den Tweed CreditAttribution: Den Tweed at Coworks.be commentedFixed patched in #60 as it had mentions of Drupal installation directory structure (web/sites/all/modules/contrib/link/)
Comment #62
DamienMcKenna