workbench_access_entity_access should cache per user, or a new WBA specific cache context (per section)

Finally got a failing test for this, the crucial piece is that both users need to use the exact same role. Even if they use the same permissions, it'll create a different cid hash, see PermissionsHashGenerator::generate()

Seems like MR testing is broken here for some reason.

The last submitted patch, 5: 3376975-5--test-only.patch, failed testing. View results →

The last submitted patch, 5: 3376975-5.patch, failed testing. View results →

I have been able to replicate this locally, but the patch does not solve the issue.

Investigating.

This looks to be a render cache issue. Disabling cache on the view "fixes" the problem.

Even with the patch, here's what I see in the cache_render table for a cache id (cid):

| views:fields:content:page_1:124823b49aca05d07a5a8ee7bad0f807955bb52d82dc862923ca6c257c5b7045:
[languages:language_interface]=en:
[theme]=claro:
[user.permissions]=3a5d7130b0cfe7aade0408b995c8f254a5ab4f84add3558eb18d9335d88a6152   

I would expect the patch to change the [user.permission] part of this key, but it does not.

It seems like the access context isn't "bubbling" as expected?

It seems that the entity row render in views is not inheriting cache context.

This is what the $row element has -- and that's what is in the render cache.

I think this is a core bug. Making this change to Drupal\views\Plugin\views\field\EntityOperations::render() fixes the issue:

  public function render(ResultRow $values) {
    $entity = $this->getEntity($values);
    // Allow for the case where there is no entity, if we are on a non-required
    // relationship.
    if (empty($entity)) {
      return '';
    }

    $entity = $this->getEntityTranslationByRelationship($entity, $values);
    $operations = $this->entityTypeManager->getListBuilder($entity->getEntityTypeId())->getOperations($entity);
    if ($this->options['destination']) {
      foreach ($operations as &$operation) {
        if (!isset($operation['query'])) {
          $operation['query'] = [];
        }
        $operation['query'] += $this->getDestinationArray();
      }
    }
    $build = [
      '#type' => 'operations',
      '#links' => $operations,
      // Entity operations are user-dependent.
      '#cache' => [
        'contexts' => [
          'user',
        ],
      ],
    ];

    return $build;
  }

What I can't understand is why these changes cause existing tests to fail.

Wow. Tests were failing locally due to format issues with views.view.edit_link_test.yml causing imports to silently fail.

194:1 error Expected indentation of 8 spaces but found 10 spaces yml/indent
195:1 error Expected indentation of 8 spaces but found 10 spaces yml/indent
196:1 error Expected indentation of 8 spaces but found 10 spaces yml/indent

etc.

Something about the view in this test simply would not work for me - it kept breaking the install of other config and making tests silently fail.

I have refactored the test as a result.

Not sure why `route` is now part of the cache contexts on the view, nor why this failed in the first place.

@agentrickard as per the IS, you need the core patch to reproduce the issue with the Operations.

The changes to the view are quite drastic, my goal was to keep it as simple as possible. Adding the workbench access filter is not required and the reason it was left out was because there are other cache based things going on with that filter (I found a few previous issues dealing with it)

Let's see if the test only patch correctly fails.

#16 passing proves that the views changes need to be reverted (and retweaked if there are actually issues with it)

I don't understand why we're undoing PHP8 stuff here either, and the user role change is incorrect. We need 2 users with the exact same role. drupalCreateUser will create different roles for the same permission set, those roles will make the permissions cache context vary because it takes the rid into account.

Going to revert back to my changes and go from there.

Reverted changes and merged 2.0.x, but yes there is some weird issue that this view is causing, it doesn't seem to be the config directly though.

I recreated a brand new view from scratch and exported it, it ended up with almost identical config than what I originally had bar a few random keys. There were no indentation issues or anything so not sure what happened in #13

Anyway, deleting just that views config yaml makes tests pass, so agreed something is going on here but it really can't be the view itself. My inkling is that it's probably related to workbench_access_views_post_render/workbench_access_view_check and the static in there.

Funnily enough, it is cache related. Using ViewsUserOutputTest as a test bed (since it's simpler and doesn't involve anything new from this issue) I added a breakpoint just after the drupalLogin. Then updated my local env to talk to the test env and logged in as that user (after a cache clear) and saw the expected output. I then resumed the debugger and the test passed. Without the cache clear in there the sections were not output on the user-sections view display.

The only difference I can see is that in the fail state, the page has the workbench_access_view tag. Removing the edit_links view makes it pass and then we don't get that tag. The tag is added by workbench_access_views_post_render from what I can see.

Final debugging for the day:

Tried removing workbench_access_views_post_render entirely and adding the contexts and tags directly to the view but it still failed in the same way.

Notes:

* I updated the view because the initial view was crashing config imports and making other tests fail, so I copied an existing test view and added the edit link.
* The problem in #13 was caused by me doing a copy/paste
* Re: PHP8 -- we still support Drupal 9.5 in the 2.0 branch, which still allows PHP 7.4 → , so no PHP8-specific code just yet
* Re: "Then updated my local env to talk to the test env" --- How does one do that?!?

I am not liking the MR workflow that has us in the same fork, since my changes were all passing and don't seem problematic -- except I agree that we should remove the WA-section fields from the edit_links view.

This is...just odd. I can make the edit_links view work with this change:

langcode: en
status: true
dependencies:
  module:
    - node
    - user
    - workbench_access_test

Adding the test module as a dependency shouldn't be necessary -- since its in the $modules of the ViewsCacheTest. I wonder if the test runner is doing something odd with config import order though.

Here's a patch version for testing. I am hands-off the MR branch.

Missed a php8 specific annotation.

The last submitted patch, 25: 3376975-wa-cache-test-25.patch, failed testing. View results →

Last patch missed a merge and a syntax change.

since my changes were all passing and don't seem problematic

The changes made the test pass without the fix, which is problematic. Apologies for reverting them all.

I updated the view because the initial view was crashing config imports and making other tests fail,

I couldn't reproduce this at all with the original view, the test failures were the same issues we've had the whole time seemingly caused by importing the view before WBA was installed (which is fixed by adding the module as a config dependency). I don't understand how that can break other views.

Re: "Then updated my local env to talk to the test env" --- How does one do that?!?

How I do it is to stop the test (after hitting a breakpoint) so the tearDown doesn't clean up the environment. Then inspect the database to find the table prefix and update my settings.php to use that prefix.

Adding the test module as a dependency shouldn't be necessary -- since its in the $modules of the ViewsCacheTest. I wonder if the test runner is doing something odd with config import order though.

I thought I did try this although it might've been adding workbench_access as the dependency. I would love to know what's causing that to break other views but after already spending a day debugging it I'm not that inclined to continue 😅

Re: PHP8 -- we still support Drupal 9.5 in the 2.0 branch, which still allows PHP 7.4, so no PHP8-specific code just yet

That documentation is deprecated, https://www.drupal.org/docs/getting-started/system-requirements/php-requ... → is the correct one which states that PHP 7.3 support has been dropped since 9.4. Yes there are some caveats but I'd find it highly unlikely that we need to support it still, especially in test code.

Interdiff between the MR and #27 looks good, again I would love to know why that dependency is required but not sure if it's worth the effort to investigate.

@acbramley -- I will take one last look to see why that dependency is needed. If I can't figure it out in a half hour, I will commit this.

Nice note about PHP 8 - I think from, this point we can drop PHP 7, and I will note that on the project page.

Also -- nice work on the original issue. This one was a little tricky to catch.

I will follow-up on the Operations cache issue separately.

The problem with the test seems to be that we create nodes during setup() and those require workbench_access_test fields. But I can't easily figure out why that config isn't being imported.

Leaving the requirement on the view seems the solution.

This has been committed!

Thanks very much @agentrickard!

+++ b/workbench_access.module
@@ -63,7 +63,7 @@ function workbench_access_entity_access(EntityInterface $entity, $op, AccountInt
-    $carry->addCacheableDependency($scheme)->cachePerPermissions()->addCacheableDependency($entity);
+    $carry->addCacheableDependency($scheme)->cachePerUser()->addCacheableDependency($entity);

doesn't this mean anything that checks access with WBA is now UNCACHEABLE?

I would still like to explore a WBA specific cache context, something less granular than user but more granular than user.permissions. We can do that in a follow-up though.

Perhaps user.sections and we could just make it a hash of the user's section IDs.

We can use something like \Drupal\Core\Session\PermissionsHashGenerator::hash over the output from \Drupal\workbench_access\UserSectionStorageInterface::getUserSections

But I wonder how common it is for users to be part of the exact same sections?

I guess if you think of a scenario where you have an org split into departments. The bulk of users would be in a single department, so it would be common for them to have distinct groups of users with the same sections.

Worth exploring?

We could make it a configuration option, so that sites could decide if 'per user' (i.e no caching) was better than using a context with a lot of variations that would effectively grow their cache tables but could also have a poor hit rate anyway

@larowlan That should be a follow-up issue. This one was a bit of a quick fix. You can only cache per-user. That mostly affects edit/delete links from Views and routes. Given that these are all authenticated users anyway, I think the performance hit is very minor.

Entity access cache is really tricky but I like the user.sections hash idea. And this gives us a test to break (or not) when that it implemented.

Added a follow-up with some code for review

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