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

Comments

pwolanin’s picture

pwolanin commented
Priority: Critical » Major
Issue summary: View changes
Status: Active » Needs review
Issue tags: +Needs tests
StatusFileSize
new 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 commented
StatusFileSize
new increment.txt3.46 KB
new 2265913-test-only-2.patch2.3 KB
new 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
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 commented
Issue tags: -Needs tests
StatusFileSize
new increment.txt1.86 KB
new 2265913-4.patch3.19 KB

Sure.

pwolanin’s picture

pwolanin commented
StatusFileSize
new increment.txt717 bytes
new 2265913-5.patch3.2 KB

small wording fix.

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

pwolanin’s picture

pwolanin commented
StatusFileSize
new increment.txt1.47 KB
new 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
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 commented
Issue summary: View changes
webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
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
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 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.