Problem/Motivation

Logic error: In Drupal\Component\Utility\Html, the transformRootRelativeUrlsToAbsolute() function tries to operate on any srcset attributes in its input every time it checks for every other attribute.

Proposed resolution

Move foreach ($xpath->query("//*[@srcset]") outside the foreach (static::$uriAttributes as $attr) loop.

CommentFileSizeAuthor
#2 core-transform_rel_to_absolute_srcset-3360220-2.patch2.46 KBjeffschuler

Comments

jeffschuler created an issue. See original summary.

jeffschuler’s picture

jeffschuler
Location Boulder, Colorado
commented
Status: Active » Needs review
StatusFileSize
new core-transform_rel_to_absolute_srcset-3360220-2.patch2.46 KB
borisson_’s picture

borisson_
Primary language Dutch
Location Mechelen, 🇧🇪
commented
Status: Needs review » Needs work
Issue tags: +Needs tests

Can we add tests to prove the bug exists and that this fixes it?

jeffschuler’s picture

jeffschuler
Location Boulder, Colorado
commented
Status: Needs work » Needs review

@borisson_: I understand why you're requesting this but I don't believe it's a bug that deserves tests and it doesn't result in incorrect output. It's just a logical error that results in unnecessary over-processing.

This:

foreach (static::$uriAttributes as $attr) {

loops over each of the attributes in this array:

protected static $uriAttributes = ['href', 'poster', 'src', 'cite', 'data', 'action', 'formaction', 'srcset', 'about'];

For each of these attributes the current code is looking for all instances of that attribute, and then also looking for all instances of srcset, and operating on the value of each srcset it finds.

So ... for every attribute ... we'll parse and operate on every srcset needlessly. We should only parse for and operate on each srcset once.

smustgrave’s picture

smustgrave commented
Status: Needs review » Reviewed & tested by the community

Refactor makes sense. This could probably be considered a task as it's an improvement over a bug. Will see what the committers think!

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 2: core-transform_rel_to_absolute_srcset-3360220-2.patch, failed testing. View results

borisson_’s picture

borisson_
Primary language Dutch
Location Mechelen, 🇧🇪
commented
Status: Needs work » Reviewed & tested by the community
Issue tags: -Needs tests

Looks like that is a random fail, back to rtbc per #6. I'm convinced by #4 that we don't need tests for this particular one.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 2: core-transform_rel_to_absolute_srcset-3360220-2.patch, failed testing. View results

smustgrave’s picture

smustgrave commented
Status: Needs work » Reviewed & tested by the community

Think another random failure

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 2: core-transform_rel_to_absolute_srcset-3360220-2.patch, failed testing. View results

borisson_’s picture

borisson_
Primary language Dutch
Location Mechelen, 🇧🇪
commented
Status: Needs work » Reviewed & tested by the community

Another random fail

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 2: core-transform_rel_to_absolute_srcset-3360220-2.patch, failed testing. View results

borisson_’s picture

borisson_
Primary language Dutch
Location Mechelen, 🇧🇪
commented
Status: Needs work » Reviewed & tested by the community

back to rtbc, another random fail.

lauriii’s picture

lauriii
he/him
Primary language Finnish
Location Finland
commented
Status: Reviewed & tested by the community » Fixed

Committed 98df2c9 and pushed to 11.x. Thanks!

  • lauriii committed 98df2c98 on 11.x 
    Issue #3360220 by jeffschuler: Logic error in Html::...
wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Related issues: +#88183: Relative URLs in feeds should be converted to absolute ones

Oops! I introduced this bug ~7 years ago in #88183: Relative URLs in feeds should be converted to absolute ones 😇 Thanks, @jeffschuler!

jeffschuler’s picture

jeffschuler
Location Boulder, Colorado
commented

@Wim Leers your amount of productive output deserves a few mistakes. At least it wasn't causing any problems!

Thanks all for seeing this through.

Status: Fixed » Closed (fixed)

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