Problem/Motivation

Let's start by acknowledging that this is problematic topic, and following issues are referenced in many places:

  1. https://www.drupal.org/project/drupal/issues/2886800
  2. https://www.drupal.org/project/drupal/issues/3180960

I wanted to open a ticket as there was a behavior change in 1.0 to 1.1 upgrade of this module. Our custom module test is making multiple sub requests in a Kernel test and in version 1.1 and later the token set in a request stays there for the following requests unless the next request also is passing some value for hash_key.

We already had a workaround in place for the core issues mentioned earlier

$this->entityTypeManager->getAccessControlHandler('node')->resetCache();

And now to make tests pass we'd have to couple that with (or make extra sub request with auHash=NULL etc...):

\Drupal::service('access_unpublished.token_getter')->setToken(NULL);

But that does not feel right, maybe there is edge cases where that is bad? Could or should TokenGetter::setTokenFromRequest be updated to clear that token if hash_key is missing from query parameters?

Steps to reproduce

Proposed resolution

Remaining tasks

User interface changes

API changes

Data model changes

Issue fork access_unpublished-3229638

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

mikran created an issue. See original summary.

joachim’s picture

joachim commented 
    if ($event->getRequest()->query->has($tokenKey)) {
      $this->setToken($event->getRequest()->query->get($tokenKey));
    }

Agreed, there should be an 'else' clause after this which clears the token.

This is going to be a problem not just for tests, but for a persistent app server -- see #2218651: [meta] Make Drupal compatible with persistent app servers like ReactPHP, PHP-PM, PHPFastCGI, FrankenPHP, Swoole.

mably’s picture

mably commented
Status: Active » Postponed (maintainer needs more info)
Issue tags: +Needs steps to reproduce

This looks like an easy fix.

Could you provide steps to reproduce the problem please?

mably opened merge request !51

mably’s picture

mably commented
Status: Postponed (maintainer needs more info) » Needs review

@joachim I created an MR according to your comment.

Could you have a look at it please?

  • mably committed 680d1673 on 8.x-1.x 
    feat: #3229638 TokenGetter::setTokenFromRequest behaviour change with...
mably’s picture

mably commented
Status: Needs review » Fixed
Issue tags: -Needs steps to reproduce

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.

Status: Fixed » Closed (fixed)

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