Afraid I can't see that behaviour (404s until cache clear) here in my local testing, what's your config for stage_file_proxy.settings? The other possibility if it works after cache clears/disabling markup cache is that somehow the image token "itok" values are invalid on first load? If that's the case then the image style controller would return a 404 error that is intended to be cached.
I've done some work to fix up the tests, added an additional test for multilingual and content moderation together. I also think I've figured out a slightly better way of parsing the URL parameters when the access check is being run before the route object is available. I ended up refactoring some test case stuff into a trait to remove some of the duplication, hopefully that's OK.
Also while writing the tests, I found an additional issue where cache tags for the preview link entity weren't being added to the access result for the preview link route. This meant the page cache could potentially cache the preview page and not be invalidated when regenerating the access token.
It would be great to get a review on this MR now:
https://git.drupalcode.org/project/preview_link/-/merge_requests/27
So I've been testing this out with preview links using url based language-negotiation (so the preview links are generated with the language prefix).
This is the behaviour I can see with the current version of the module:
- If all translations are unpublished, preview link for the default language works, preview link for other unpublished translations return a 404.
- If the default language is published, with a draft translation, the preview link with a language prefix shows the published translation.
- If the default language is published and has a newer draft, and the translation is published with a newer draft, the preview link for the default language works correctly, and the preview link with a language prefix continues to show the published translation
I tried the core patch linked in #49 but it didn't appear to change the behaviour at all.
I then applied the changes from MR 27 and saw the following behaviour:
- If all translations are unpublished, preview link for the default language works, preview link for an additional draft translation displays correctly.
- If the default language is published, with a draft translation, the preview link with a language prefix shows the correct draft translation.
- If the default language is published and has a newer draft, and the translation is published with a newer draft, both preview links work correctly and show the correct drafts, and the language switcher block works correctly.
So the changes seem to work well in my local testing.
I'm not sure if there's a better way to grab out the URL parameters than directly parsing the path info in the new code, but I can see why it's required as the access check is being invoked as part of the parameter upconversion process so the route match isn't available yet.
Created an MR with the page cache kill switch, works in my local testing to prevent the infinite redirects on image style derivative URLs.
We're seeing what I think is it the same issue of an infinite redirect loop when using the non-hotlinking mode and accessing image style derivatives, I believe the issue is caused by this change (and requires having the page_cache module enabled):
https://www.drupal.org/project/stage_file_proxy/issues/3396634 🐛 CORS headers aren't applied to the module response Fixed
After StageFileProxySubscriber has fetched the file, it returns a RedirectResponse (to the same URL as the current request) which shouldn't be cacheable. However \Drupal\Core\EventSubscriber\RedirectResponseSubscriber has logic that converts it into a LocalRedirectResponse which is cacheable. So the redirect to the same URL gets cached by page_cache and the browser gets into an infinite loop.
I think this is only an issue for image style derivatives as for other public files, once the file is downloaded the web server would intercept the next request so it wouldn't get to drupal to hit the page cache.
Using the page_cache kill switch seems to resolve the issue for me locally. I'll create an issue fork with the fix.
I'm wondering if this is only showing up now because of this change in 10.1 (AJAX views using GET by default)?
https://www.drupal.org/node/3193798 →
Previously the AJAX form would have been POSTed which would have skipped the render cache entirely?
https://git.drupalcode.org/project/drupal/-/blob/11.x/core/lib/Drupal/Co...
We're seeing the same issue here, it doesn't appear to be related to Redis, I can reproduce it as long as the render cache is enabled.
After some debugging it looks like the issue is that \Drupal\media_library\Plugin\views\field\MediaLibrarySelectForm renders a placeholder with the row index (e.g. <!--form-item-media_library_select_form--0-->), which is then replaced later with the actual checkbox.
Problem is when the render cache is used the placeholder gets cached and then when you add a new item that matches the filter you end up with multiple items with <!--form-item-media_library_select_form--0--> and then all the following row indexes are still the old cached ones and are incorrect, leading to the code inserting the wrong item.
Setting the cache plugin to None on the view does seem to fix the issue, but the core configuration for the media library view is set to tag-based caching so I'm not sure if this combination is expected to work? Other option would be to disable caching in the MediaLibrarySelectForm, as it seems like the approach taken here is incompatible with the render cache if it relies on printing a placeholder with the row index.
No worries, happy to try the simpler alternative. I've added an issue fork with a hook to update the access time when a user is unblocked and added some extra testing to verify the logic.
Thanks
Was this issue intended to be set to fixed, I can't see any associated code changes?
The previous patch no longer applies to 2.4, attached a re-rolled one.
Updated issue summary and reproducing steps for D9/10 with content moderation.
I've based my description of the proposed solution on the patch in #140, hopefully I understood that correctly.