Problem/Motivation

While working on #2287071: Add cacheability metadata to access checks, I had to update AccessManagerTest. In doing so, I could not get two tests to pass.

After wasting many hours on investigating why this is the case, it turns out it's because the container was being used incorrectly.

Proposed resolution

$this->container->register('test_access', $access_check);

should have been

$this->container->set('test_access', $access_check);

in both tests.

Now the mocked access checker's (mocked) applies() method will actually (finally) be invoked. Hurray! However, doing so reveals that the arguments resolver was not yet being mocked for both of these tests. So I added that as well.

To prevent such regressions in these tests in the future, I changed the mocked access checkers' methods from any() to atLeastOnce(), which would have revealed this problem immediately.

In other words: these tests have never worked!

Remaining tasks

Review + commit.

User interface changes

None.

API changes

None.

CommentFileSizeAuthor
access_manager_test_broken.patch3.24 KBWim Leers
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

dawehner’s picture

Component: base system » routing system
Status: Needs review » Reviewed & tested by the community

In other words: these tests have never worked!

We should certainly not use $this->any() anymore, unless not possible. ... #2222719-16: Use parameter matching via reflection for access checks instead of pulling variables from request attributes

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Committed bf7b1b0 and pushed to 8.x. Thanks!

  • alexpott committed bf7b1b0 on 8.0.x
    Issue #2313115 by Wim Leers: Fixed AccessManagerTest is broken.
    

Status: Fixed » Closed (fixed)

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