Fix error when resolving the entity on entity.node.revision route

[bboro]

When an entity's render array has 'og_membership_state' cache
context, then OG's route resolvers error out when viewing that
node's earlier revisions on the entity.node.revision route.

This happens because $route->getParameter('node') returns a string
ID of the entity instead of the fully loaded entity.

Reproduction steps for testing

I tested this on Gizra/drupal-starter which comes already with pluggable entity view builder module.

1. Start ddev and install og ddev start && ddev composer require drupal/og:1.x-dev

2. Enable og and og_ui ddev . drush --no-interaction en og og_ui

3. Go to Structure -> Content types and

   1. Set Basic Page to be a Group
   2. Set Article to be Basic Page's Group content

4. Create a basic page node : Test group

5. Create an article node : Test group content

6. Edit the article and create a new revision with some changes.

7. Add og_membership_state cache context on the article's render array. To do this we edit Drupal\server_general\Plugin\EntityViewBuilder\NodeArticle class's buildFull method. Add the following line before the return $build; line:

   CacheableMetadata::createFromRenderArray($build)->addCacheContexts(['og_membership_state'])->applyTo($build);

   So the whole function becomes:

   public function buildFull(array $build, NodeInterface $entity) {
     $build['header'] = $this->buildHeroHeader($entity);
     $build['tags'] = $this->buildContentTags($entity);
     $build['body'] = $this->buildBody($entity);
     CacheableMetadata::createFromRenderArray($build)->addCacheContexts(['og_membership_state'])->applyTo($build);
     return $build;
   }

   (Don't forget to import the CacheableMetadata class :) )

8. Open the article's revision we created earlier. If this was a fresh install it's probably going to be at /node/2/revisions/2/view path.

Expected result:

Opens revision

Actual result:

Error: Call to a member function getEntityTypeId() on string in Drupal\og\Plugin\OgGroupResolver\RouteGroupResolver->resolve()

[amitaibu]

@bboro Thanks. Can you add a test for this as-well please.

[bboro]

@amitaibu I guess a good way to achieve the test would be to create a test module, which is implementing hook_preprocess_node and add the cache context there? Then all that's needed is to setup the group and the group content, create the content, create revisions, then access the old revisions to assert the status code to be 200. Does that sound reasonable :)

[amitaibu]

Yeah, sounds good. I think we already have a test module, so you can have the hook_preprocess_node act based on some node title, or some dummy $node->_invoke_preprocess_node

[bboro]

Without the fix (test only)

With fix:

[amitaibu]

Seems Travis is broken (unrelated the the PR, as they fail earlier)

[amitaibu]

@pfrenssen are you familiar with the errors we see in https://github.com/Gizra/og/runs/1538496345 ?

[MPParsley]

@amitaibu, they're caused by the upgrade to composer 2. See #711.

[MPParsley]

@bboro, can you have a look at the failing tests?

[bboro]

@MPParsley I think the route resolve test issues should be "resolved" hehe.

baysaa@drupal-starter-web:/var/www/html/web$ phpunit -c phpunit.xml.dist --group og --filter testResolve
HTML output directory /var/www/html/web/sites/simpletest/browser_output is not a writable directory.
PHPUnit 8.5.21 by Sebastian Bergmann and contributors.

Testing 
................................                                  32 / 32 (100%)

Time: 24.39 seconds, Memory: 946.00 MB

OK (32 tests, 176 assertions)

Remaining self deprecation notices (12)

  8x: Calling the Symfony\Component\EventDispatcher\EventDispatcherInterface::dispatch() method with a string event name as the first argument is deprecated in drupal:9.1.0, an Event object will be required instead in drupal:10.0.0. See https://www.drupal.org/node/3154407
    8x in OgMembershipStateCacheContextTest::testResolvedGroupEntity from Drupal\Tests\og\Functional

  3x: Relying on entity queries to check access by default is deprecated in drupal:9.2.0 and an error will be thrown from drupal:10.0.0. Call \Drupal\Core\Entity\Query\QueryInterface::accessCheck() with TRUE or FALSE to specify whether access should be checked. See https://www.drupal.org/node/3201242
    3x in OgMembershipStateCacheContextTest::testResolvedGroupEntity from Drupal\Tests\og\Functional

  1x: The Drupal\Tests\og\Functional\OgMembershipStateCacheContextTest::$modules property must be declared protected. See https://www.drupal.org/node/2909426
    1x in DrupalListener::startTest from Drupal\Tests\Listeners

[pfrenssen]

This could have been implemented in a cleaner way by creating an @OgGroupResolver plugin which handles this special case. We have provided this plugin type for this reason, so we can override specific routes that define the group entity in a different argument in the path.

[pfrenssen]

Hmm I was looking into this but it seems the fix is not needed after all? When I revert the change to OgRouteGroupResolverBase, then the tests are still passing. It's also possible the test does not expose the bug.

[MPParsley]

Hmm I was looking into this but it seems the fix is not needed after all?

You're right, this should have been fixed in core in https://www.drupal.org/project/drupal/issues/2730631