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

CommentFileSizeAuthor
#61 link-revert_url_validation-2666912-61.patch8.43 KBDen Tweed
#60 link-revert_url_validation-2666912-60.patch8.87 KBKevin.-
#55 link-fix-internal-validation-2666912-55.patch568 bytescboyden
#54 link-revert-url-validation-2666912-54.patch9.42 KBcboyden
#51 link-2666912-51.patch603 bytesPete B
#46 revert-url-validation-2666912-43.patch9.32 KBjamaral86
#42 link-2666912-revert-url-validation-42.patch9.17 KBAlina Basarabeanu
#41 revert-url-validation-2666912-39.patch8.76 KBsjerdo
#40 revert-url-validation-2666912-38.patch8.76 KBjamaral86
#39 revert-url-validation-2666912-37.patch8.76 KBjamaral86
#37 revert-url-validation-2666912-36.patch8.76 KBjamaral86
#36 revert-url-validation-2666912-35.patch9.25 KBjamaral86
#34 diff-33-34.txt49 bytessjerdo
#34 revert-url-validation-2666912-34.patch9.25 KBsjerdo
#33 revert-url-validation-2666912-2.patch9.25 KBjamaral86
#32 link-revert_url_validation-2666912-7.x-1.5.patch9.21 KBdsnopek
#30 link-revert_url_validation-2666912-30.patch10.36 KBidebr
#28 link-revert_url_validation-2666912.patch9.85 KBfskreuz
#27 link-revert_url_validation-2666912.patch9.23 KBfskreuz
#26 link-revert_url_validation-2666912.patch8.03 KBfskreuz
#6 revert-url-validation-2666912.patch10.26 KBmathieuhelie
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

stephmto created an issue. See original summary.

stephmto’s picture

I 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.

nickvanboven’s picture

Wrong related isseu cant delete it

nickvanboven’s picture

mathieuhelie’s picture

Title: Url validation reject valid url values while editing nodes » URL validation rejects existing valid content after upgrade to 7.x-1.4
Priority: Normal » Critical

Releasing 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".

mathieuhelie’s picture

The attached patch reverts the new url validation logic and returns to simply verifying what type of URL has been provided.

mathieuhelie’s picture

Issue summary: View changes
hugronaphor’s picture

Worth 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.

stephmto’s picture

Thank you, I used the patch and it's working now.

Marty2081’s picture

Applying the patch from #6 to Link 7.x-1.4 works.

eelkeblok’s picture

Adding 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.

drupov’s picture

Status: Needs review » Reviewed & tested by the community

Works for me too

aaron.ferris’s picture

Another +1 for the patch in #6.

cozzamara’s picture

Same issue for me. I did not try any patch, and I wait a release updating. Please fix soon, thanks

millerrs’s picture

Can also confirm the patch from #6 works.

dalin’s picture

One 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.

mallezie’s picture

Just 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.

Renrhaf’s picture

Confirming 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.'

fengtan’s picture

We 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.

hugronaphor’s picture

This 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!?

pefferen’s picture

As 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.

jschrab’s picture

What 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.

pelicani’s picture

We 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

pifagor’s picture

Status: Reviewed & tested by the community » Needs work

Dear community. Please check again patch. Thank you.

John Cook’s picture

While 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.

fskreuz’s picture

Rewrote patch the best I could against dev.

fskreuz’s picture

Ugh, I hate it when IDEs generate patches. Ignore #26. Updated patch.

fskreuz’s picture

maticb’s picture

The patch in #28 works on 7.15, but fails to patch the link.validate.test

idebr’s picture

Version: 7.x-1.4 » 7.x-1.x-dev
Status: Needs work » Needs review
FileSize
10.36 KB

Rerolled #28 against 7.x-1.x

Status: Needs review » Needs work

The last submitted patch, 30: link-revert_url_validation-2666912-30.patch, failed testing. View results

dsnopek’s picture

Here's #30 re-rolled against 7.x-1.5, so that it can be used in Drush .make files.

jamaral86’s picture

Ported to work with the latest release per PSA-2019-02-19

sjerdo’s picture

Status: Needs work » Needs review
FileSize
9.25 KB
49 bytes

Fixed missing comma in test.

Status: Needs review » Needs work

The last submitted patch, 34: revert-url-validation-2666912-34.patch, failed testing. View results

jamaral86’s picture

Ported to work with the latest release per PSA-2019-02-19

(missing last commit) from 1.x

jamaral86’s picture

Ported 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

sjerdo’s picture

The tests are still broken.

jamaral86’s picture

fixed lint error

jamaral86’s picture

sjerdo’s picture

Alina Basarabeanu’s picture

thetoast’s picture

#42 patch worked for me. Thanks for posting alinaBasa.

alesr’s picture

Just 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?

thetoast’s picture

Well, 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?

jamaral86’s picture

scottalan’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 46: revert-url-validation-2666912-43.patch, failed testing. View results

coconnor’s picture

Note 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

nileema.jadhav’s picture

Patch mentioned #46 worked fine for me.

Pete B’s picture

I 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.

function link_validate_url($text, $langcode = NULL) {

  $text = _link_clean_relative($text);
  $text = link_cleanup_url($text);
  $type = link_url_type($text);

  if ($type && ($type == LINK_INTERNAL || $type == LINK_EXTERNAL)) {
    $flag = valid_url($text, TRUE);
    if (!$flag) {
      $normal_path = drupal_get_normal_path($text, $langcode);
      $parsed_link = parse_url($normal_path, PHP_URL_PATH);
      if ($normal_path != $parsed_link) {
        $normal_path = $parsed_link;
      }
      $flag = drupal_valid_path($normal_path);
    }
    if (!$flag) {
      $flag = file_exists($normal_path);
    }
    if (!$flag) {
      $uri = file_build_uri($normal_path);
      $flag = file_exists($uri);
    }
  }
  else {
    $flag = (bool) $type;
  }

  return $flag;
}
jim22’s picture

Thanks, Pete B. #51 worked for me.

Michelle’s picture

#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.

cboyden’s picture

Status: Needs work » Needs review
FileSize
9.42 KB

The 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.

cboyden’s picture

And here's a re-roll of #51.

DamienMcKenna’s picture

Status: Needs review » Needs work
Issue tags: -failed url validation +Needs tests

I'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.

cboyden’s picture

It'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:

  • With validation turned on for all Link fields
  • With the "absolute" flag on or off for the field (affects how the URL is output)
  • Using either Field Collection or a link field directly on the node
  • Relative path alias with leading slash
  • Relative path alias without leading slash
  • Relative node/nid path with leading slash
  • Relative node/nid path without leading slash
  • Relative path alias to nonexistent (not served by Drupal) path
  • Absolute URL with invalid external TLD
  • Absolute URL of nonexistent external site
  • Absolute URL malformed (all legal characters, but in the wrong places)

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.

cboyden’s picture

roderik’s picture

Patch #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:

  • A problem description that is not clear to me (should the link field even support "public://" URLs? How does that work exactly?)
  • The patch in #54 to roll things back to the 7.x-1.3 situation (which does not apply to 7.x-1.9 anymore, and I don't know how feasible this is by now)
  • A list in #56 which can be useful to convert into tests in LinkValidationApiTest, to see what is still breaking.

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.

Kevin.-’s picture

Re-rolled patch mentioned in #46 against 7.x-1.11.

Den Tweed’s picture

Fixed patched in #60 as it had mentions of Drupal installation directory structure (web/sites/all/modules/contrib/link/)

DamienMcKenna’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 61: link-revert_url_validation-2666912-61.patch, failed testing. View results