Problem/Motivation

I'm using the session preview handler. When I use a preview link the first time, it works great.

But if I try to use it a second time without clearing the Drupal cache, I get redirected without starting a preview.

Steps to reproduce

Try to use a preview link twice without clearing the cache. The second attempt will redirect but won't start a preview.

FWIW, I'm on my local and I'm using the Session handler. But it I understand the bug correctly then it's a bug in the preview controller rather than the preview handler.

This is likely only a bug in the session handler.

Proposed resolution

Add the session cache context to the session preview handler.

Change this:

  /**
   * {@inheritdoc}
   */
  public function getCacheContexts(): array {
    return ['workspace'];
  }

to this:

  /**
   * {@inheritdoc}
   */
  public function getCacheContexts(): array {
    return ['workspace', 'session'];
  }

Remaining tasks

User interface changes

API changes

Data model changes

Issue fork workspace_preview-3608283

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

danflanagan8 created an issue. See original summary.

danflanagan8 opened merge request !7

danflanagan8’s picture

danflanagan8
Primary language English
Location St. Louis, US
commented
Issue summary: View changes

I was cooking up a test for this bug and I couldn't get it to fail for the cookie handler. So then I started suspecting that perhaps it was important that I was specifically using the session handler. And indeed we see a missing cache context on that handler. Adding that cache context makes things work for me locally.

I've updated the IS.

I still hope to add the test but it's quite ready.

danflanagan8’s picture

danflanagan8
Primary language English
Location St. Louis, US
commented
Status: Active » Needs review

I got the test in a good enough place to put it in the MR. I think this is ready for review now.

danflanagan8’s picture

danflanagan8
Primary language English
Location St. Louis, US
commented
Status: Needs review » Needs work

Darn. The cache context only fixes the bug for authenticated users doing previews. For anonymous users with permission to access preview links, the first use of the preview links still poisons the cache.

amateescu made their first commit to this issue’s fork.

amateescu’s picture

amateescu commented

This is a really nice find!

While looking into the new test, I found another bug: switching the active preview handler (e.g. from cookie to session) didn't invalidate previously-cached /workspace-preview/{id} activation redirects, so a visitor whose browser still has the old handler's cookie could get served a stale cached redirect built under the previous handler instead of the newly active one. Fixed that and improved the test a bit.

I'll look into #5, but probably tomorrow, so feel free to update the MR if you figure it out in the meantime.

amateescu’s picture

amateescu commented
Status: Needs work » Needs review

Turns out the answer to both problems is to use the page cache kill switch instead of adding the session context. Added a test for that problem as well :)

danflanagan8’s picture

danflanagan8
Primary language English
Location St. Louis, US
commented

Hi @amateescu

I think I understand caching well until I run into something like this...I can't believe that the kill switch ends up being the right approach. That's what I had in the IS originally and that's what I put in my first little MR. Then I saw the cache context that didn't look right to me and went down that rabbit hole so confidently!

I'm pulling down the MR now to test everything. I know the controller works because the kill switch MR i what I ended up applying to my project. The new tests look amazing, but there's one or two assertions I want to challenge a bit.

danflanagan8’s picture

danflanagan8
Primary language English
Location St. Louis, US
commented
Status: Needs review » Needs work

I posted my two little suggestions for the tests so I'll flip it back to NW to see what you think.

I think the all new test coverage puts the module in a really healthy place. Really awesome work!

danflanagan8’s picture

danflanagan8
Primary language English
Location St. Louis, US
commented
Status: Needs work » Reviewed & tested by the community

I love it.

Edit: I'm not sure why the tests are failing in Gitlab. They work great locally.

Edit 2: It's something to do with the test runner using a subdirectory, which I guess probably indicates some kind of incompatibility between workspace_preview and a site installed in a subdirectory. :(

amateescu’s picture

amateescu commented

I think I understand caching well until I run into something like this...

Same for me 😂

It's something to do with the test runner using a subdirectory

Yup, looking into that now. It's great that we have a functional test now to catch stuff like this!

amateescu’s picture

amateescu commented
Status: Reviewed & tested by the community » Fixed

Well.. this was a wild and fun ride :) Merged into 1.0.x, thanks!

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.

amateescu’s picture

amateescu commented

Released 1.0.0-beta8 as well.

Status: Fixed » Closed (fixed)

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