Update CacheableMetadata & AccessResult to use RefinableCacheableDependency(Interface|Trait)

RE: general AccessResult weirdness: it predates CacheableMetadata by many months.

RE: AccessResult::inheritCacheability(): that's because it really is designed for orIf() and andIf(). I wish we hadn't made it public. It first wasn't public.

1. +++ b/core/lib/Drupal/Core/Access/AccessResult.php
   @@ -215,35 +180,22 @@ public function isNeutral() {
      public function getCacheContexts() {
   -    sort($this->contexts);
   -    return $this->contexts;
   +    sort($this->cacheContexts);
   +    return $this->cacheContexts;
   

   +1 to leaving it unchanged, but please know that this predates the existence of Cache::mergeContexts() (which is what does the sorting for all other places in core).

   So, we should actually be able to remove this sort().

2. +++ b/core/lib/Drupal/Core/Access/AccessResult.php
   @@ -321,8 +260,7 @@ public function cachePerUser() {
      public function cacheUntilEntityChanges(EntityInterface $entity) {
   -    $this->addCacheTags($entity->getCacheTags());
   -    return $this;
   +    return $this->addCacheableDependency($entity);
      }
    
      /**
   @@ -334,8 +272,7 @@ public function cacheUntilEntityChanges(EntityInterface $entity) {
   
   @@ -334,8 +272,7 @@ public function cacheUntilEntityChanges(EntityInterface $entity) {
       * @return $this
       */
      public function cacheUntilConfigurationChanges(ConfigBase $configuration) {
   -    $this->addCacheTags($configuration->getCacheTags());
   -    return $this;
   +    return $this->addCacheableDependency($configuration);
      }
   

   YAY!

If you want, I can take a look at the remaining test failures?

1. +++ b/core/modules/content_translation/tests/src/Unit/Access/ContentTranslationManageAccessCheckTest.php
   index c3a9f7a..5fba2b2 100644
   --- a/core/modules/quickedit/tests/src/Unit/Access/EditEntityFieldAccessCheckTest.php
   

   The clean-up you did here to address #9 is beautiful! Thank you!

2. +++ b/core/modules/quickedit/tests/src/Unit/Access/EditEntityFieldAccessCheckTest.php
   @@ -84,16 +61,28 @@ public function providerTestAccess() {
   +  public function testAccess($entity_is_editable, $field_storage_access, AccessResult $expected_result) {
   

   s/$field_storage_access/$field_storage_is_accessible/

3. +++ b/core/tests/Drupal/Tests/Core/Access/AccessResultTest.php
   @@ -856,6 +876,10 @@ public function testAllowedIfHasPermissions($permissions, $conjunction, $expecte
   +    if ($permissions) {
   +      $expected_access->cachePerPermissions();
   +    }
   +
        $access_result = AccessResult::allowedIfHasPermissions($account, $permissions, $conjunction);
        $this->assertEquals($expected_access, $access_result);
      }
   @@ -869,14 +893,14 @@ public function providerTestAllowedIfHasPermissions() {
   
   @@ -869,14 +893,14 @@ public function providerTestAllowedIfHasPermissions() {
        return [
          [[], 'AND', AccessResult::allowedIf(FALSE)],
          [[], 'OR', AccessResult::allowedIf(FALSE)],
   -      [['allowed'], 'OR', AccessResult::allowedIf(TRUE)->addCacheContexts(['user.permissions'])],
   -      [['allowed'], 'AND', AccessResult::allowedIf(TRUE)->addCacheContexts(['user.permissions'])],
   -      [['denied'], 'OR', AccessResult::allowedIf(FALSE)->addCacheContexts(['user.permissions'])],
   -      [['denied'], 'AND', AccessResult::allowedIf(FALSE)->addCacheContexts(['user.permissions'])],
   -      [['allowed', 'denied'], 'OR', AccessResult::allowedIf(TRUE)->addCacheContexts(['user.permissions'])],
   -      [['denied', 'allowed'], 'OR', AccessResult::allowedIf(TRUE)->addCacheContexts(['user.permissions'])],
   -      [['allowed', 'denied', 'other'], 'OR', AccessResult::allowedIf(TRUE)->addCacheContexts(['user.permissions'])],
   -      [['allowed', 'denied'], 'AND', AccessResult::allowedIf(FALSE)->addCacheContexts(['user.permissions'])],
   +      [['allowed'], 'OR', AccessResult::allowedIf(TRUE)],
   +      [['allowed'], 'AND', AccessResult::allowedIf(TRUE)],
   +      [['denied'], 'OR', AccessResult::allowedIf(FALSE)],
   +      [['denied'], 'AND', AccessResult::allowedIf(FALSE)],
   +      [['allowed', 'denied'], 'OR', AccessResult::allowedIf(TRUE)],
   +      [['denied', 'allowed'], 'OR', AccessResult::allowedIf(TRUE)],
   +      [['allowed', 'denied', 'other'], 'OR', AccessResult::allowedIf(TRUE)],
   +      [['allowed', 'denied'], 'AND', AccessResult::allowedIf(FALSE)],
   

   This seems like an unnecessary bit of clean-up, and I'm not sure if it makes things actually cleaner/clearer?

   Can you explain why you made this change?

#14: Oh, right, that results in indirect container calls. Thanks!

#15:

Reroll, this is why I was waiting for that other issue to continue with this :)

:)

Here's a full review.

1. +++ b/core/lib/Drupal/Core/Access/AccessResult.php
   @@ -252,20 +203,7 @@ public function addCacheContexts(array $contexts) {
   +    $this->cacheContexts = array();
   
   @@ -275,7 +213,7 @@ public function addCacheTags(array $tags) {
   +    $this->cacheTags = array();
   

   If we're touching these lines anyway, let's also use [].

2. +++ b/core/lib/Drupal/Core/Access/AccessResult.php
   @@ -343,28 +281,6 @@ public function cacheUntilConfigurationChanges(ConfigBase $configuration) {
   -  public function addCacheableDependency($other_object) {
   

   Yay!

3. +++ b/core/lib/Drupal/Core/Cache/CacheableMetadata.php
   @@ -128,36 +81,7 @@ public function setCacheMaxAge($max_age) {
   -  public function addCacheableDependency($other_object) {
   

   Yay!

4. +++ b/core/lib/Drupal/Core/Cache/RefinableCacheableDependencyTrait.php
   @@ -53,7 +53,9 @@ public function addCacheableDependency($other_object) {
   -    $this->cacheContexts = Cache::mergeContexts($this->cacheContexts, $cache_contexts);
   +    if ($cache_contexts) {
   +      $this->cacheContexts = Cache::mergeContexts($this->cacheContexts, $cache_contexts);
   +    }
   
   @@ -61,7 +63,9 @@ public function addCacheContexts(array $cache_contexts) {
   -    $this->cacheTags = Cache::mergeTags($this->cacheTags, $cache_tags);
   +    if ($cache_tags) {
   +      $this->cacheTags = Cache::mergeTags($this->cacheTags, $cache_tags);
   +    }
   

   Sensible optimizations.

5. +++ b/core/modules/quickedit/tests/src/Unit/Access/EditEntityFieldAccessCheckTest.php
   @@ -33,6 +35,11 @@ class EditEntityFieldAccessCheckTest extends UnitTestCase {
   +    $cache_contexts_manager = $this->prophesize(CacheContextsManager::class)->reveal();
   

   <3

6. +++ b/core/tests/Drupal/Tests/Core/Access/AccessResultTest.php
   @@ -852,8 +851,16 @@ public function testOrIfCacheabilityMerging() {
   +   *   The conjunction to use when checking for permission. Either 'AND" or
   

   Nit: inconsistent quotes. Fixed. Also reworded to fit on one line.

7. +++ b/core/tests/Drupal/Tests/Core/Entity/EntityAccessCheckTest.php
   @@ -25,6 +27,12 @@ class EntityAccessCheckTest extends UnitTestCase {
      public function testAccess() {
   +
   +    $cache_contexts_manager = $this->prophesize(CacheContextsManager::class)->reveal();
   

   Nit: needless blank line. Fixed.

8. +++ b/core/tests/Drupal/Tests/Core/Route/RoleAccessCheckTest.php
   @@ -143,6 +145,12 @@ public function roleAccessProvider() {
      public function testRoleAccess($path, $grant_accounts, $deny_accounts) {
   +
   +    $cache_contexts_manager = $this->prophesize(CacheContextsManager::class)->reveal();
   

   Same nit. Fixed.

RE: the inheritCacheability() confusingness — I propose we deprecate that method in a follow-up (it's clearly out of scope). But what is in scope, is making it not duplicate that much anymore. So, I did that, and added docs explaining the difference. Nevertheless it is still quite confusing, which is why I think we should deprecate it in a follow-up, and remove at least the callers in core.

The patch in #19 is RTBC IMO. In #20, I only simplified and documented inheritCacheability() to the extent that that is reasonably in scope for this issue.

Therefore, if Berdir agrees with the changes I made in #20, I think this is RTBC.

OMG we totally lost track of this!

Oh. I misread.

1. --- a/core/modules/system/tests/src/Unit/Menu/MenuLinkTreeTest.php
   +++ b/core/modules/system/tests/src/Unit/Menu/MenuLinkTreeTest.php
   

   Changes here look great.

2. +++ b/core/modules/system/tests/src/Unit/Menu/MenuLinkTreeTest.php
   --- a/core/tests/Drupal/Tests/Core/Form/FormBuilderTest.php
   +++ b/core/tests/Drupal/Tests/Core/Form/FormBuilderTest.php
   

   Like you indicated on IRC, the changes here look spotty.

3. +++ b/core/tests/Drupal/Tests/Core/Form/FormBuilderTest.php
   @@ -592,7 +592,7 @@ public function providerTestChildAccessInheritance() {
   -    $access_result = AccessResult::forbidden()->addCacheContexts(['user']);
   +    $access_result = AccessResult::forbidden();
        $clone['#access'] = $access_result;
    
        $expected_access = [];
   @@ -624,11 +624,9 @@ public function providerTestChildAccessInheritance() {
   
   @@ -624,11 +624,9 @@ public function providerTestChildAccessInheritance() {
    
        // Allow access on the most outer level but forbid otherwise.
        $clone = $element;
   -    $access_result_allowed = AccessResult::allowed()
   -      ->addCacheContexts(['user']);
   +    $access_result_allowed = AccessResult::allowed();
        $clone['#access'] = $access_result_allowed;
   -    $access_result_forbidden = AccessResult::forbidden()
   -      ->addCacheContexts(['user']);
   +    $access_result_forbidden = AccessResult::forbidden();
   

   However, these changes are merely changing both the input and the expected output to not have cache contexts. That's it.

   Since this test is really only about testing the behavior of #access set to a forbidden access result at the root of a render tree, that is totally fine. The cacheability metadata is not essential for testing here. It's #access's support for AccessResultInterface objects that we're testing.

   However, we can even continue to test cacheability here. Just don't test with cache contexts, which use an external service. Use max-age instead :)

   NW for changing these from using the user cache context to max-age (pick any value you like, I'd say pick a fun number :)).

Then, we should be able to finally get this in :)

1. +++ b/core/tests/Drupal/Tests/Core/Form/FormBuilderTest.php
   @@ -19,6 +19,8 @@
   +use Drupal\Core\Cache\Context\CacheContextsManager;
   +use Symfony\Component\DependencyInjection\ContainerBuilder;
   
   @@ -27,6 +29,25 @@
      /**
   +   * The dependency injection container.
   +   *
   +   * @var \Symfony\Component\DependencyInjection\ContainerBuilder
   +   */
   +  protected $container;
   +
   +  /**
   +   * {@inheritdoc}
   +   */
   +  protected function setUp() {
   +    parent::setUp();
   +
   +    $this->container = new ContainerBuilder();
   +    $cache_contexts_manager = $this->prophesize(CacheContextsManager::class)->reveal();
   +    $this->container->set('cache_contexts_manager', $cache_contexts_manager);
   +    \Drupal::setContainer($this->container);
   +  }
   +
   +  /**
   

   These changes can now be reverted.

2. +++ b/core/tests/Drupal/Tests/Core/Form/FormBuilderTest.php
   @@ -521,6 +542,14 @@ public function testChildAccessInheritance($element, $access_checks) {
   +
   +      if ($actual_access instanceof AccessResult) {
   +        $this->assertEquals(42, $actual_access->getCacheMaxAge());
   +      }
   +    }
   +
   +    if ($element['#access'] instanceof AccessResult) {
   +      $this->assertEquals(42, $element['#access']->getCacheMaxAge());
        }
   

   This is again specifically testing cacheability. We don't want to do that here, that's not what the test is about. So, then, let's remove the max-age stuff altogether, just like you had in the previous patch.

Came back green on DrupalCI, let's get this in finally :)

Hurray for more consistency in dealing with cacheability metadata!

Thanks for your patience, @borisson_!

Crazy errors like

PHP Fatal error:  Uncaught exception 'ReflectionException' with message 'Class Twig_ExtensionInterface does not exist' in /var/lib/drupaltestbot/sites/default/files/checkout/core/lib/Drupal/Core/DependencyInjection/Compiler/TaggedHandlersPass.php:99

Looks like testbot was temporarily broken. Re-testing.

YAY! :)

Updated https://www.drupal.org/developing/api/8/cache/cacheable-dependency-inter... to mention this: https://www.drupal.org/node/2541914/revisions/view/8784837/8784851