Preview Link does not work with multilingual site

Created on 3 April 2020, over 4 years ago
Updated 26 January 2023, almost 2 years ago

Hi,

I have a multilingual site(English and Japanese). When i create a content and both translations are unpublished. So if i create a preview link from either of the languages it still shows the content in the default translation, not in the translation where the preview link is generated.

🐛 Bug report
Status

Needs work

Version

2.0

Component

Code

Created by

🇮🇳India Chirag_Garg

Live updates comments and jobs are added and updated live.
  • Needs tests

    The change is currently missing an automated test that fails when run with the original code, and succeeds when the bug has been fixed.

Sign in to follow issues

Merge Requests

Comments & Activities

Not all content is available!

It's likely this issue predates Contrib.social: some issue and comment data are missing.

  • 🇧🇪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
  • Status changed to Needs work almost 2 years ago
  • Status changed to Needs review almost 2 years ago
  • 🇧🇪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.

  • 🇺🇸United States les lim

    Les Lim made their first commit to this issue’s fork.

  • Open on Drupal.org →
    Core: 9.5.5 + Environment: PHP 8.0 & MySQL 5.7 updated deps
    last 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.

  • Open in Jenkins → Open on Drupal.org →
    Core: 10.0.7 + Environment: PHP 8.1 & MySQL 8
    last update over 1 year ago
    run-tests.sh fatal error
  • Open in Jenkins → Open on Drupal.org →
    Core: 9.5.5 + Environment: PHP 8.0 & MySQL 5.7 updated deps
    last update over 1 year ago
    run-tests.sh fatal error
  • 🇺🇸United States les lim

    @DieterHolvoet Thanks!

  • First commit to issue fork.
  • Open in Jenkins → Open on Drupal.org →
    Core: 10.1.4 + Environment: PHP 8.1 & MySQL 8
    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
  • 🇦🇺Australia acbramley

    Let's get the MR green.

  • 🇨🇦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

  • 🇵🇭Philippines Zed9

    This patch worked for me.

  • 🇺🇸United States les lim

    One problem with the previous MR is the getRouteCollectionForRequest() call, which calls out to cache. The PreviewLinkEntityHooks 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.

  • Pipeline finished with Failed
    8 months ago
    Total: 144s
    #140816
  • miiimooo Europe

    miiimooo changed the visibility of the branch preview_link-3124785-reroll-68f89a1 to hidden.

  • Pipeline finished with Failed
    8 months ago
    Total: 143s
    #140833
  • Pipeline finished with Failed
    8 months ago
    Total: 143s
    #140894
  • 🇨🇦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
  • 🇦🇺Australia acbramley

    Need confirmation on #49

  • 🇦🇺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 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.

  • Pipeline finished with Success
    7 months ago
    Total: 278s
    #173957
  • Pipeline finished with Success
    7 months ago
    Total: 284s
    #173960
  • Pipeline finished with Success
    7 months ago
    Total: 365s
    #174001
  • Pipeline finished with Success
    7 months ago
    Total: 310s
    #174726
  • Pipeline finished with Success
    7 months ago
    Total: 560s
    #174727
  • Status changed to Needs review 7 months ago
  • 🇦🇺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

  • Pipeline finished with Success
    7 months ago
    Total: 337s
    #174862
  • Pipeline finished with Success
    7 months ago
    Total: 345s
    #176556
  • Pipeline finished with Success
    7 months ago
    Total: 364s
    #176574
  • Pipeline finished with Success
    7 months ago
    Total: 373s
    #176627
  • 🇵🇭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

  • Pipeline finished with Success
    22 days ago
    Total: 329s
    #351440
  • Pipeline finished with Failed
    22 days ago
    Total: 290s
    #351455
  • Pipeline finished with Success
    21 days ago
    Total: 220s
    #352477
  • Pipeline finished with Success
    21 days ago
    Total: 218s
    #352483
  • 🇦🇺Australia richard.thomas

    I've re-based MR27 onto the latest release so it should be mergeable again and the diff should apply to 2.2.0-alpha1.

    Is there anything else I can do to try and get this one merged in? We've been using it for several months now across a few sites and it's been working well.

Production build 0.71.5 2024