Problem/Motivation

hook_shs_term_data_response_alter isn't processed when $access_unpublished = FALSE in https://git.drupalcode.org/project/shs/-/blob/2.0.0-rc1/src/Controller/S...

Steps to reproduce

Reproducing this requires a custom module that implements hook_shs_term_data_response_alter. Once that is in place, access the route using /shs-term-data/{identifier}/{bundle}/{entity_id} as a user with the `administer taxonomy` permission. You will see the altered JSON response.

Now try accessing the same path as a user without that permission. You will see the unaltered JSON.

Proposed resolution

Unsure. The logic in https://git.drupalcode.org/project/shs/-/blob/2.0.0-rc1/src/Controller/S... works with unpublished terms, but it limits hook_shs_term_data_response_alter to being used with users who have access unpublished terms.

Remaining tasks

User interface changes

API changes

Data model changes

Issue fork shs-3257879

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

kreynen created an issue. See original summary.

kreynen’s picture

kreynen commented

I think this can be resolved by changing $cache_tags[] = 'access_unpublished:' to something like $access_unpublished;
$cache_tags[] = sprintf('access_unpublished:taxonomy_term:%d', $tid); in https://git.drupalcode.org/project/shs/-/blob/2.0.0-rc1/src/Controller/S...

This may or may not be related to the changes made in #3225034: Simplify ResourceTypeRepository control flow for returning cached data.

I could really use some help understanding what is happening with the cached values when dealing with published vs. unpublished terms and these permissions.

kreynen opened merge request !2

stborchert’s picture

stborchert
he/him
Primary language German
commented
Version: 2.0.0-rc1 » 2.0.x-dev
Issue tags: +Needs tests

Hey.
Wow, I've never seen this behavior before. Very strange, indeed ...
It would be cool to have some tests for the controller so we can check the output in different versions ... I will try to add some within the next few days.

Updating the cache tags doesn't hurt, I've committed your changes.

kreynen’s picture

kreynen commented

If you can point me to a pattern of testing a hook like this, I'd be happen to work on some tests. I thought about creating a simplified version of how we're leveraging hook_shs_term_data_response_alter in as an example module and sharing that to show the issue. I really have no idea how to approach testing something like this on Drupal.org when the functionality is intended to be leveraged in another module. I know Webform includes a number of example sub modules, but I haven't dug into if they use those for tests.

Happy to help where I can.

kreynen’s picture

kreynen commented

Sorry. That merge should be reverted.

$tid isn't even defined at the point I tried to include it in the cache tag.

I've traced the problem to https://git.drupalcode.org/project/shs/-/blob/2.0.x/src/Cache/ShsCacheab..., but I'm at a loss. Regardless of what roles and permissions a user has, this is always processed when there is no cache available. When access the endpoint directly after clearing the cache, the process is something like the getTermData -> setData -> setData -> sendContent the first time. I'm not sure why setData is show up twice, but that's not really the issue. On the 2nd call (and every other call), we jump directly into sendContent. For an admin user, \Drupal::moduleHandler()->alter still runs as expected and returns the altered results in the JSON. For anonymous users \Drupal::moduleHandler()->alter doesn't run despite having the same values for $hooks, $this->content and $this->context.

stborchert’s picture

stborchert
he/him
Primary language German
commented

I've reverted the last commit.

I thought about creating a simplified version of how we're leveraging hook_shs_term_data_response_alter in as an example module and sharing that to show the issue. ... I know Webform includes a number of example sub modules

Yeah, that's exactly as core does for its tests and I would do it, too. Create a submodule specific for this test case that implements the hook and compare the given results with the expected.

When access the endpoint directly after clearing the cache, the process is something like the getTermData -> setData -> setData -> sendContent the first time. ... On the 2nd call (and every other call), we jump directly into sendContent.

This is the expected behavior for the response since the data is cached.

But ...

For anonymous users \Drupal::moduleHandler()->alter doesn't run despite having the same values for $hooks, $this->content and $this->context.

This is very strange and sounds really bad.

I could not test this right now but does this code in ModuleHandler::alter() call your hook implementation? Or is your function not in the list ($this->alterFunctions[$cid]) for anonymous users?

<?php
    foreach ($this->alterFunctions[$cid] as $function) {
      $function($data, $context1, $context2);
    }
?>

  • jhedstrom authored 127689c on 2.0.x 
    Issue #3257879 by finagler, kreynen, jhedstrom, stBorchert: add cache...