Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[5.x] Always append original filenames to Glide URLs #9616

Merged
merged 10 commits into from
Apr 16, 2024

Conversation

duncanmcclean
Copy link
Member

This pull request drops the config option added in #8661 so original filenames are always appended to Glide URLs. The behaviour is no longer optional.

@duncanmcclean
Copy link
Member Author

Marking this as a draft until I can figure out why the tests are failing 🤔

@duncanmcclean duncanmcclean marked this pull request as draft March 1, 2024 12:54
@jasonvarga
Copy link
Member

Also wait until #9610 is done because I bet the merge conflicts will be a pain.

@daun
Copy link
Contributor

daun commented Mar 5, 2024

@duncanmcclean In a review comment on #9610, @ryanmitchell mentioned that the Glide version used by tests is older and doesn't support the getCachePathCallable method. That probably either requires checking if the method exists or upgrading the Glide version used in tests. His suggested code:

$server = $this->server();

if (method_exists($server, 'getCachePathCallable)) {
       $customCallable = $server->getCachePathCallable();
       $server->setCachePathCallable(null);
       $server->deleteCache($pathPrefix.'/'.$asset->path());
       $server->setCachePathCallable($customCallable);
} else {
        $server->deleteCache($pathPrefix.'/'.$asset->path());
}

@duncanmcclean
Copy link
Member Author

Thanks! I'll wait until that pull request has been merged before continuing with this PR.

@jasonvarga
Copy link
Member

A note for when we get around to this...

Whatever ends up getting merged in #9610, we can probably ignore/revert and just bump the min version of Glide to ^2.3 where the getCachePathCallable was introduced. Then we don't have to include conditional logic. We can't do that in that PR since Glide v1 still needs to be supported for Statamic 4.

@robdekort
Copy link
Contributor

I think this PR should also remove the append_original_filename config option from assets.php.

@jasonvarga jasonvarga marked this pull request as ready for review April 16, 2024 18:01
@jasonvarga jasonvarga merged commit 4615e64 into master Apr 16, 2024
31 checks passed
@jasonvarga jasonvarga deleted the fix/always-append-original-filename branch April 16, 2024 18:45
duncanmcclean added a commit to statamic/statamic that referenced this pull request Apr 18, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

None yet

4 participants