Problem/Motivation

Currently the IMCE UI makes assumptions about URL generation instead of using existing core API to generate URLs for each file.

Currently IMCE attempts to find a root_url from output of FileUrlGenerator::generateAbsoluteString() and use this URL to generate all additional URLs in the UI by appending the file location onto the end of this root_url.

This assumption works for a stock Drupal Core with no customization however it is not accurate once the full core API is utilized.

Sites that implement a hook_file_url_alter() can be negatively impacted by this method of generating links.

Addtionaly in the case of the s3fs module our getExternalUrl() can return a URL that differs for each request and each path based on the configuration options utilized. Taking the URL for "s3://something/" and assuming "s3://something/test1.txt" only needs "test1.txt" appended is not always accurate.

Steps to reproduce

See attached test-only patch. While this tests with a url_alter this is also fundamentally testing the ability to work with contrib streamWrappers such as s3fs by ensuring that the link generated in the UI matches the output from FileUrlGenerator::generateAbsoluteString() for each uri.

Proposed resolution

Call FileUrlGenerator::generateAbsoluteString() for each that a URL is requested for, either as part of providing the directory listing or preferably when a user clicks on a file to reduce bandwidth and allow for time sensitive links.

Remaining tasks

Create fix patch

User interface changes

None

API changes

TBD

Data model changes

TBD

CommentFileSizeAuthor
#4 interdiff-2-4.txt817 bytescmlara
#4 3320228-invalid_link_generation-4-test-only.patch4.41 KBcmlara
#2 3320228-invalid_link_generation-2-test-only.patch4.53 KBcmlara

Comments

cmlara created an issue. See original summary.

cmlara’s picture

cmlara
he/him
commented
StatusFileSize
new 3320228-invalid_link_generation-2-test-only.patch4.53 KB

Attaching test-only patch.

This was written against 3.0.1 however since there is no 3.x branch to test with I'm queuing it against the 2.x branch for testing in hopes it also applies cleanly against the 2.x branch.

cmlara’s picture

cmlara
he/him
commented

Patch applied cleanly to 2.x however its failing on two tests that pass locally.

2) Drupal\Tests\imce\FunctionalJavascript\LinkGenerationTest::testPreviewLinkGeneration with data set "private" ('private', null)
Failed asserting that two strings are equal.

--- Expected
+++ Actual
@@ @@
-'http://php-apache-jenkins-drupal8-contrib-patches-131096/subdirectory/system/files/test.txt'
+'http://php-apache-jenkins-drupal8-contrib-patches-131096/subdirectory/subdirectory/system/files/test.txt'

I suspect this may indicate an additional issue in URL generation with the IMCE module when the site is in a sub directory since we see /subdirectory/subdirectory/ however I have not confirmed that is the case.

If we ignore the 'subdirectory/subdirectory' for a moment we can see where test.txt is appended to the end of the base path for 'public://.' when an alter is in place demonstrating how the the current code can fail.

-'http://php-apache-jenkins-drupal8-contrib-patches-131096/subdirectory/sites/simpletest/73575083/files/test.txt?md5=dd18bf3a8e0a2a3e53e2661c7fb53534'
+'http://php-apache-jenkins-drupal8-contrib-patches-131096/subdirectory/subdirectory/sites/simpletest/73575083/files/.?md5=5058f1af8388633f609cadb75a75dc9d/test.txt'

files/.?md5=5058f1af8388633f609cadb75a75dc9d/test.txt being the returned url for "public://test.txt" when "public://." (which is the root_url source) is altered to include an MD5sum at the end.

cmlara’s picture

cmlara
he/him
commented
StatusFileSize
new 3320228-invalid_link_generation-4-test-only.patch4.41 KB
new interdiff-2-4.txt817 bytes

Interesting, the 'subdirectoyr/subdirectory' issue appears to be related to the fact I used URL::fromRoute() to try and generate the request path to drupalGet().

Reworked the test from #2 to use a 'simpler' call for drupalGet().

  • ufku committed be304819 on 3.x 
    Issue #3320228: Added an option to fix file URLs for s3
ufku’s picture

ufku commented
Version: 3.0.1 » 3.x-dev
Status: Active » Fixed

Added "Enable URL altering" option under advanced settings.

Status: Fixed » Closed (fixed)

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