Problem/Motivation

If a file url has query parameters, or a hash, added to it. The file-link component strips these out.

Drupal would not normally use query parameters on a file url. But in our case they are being added in by a stream wrapper, from the flysystem_s3 module (we are using them to pass through a url signature, to allow the user access to the file).

Query parameters could also be added in by hook_file_url_alter(), these would again be stripped out.
The core public file stream wrapper url encodes any query parameters added by hook_file_url_alter(). So these the are retained.

The issue is caused by the new file handling in template_preprocess_file_link(), which used to create the Url object itself, but now uses the new file_url_generator service. Since this changed was introduced, the $options is being overwritten by $url->setOptions($options)

Steps to reproduce

Proposed resolution

Use
$url->mergeOptions($options)
Instead of
$url->setOptions($options)

Remaining tasks

User interface changes

API changes

Data model changes

Release notes snippet

CommentFileSizeAuthor
#14 3254727-file-links-with-query-params-14.patch1.85 KBleon kessler
#10 interdiff_9-10.txt1.67 KBranjith_kumar_k_u
#10 3254727-10.patch10.6 KBranjith_kumar_k_u
#9 3254727-file-links-with-query-params-with-3254553-9.patch10.62 KBleon kessler
#6 3254553-7-3254727-6-interdiff.txt1.6 KBleon kessler
#6 3254727-file-links-with-query-params-with-3254553-6.patch10.96 KBleon kessler
#5 3254727-file-links-with-query-params-5-TEST-ONLY-FAIL.patch4.65 KBleon kessler
#4 3254727-file-links-with-query-params-4-TEST-ONLY-FAIL.patch4.84 KBleon kessler
#2 3254727-file-links-with-query-parameters-2.patch591 bytesleon kessler

Comments

Leon Kessler created an issue. See original summary.

leon kessler’s picture

leon kessler commented
StatusFileSize
new 3254727-file-links-with-query-parameters-2.patch591 bytes

Adding patch that switches to using $url->mergeOptions($options)

Looking at writing a test for this, adding something to FileFieldWidgetTest.php and then using hook_file_alter() to add in the query parameter.

leon kessler’s picture

leon kessler commented
Issue summary: View changes

Does not appear to be an issue for hook_file_url_alter() with the default public stream wrapper. As this url encodes query parameters (before they are parsed FileUrlGenerator::generate).

So this issue is more specific to third party stream wrappers, and really it's only relevant for externally hosted files (e.g. on S3/CDN, etc). As Drupal would not be using query parameters on files anyway.

leon kessler’s picture

leon kessler commented
Parent issue: » #3254553: FileUrlGenerator::generate() does not work with externally hosted files using stream wrappers
StatusFileSize
new 3254727-file-links-with-query-params-4-TEST-ONLY-FAIL.patch4.84 KB

I've worked up a test to demo the issue. This depends on the dummy-external-readonly stream wrapper from #3254553: FileUrlGenerator::generate() does not work with externally hosted files using stream wrappers (so I've now made this a child issue).

It's admittedly a bit of an edge case, so was quite difficult to find a nice place for the test to live.

leon kessler’s picture

leon kessler commented
StatusFileSize
new 3254727-file-links-with-query-params-5-TEST-ONLY-FAIL.patch4.65 KB

Previous patch was in the wrong file. New patch should run correctly and produce 1 failure.

leon kessler’s picture

leon kessler commented
Status: Active » Needs review
StatusFileSize
new 3254727-file-links-with-query-params-with-3254553-6.patch10.96 KB
new 3254553-7-3254727-6-interdiff.txt1.6 KB

Here's a patch with the tests + the fix. This should pass.

It's also dependent on the fix from the parent issue (otherwise the query string gets url encoded). So this is also combined into the patch to allow it to pass. Have created an interdiff to show the changes related to this issue.

Status: Needs review » Needs work

The last submitted patch, 6: 3254727-file-links-with-query-params-with-3254553-6.patch, failed testing. View results

andileco’s picture

andileco commented

#6 worked for me to fix this issue: https://www.drupal.org/project/s3fs/issues/3254464

Thank you for the patch!

leon kessler’s picture

leon kessler commented
Status: Needs work » Needs review
StatusFileSize
new 3254727-file-links-with-query-params-with-3254553-9.patch10.62 KB

Re-rolling the combined patch with updates from parent issue.

The interdiff from my comment on #6 (between this patch and the parent patch) is the same).

ranjith_kumar_k_u’s picture

ranjith_kumar_k_u commented
StatusFileSize
new 3254727-10.patch10.6 KB
new interdiff_9-10.txt1.67 KB
rokzabukovec’s picture

rokzabukovec commented

The patch from #10 fixed the malformed URL issue only when uploading a new media item/file. When I try to view the node the error is back. I'm on Drupal core 9.3.2.

leon kessler’s picture

leon kessler commented

@rokzabukovec This isn't happening for me, the file url on the "Generic file" formatter has the correct url. Possibly you are using another formatter or theme function?

cmlara’s picture

cmlara
he/him
commented
Status: Needs review » Reviewed & tested by the community

Tested against s3fs with presign url's which would generate option parameters and it solved the faults. Unable to duplicate concerns from #11, I also suspect it is another formatter or theme function.

RTBC #10 so long as the parent issue remains at patch #20.
May need a trivial rebase after the parent is committed.

leon kessler’s picture

leon kessler commented
StatusFileSize
new 3254727-file-links-with-query-params-14.patch1.85 KB

Now that the parent issue has been pushed to 9.3.x, here's a re-roll of the patch, with just the changes for this issue.

Not sure if this needs to go back to "needs review"? There are no changes since the last patch.

cmlara’s picture

cmlara
he/him
commented

I’m still good with RTBC from me.

Same patch with expected patent issue code removed now that it was committed.

cmlara’s picture

cmlara
he/him
commented
Priority: Normal » Major

Moving to major on the grounds it interferes with a normal site visitors’ use of the site.

Use cases where this is an issue can cause inability to access content (s3 presigned urls.)

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 14: 3254727-file-links-with-query-params-14.patch, failed testing. View results

cmlara’s picture

cmlara
he/him
commented
Status: Needs work » Reviewed & tested by the community

I don't see how this patch could be at fault for this.

#2829040-142: [meta] Known intermittent, random, and environment-specific test failures also reports random issues around this test.

Back to RTBC as I believe this is a random failure.

  • catch committed d60ba4d on 10.0.x 
    Issue #3254727 by Leon Kessler, ranjith_kumar_k_u, cmlara, andileco:...

  • catch committed 6db6dca on 9.4.x 
    Issue #3254727 by Leon Kessler, ranjith_kumar_k_u, cmlara, andileco:...

  • catch committed 83cb0c8 on 9.3.x 
    Issue #3254727 by Leon Kessler, ranjith_kumar_k_u, cmlara, andileco:...
catch’s picture

catch
he/him
Primary language English
commented
Status: Reviewed & tested by the community » Fixed

Committed/pushed to 10.0.x, cherry-picked to 9.4.x and 9.3.x, thanks!

Status: Fixed » Closed (fixed)

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