- 🇧🇪Belgium dieterholvoet Brussels
The last two patches (#22, #23) seem to have ignored the work that was done before, including tests. I'll create a MR based on the patch in #18 and rebase it on 2.0.x-dev.
- @dieterholvoet opened merge request.
- 🇧🇪Belgium dieterholvoet Brussels
The approach in the previous patches (replacing the entity with its translation in
PreviewLinkController::resolveEntity
doesn't seem to fix the issue for me. It doesn't seem to change anything really, so I'm going to revert that until we're sure it's actually necessary. What does work is the approach in #3269474: Preview Link not working for translations (403) → . I rebased that patch and applied it here and that does seem to fix the issue. - Status changed to Needs review
almost 2 years ago 11:00am 26 January 2023 - Status changed to Needs work
almost 2 years ago 1:07pm 30 January 2023 - 🇧🇪Belgium dieterholvoet Brussels
The current state of the MR causes 🐛 Media library does not open when clicking "Add media": “TypeError: Argument 2 passed to Drupal\media_library\MediaLibraryState::create() must be of the type array, null given” Needs work , so setting to Needs work.
- Status changed to Needs review
almost 2 years ago 1:18pm 30 January 2023 - 🇧🇪Belgium dieterholvoet Brussels
I discovered the root cause: the call to
\Drupal::service('router.route_provider')->getRouteCollectionForRequest($request)
. That method modifies the passed request object, which happens to be the currently active request. We don't want that to happen in a simple access check, passing a clone to that method fixed the issue. - 🇧🇪Belgium dieterholvoet Brussels
Fixed an issue where preview links were still broken for untranslated entities.
- Merge request !17Issue #3124785: Preview Link does not work with multilingual site → (Open) created by les lim
- Open on Drupal.org →Core: 9.5.5 + Environment: PHP 8.0 & MySQL 5.7 updated depslast update
over 1 year ago Not currently mergeable. - 🇺🇸United States les lim
MR #16 doesn't apply cleanly anymore, since the entity hook logic has been moved to a service class in both 2.x branches. Opened MR #17 as a reroll with minor refactoring for dependency injection.
- 🇺🇸United States les lim
Shoot, MR #17 was intended to open against 2.1.x, but it's trying to merge into 2.0.x and I can't figure out how to change the MR or create a new one. What am I missing?
- 🇧🇪Belgium dieterholvoet Brussels
On the MR page you can click Edit in the top right corner and change the base branch by clicking on the dropdown next to the 'From X into Y' sentence right underneath the page title.
- last update
over 1 year ago run-tests.sh fatal error - last update
over 1 year ago run-tests.sh fatal error - First commit to issue fork.
- last update
about 1 year ago run-tests.sh fatal error - miiimooo Europe
After upgrading to Drupal 10 with the patch from MR17 I got a deprecation notice for the Drupal\Core\Http\RequestStack
I've added a commit 54abd2 that makes the patch Drupal 10 compatible in MR17
- Status changed to Needs work
about 1 year ago 10:20pm 16 October 2023 - 🇨🇦Canada joseph.olstad
FYI: There's a working patch for core that deals with this issue.
🐛 node preview translation needs to get translation of entity if it hasTranslation Needs work
- 🇺🇸United States les lim
One problem with the previous MR is the
getRouteCollectionForRequest()
call, which calls out to cache. ThePreviewLinkEntityHooks
class that contains it is not a service; it is instantiated new for every entity access check, for which there could be dozens on any given page. Our call to get the current route collection thus gets called new every time, even though the result is the same each time.Converting the class into a service saves memory and allows us to statically cache the
getRouteCollectionForRequest()
result, cutting down substantially on queries. - miiimooo Europe
miiimooo → changed the visibility of the branch preview_link-3124785-reroll-68f89a1 to hidden.
- Merge request !27Issue #3124785: Preview Link does not work with multilingual site → (Open) created by miiimooo
- 🇨🇦Canada joseph.olstad
There's a core patch available and it should resolve the issue.
🐛 node preview translation needs to get translation of entity if it hasTranslation Needs work - Status changed to Postponed: needs info
8 months ago 2:34am 9 April 2024 - 🇦🇺Australia richard.thomas
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 translationI 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.
- Status changed to Needs review
7 months ago 2:42am 17 May 2024 - 🇦🇺Australia richard.thomas
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
- 🇵🇭Philippines johnrosswvsu
Please add patch for 2.2.x or 2.2.0-alpha1. Thanks.
- miiimooo Europe
The merge request does no longer work as patch, so I'm providing a simple patch here. Patch applies with 2.2.0-alpha1
- 🇦🇺Australia richard.thomas