Problem/Motivation

When the missing file is requested via a 'non-Clean URL' e.g. /index.php/path/to/asset.jpg, it needs to:

1. Check if the file exists, skipping the need to re-download it when not necessary.
2. Stop an infinite redirect, by ensuring the redirected file path *does not* include the "script" part of the URL. See \Drupal\Core\Utility\UnroutedUrlAssembler::addOptionDefaults()

Steps to reproduce

Request the missing file via the /index.php/ prefix, it results in a loop of it redownloading the same file whether it exists or not.

Proposed resolution

Take into account whether the current request is a clean URL (\Drupal\Core\Routing\RequestHelper::isCleanUrl()) whilst processing the redirect, or always specify the default script in the routing option so the script portion is never included in the final redirect path.

Remaining tasks

Provide issue fork/patch.

Issue fork stage_file_proxy-3521812

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

About issue forks

Comments

codebymikey created an issue. See original summary.

codebymikey opened merge request !95

codebymikey’s picture

codebymikey commented
Status: Active » Needs review
smustgrave’s picture

smustgrave commented
Status: Needs review » Needs work

Can we get a test for this change.

codebymikey’s picture

codebymikey commented
Status: Needs work » Needs review

The initial /index.php tests failed because the gitlab runner runs under a subdirectory whereas my local environment didn't.

It now takes the $base_path into account when making the test requests.

The test covers a lot more test cases, also opted to use a dedicated service for the stage_file_proxy http client rather than the generic one in case the behaviour needs to be altered for whatever reason e.g. for testing or custom implementation.

I'm unable to trigger the test only pipeline for some reason, so a maintainer will need to do that.

smustgrave’s picture

smustgrave commented
Status: Needs review » Fixed
codebymikey’s picture

codebymikey commented
Issue summary: View changes
Status: Fixed » Needs review

I don't think this issue has been fixed and probably needs further review.

The test-only CI pipeline fails to run because it requires the new stage_file_proxy.http_client service to decorate.

smustgrave’s picture

smustgrave commented
Status: Needs review » Needs work

Can we clean up the pipeline

smustgrave’s picture

smustgrave commented
Status: Needs work » Needs review

Actually upstream issue.

So what’s needed here exactly?

smustgrave’s picture

smustgrave commented

Wanted to follow up on this one more time.

codebymikey’s picture

codebymikey commented

Oh sorry, completely missed this.

It was initially marked as Needs Review because it was initially marked as Fixed, but the MR still hasn't been merged yet as far as I could tell, so wanted you to review and make sure the code made sense before merging.

I initially did some work with #3539542: Users should be able to update the Test-only changes file pattern in order to get the test-only changes to work properly since the test needed a dedicated http client service for stage file proxy.

smustgrave’s picture

smustgrave commented
Version: 3.1.x-dev » 4.0.x-dev
Status: Needs review » Fixed

I don't have a lot of time for this module I'm sorry for the delay.

I'm starting a new 4.0.x branch and will be a little "loose" with some early merges. I think merging this now is fine.

Now that this issue is closed, review the contribution record.

As a contributor, attribute any organization that helped you, or if you volunteered your own time.

Maintainers, credit people who helped resolve this issue.

Status: Fixed » Closed (fixed)

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