Problem/Motivation

If you create a Drupal\Core\Url object using an external URL (e.g. 'http://drupal.org') and pass this into \Drupal\Core\Utility\LinkGenerator::generateFromUrl() it can cause an uncaught exception because $url->getInternalPath() is called when $options['set_active_class'] is not empty.

Proposed resolution

don't call $url->getInternalPath() for an external Url, and add a test case.

CommentFileSizeAuthor
#7 2265913-7.patch2.5 KBpwolanin
#7 increment.txt1.47 KBpwolanin
#5 2265913-5.patch3.2 KBpwolanin
#5 increment.txt717 bytespwolanin
#4 2265913-4.patch3.19 KBpwolanin
#4 increment.txt1.86 KBpwolanin
#2 2265913-2.patch2.86 KBpwolanin
#2 2265913-test-only-2.patch2.3 KBpwolanin
#2 increment.txt3.46 KBpwolanin
#1 2265913-1.patch858 bytespwolanin
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

pwolanin’s picture

pwolanin CreditAttribution: pwolanin commented
Priority: Critical » Major
Issue summary: View changes
Status: Active » Needs review
Issue tags: +Needs tests
FileSize
2265913-1.patch858 bytes

Here's the fix, still needs an added test case.

downgrading to major, since I'm not sure this is called this way currently in core, but the code is clearly broken and caught me up in a patch.

pwolanin’s picture

pwolanin CreditAttribution: pwolanin commented
FileSize
increment.txt3.46 KB
2265913-test-only-2.patch2.3 KB
2265913-2.patch2.86 KB

Here's the test alone (should have 1 fail), updated patch, and increment.

Also adds a small optimization to the Url class so we don't check again that the url is external.

not that I moved the $url->isExternal() check up a little since we shouldn't need any of the logic related to the active class for an external link.

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
CreditAttribution: tim.plunkett commented 
+++ b/core/tests/Drupal/Tests/Core/Utility/LinkGeneratorTest.php
@@ -131,7 +131,7 @@ public function testGenerateFromUrl() {
-    $this->moduleHandler->expects($this->once())
+    $this->moduleHandler->expects($this->exactly(2))

This is a sign you should have a new test method. Can you just put that new test in public function testGenerateFromUrlExternal()?

pwolanin’s picture

pwolanin CreditAttribution: pwolanin commented
Issue tags: -Needs tests
FileSize
increment.txt1.86 KB
2265913-4.patch3.19 KB

Sure.

pwolanin’s picture

pwolanin CreditAttribution: pwolanin commented
FileSize
increment.txt717 bytes
2265913-5.patch3.2 KB

small wording fix.

The last submitted patch, 2: 2265913-test-only-2.patch, failed testing.

pwolanin’s picture

pwolanin CreditAttribution: pwolanin commented
FileSize
increment.txt1.47 KB
2265913-7.patch2.5 KB

tim.plunkett suggests passing the external flag to the UrlGenerator should be put in a separate issue.

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
CreditAttribution: tim.plunkett commented
Title: linkGenerator::generateFromUrl() fatal (uncaught exception) with external url » LinkGenerator::generateFromUrl() fatal (uncaught exception) with external URL
Status: Needs review » Reviewed & tested by the community

Excellent, thanks

pwolanin’s picture

pwolanin CreditAttribution: pwolanin commented
Issue summary: View changes
webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
CreditAttribution: webchick commented
Status: Reviewed & tested by the community » Fixed

Committed and pushed to 8.x. Thanks!

  • Commit d309714 on 8.x by webchick: 
    Issue #2265913 by pwolanin: LinkGenerator::generateFromUrl() fatal (...
catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch commented

tim.plunkett suggests passing the external flag to the UrlGenerator should be put in a separate issue.

Does that issue exist?

pwolanin’s picture

pwolanin CreditAttribution: pwolanin commented

Here's the follow-up with initlal patch: #2266377: Speed up UrlGenerator a little by setting the 'external' option in the Url object

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.