FileUrlGenerator::transformRelative() edge case errors

This issue is being reviewed by the kind folks in Slack, #needs-review-queue-initiative. We are working to keep the size of Needs Review queue [2700+ issues] to around 400 (1 month or less), following Review a patch or merge request → as a guide.

This could use an issue summary update for the proposed solution, specifically what is it? You listed edge cases but not how it was being solved.

Thank you for providing feedback. That sounds like a very nice initiative.

I have extended the description with a bit more info on what the patch does (basically just hardening a regular expression and adding more test data covering the discovered edge cases).

I also made sure the patch still applies cleanly to the 10.1.x branch, and has just scheduled a new test run against 10.1.x as well.

Updated the issue summary more with the default template

Changes look good though and have additional test coverage.

The last submitted patch, 15: drupal-file_url_transform_relative_edge_case_errors-2983058-15.patch, failed testing. View results →

Created an issue fork.

Looking great, very comprehensive

I think we need use preg_quote on the base path too - see MR for explanation.

Thanks again.

Thank you for your feedback @larowlan → .

You are quite right. We need to preg_quote the base path as well.

I went even further and also added preq_quote to the port. Although we would expect a number, Request::getPort() could return a string (we would still expect the string to hold a number, but this issue was created based on weird input, so I think it is better to be safe).

I added a test using a path with "weird" characters to make sure the quoting is in place.

Can see the threads in the MR have been addressed.

One more comment on the MR, I think we're missing one test case for when the URL ends in a /

I have added a test case with a trailing slash on the hostname, @larowlan → .

Additional test appeared to be added.

I ran the test through locally with the debugger attached, and none of the cases had a value for $base_path, in every instance it was an empty string.

In this comment on the MR I asked for some changes to base path and test coverage for it.

I suspect we will need to pass in an additional parameter to set a base path.

Ah, now I understand, @larowlan → . I misread your remark regarding base path as just the path... Don't why. Re-reading it now, and it's quite obvious what you meant. I'll look into it. It might some days before I can find the time for it, though.

Ok, took a stab at it now. Tests work on my machine now, but it's way past my bedtime, so I'll have to wait until tomorrow morning to see if Drupal CI agrees, and I can put it into Needs review again.

Thank you once again for the valuable feedback, @larowlan → .

Let me know if this was a wrong test.

Drupal standard install of 10.1
Added breakpoint to transformRelative on the last line
Added an image to the article field.
Breakpoint stops but base_path is always empty.

There additional steps I should take?

The base path only has a non-empty value if you install Drupal in a sub-folder.

That is the the difference between where the Drupal root/index.php is located: https://example.com vs https://example.com/here/is/drupal

Restoring status

Was able to circle back on this. Relooking at the change think all the points have been addressed.

Needs a rebase on 11.x

Wonder why the review bot didn’t catch it

Greetings from DrupalCon Lille 2023!

I have rebased the merge request in 11.x.

All tests are still green.

I'm not sure whether I'm allowed to put the status back to Reviewed & tested by the community myself. So I'm putting on Needs review instead.

11.x rebase seems good.

larowlan → changed the visibility of the branch 10.1.x to hidden.

larowlan → changed the visibility of the branch 2983058-fileurlgeneratortransformrelative-edge-case to hidden.

larowlan → changed the visibility of the branch 11.0.x to hidden.

I ran the tests locally again with the debugger and confirmed that the base path is now being correctly set.

I ran them without the fix, and confirmed we're getting fails.

Took another deep review of it and went over it with a fine tooth comb locally just to make sure I wasn't missing anything.

I think this is good to go, its been a while from when I last looked at this, thanks for keeping it fresh @arnested

Committed to 11.x

Backported to 10.2.x

larowlan → closed merge request !5045

🤔 This likely explains some of the bug reports I've been getting in the CDN module! 🤓😄

See 💬 CDN cannot find the image style images Postponed: needs info and especially 🐛 [upstream] Double slash in URL since upgrade to Drupal 10 Postponed: needs info . I've asked both to manually test again with a core release containing this or by applying this patch.

IMHO this deserves a backport to Drupal 10.1?

Thank you, larowlan → . 🎉

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