Allow Custom blocks to be set as non-reusable adding access restriction based on where it was used.

1. +++ b/core/lib/Drupal/Core/Access/AccessDependentInterface.php
   @@ -0,0 +1,26 @@
   +  /**
   +   * Sets the access dependee.
   +   *
   +   * @param \Drupal\Core\Access\AccessibleInterface $access_dependee
   +   *   The access dependee.
   +   */
   +  public function setAccessDependee(AccessibleInterface $access_dependee);
   

   "Dependee" is a strange word, can we clarify what it means in this context? Right now, it's not clear what is depending on what.

2. +++ b/core/lib/Drupal/Core/Access/AccessDependentInterface.php
   @@ -0,0 +1,26 @@
   +  /**
   +   * Gets the access dependee.
   +   *
   +   * @return \Drupal\Core\Access\AccessibleInterface
   +   *   The access dependee.
   +   */
   +  public function getAccessDependee();
   

   What if no dependee has been defined?

3. +++ b/core/lib/Drupal/Core/Access/AccessDependentTrait.php
   @@ -0,0 +1,32 @@
   +  /**
   +   * The object that is depended on.
   

   Also known as a "dependency", which might be a better word here. :)

4. +++ b/core/modules/block_content/block_content.install
   @@ -138,3 +138,47 @@ function block_content_update_8400() {
   +  $reusable = BaseFieldDefinition::create('boolean')
   +    ->setLabel(t('Reusable'))
   +    ->setDescription(t('A boolean indicating whether this block is reusable.'))
   +    ->setDefaultValue(TRUE)
   +    ->setInitialValue(TRUE);
   

   Rather than completely copy the field definition, it might be preferable to directly invoke BlockContent::baseFieldDefinitions() and use what comes back from that.

5. +++ b/core/modules/block_content/block_content.install
   @@ -138,3 +138,47 @@ function block_content_update_8400() {
   +  $view = $config_factory->getEditable('views.view.block_content');
   

   We should check if $view->isNew() and return if it is, because that would mean that the block_content view had been deleted by the site owner. Also, we don't need to keep $conig_factory in a local variable.

6. +++ b/core/modules/block_content/block_content.install
   @@ -138,3 +138,47 @@ function block_content_update_8400() {
   +      $view->set("$base.id", 'reusable');
   +      $view->set("$base.plugin_id", 'boolean');
   +      $view->set("$base.table", $base_table);
   +      $view->set("$base.field", "reusable");
   +      $view->set("$base.value", "1");
   +      $view->set("$base.entity_type", "block_content");
   +      $view->set("$base.entity_field", "reusable");
   

   I believe all of these calls can be chained.

7. +++ b/core/modules/block_content/block_content.module
   @@ -105,3 +108,62 @@ function block_content_add_body_field($block_type_id, $label = 'Body') {
   + * Alter any query with all tags:
   

   Should be "Alters"

8. +++ b/core/modules/block_content/block_content.module
   @@ -105,3 +108,62 @@ function block_content_add_body_field($block_type_id, $label = 'Body') {
   +  if ($query instanceof SelectInterface && $query->hasAllTags('entity_query_block_content', 'entity_reference') && array_key_exists('block_content_field_data', $query->getTables())) {
   

   I'm not a huge fan of the fact that this hard-codes the name of the data table. Can we get the name of the table from the entity type definition instead?

   Also, under what circumstances would the query not be an instance of SelectInterface?

9. +++ b/core/modules/block_content/block_content.module
   @@ -105,3 +108,62 @@ function block_content_add_body_field($block_type_id, $label = 'Body') {
   +      // If no reusable condition set to one.
   

   Set to TRUE, not one.

10. +++ b/core/modules/block_content/block_content.module
    @@ -105,3 +108,62 @@ function block_content_add_body_field($block_type_id, $label = 'Body') {
    + * Utility function find nested conditions using the reusable field.
    

    "...function to find..."

11. +++ b/core/modules/block_content/block_content.module
    @@ -105,3 +108,62 @@ function block_content_add_body_field($block_type_id, $label = 'Body') {
    + * @param mixed $condition
    + *   The conditions to check.
    

    Won't the condition either be an array or ConditionInterface? We can probably improve this type hint.

12. +++ b/core/modules/block_content/block_content.module
    @@ -105,3 +108,62 @@ function block_content_add_body_field($block_type_id, $label = 'Body') {
    +  $field = isset($condition['field']) ? $condition['field'] : NULL;
    

    Under what circumstances will $condition['field'] be NULL?

13. +++ b/core/modules/block_content/block_content.module
    @@ -105,3 +108,62 @@ function block_content_add_body_field($block_type_id, $label = 'Body') {
    +  if (is_object($field) && $field instanceof Condition) {
    

    I think we should use ConditionInterface instead.

14. +++ b/core/modules/block_content/block_content.module
    @@ -105,3 +108,62 @@ function block_content_add_body_field($block_type_id, $label = 'Body') {
    +    return _block_content_has_reusable_condition($field->conditions());
    

    We should probably check $condition first before recursing downwards, so that we can return earlier.

15. +++ b/core/modules/block_content/block_content.module
    @@ -105,3 +108,62 @@ function block_content_add_body_field($block_type_id, $label = 'Body') {
    +      if (is_int($key) && is_array($nested_condition)) {
    

    The is_int() makes me nervous, because it's coupling itself to deep implementation details of the Condition class. Not 100% sure how to work around that, though.

16. +++ b/core/modules/block_content/src/BlockContentAccessControlHandler.php
    @@ -19,10 +20,24 @@ class BlockContentAccessControlHandler extends EntityAccessControlHandler {
    +      if (!$entity instanceof AccessDependentInterface) {
    +        throw new \Exception("Non-reusable block entities must implement \Drupal\Core\Access\AccessDependentInterface for access control.");
    +      }
    

    I'm not sure why we'd have this check. Can't we just add AccessDependentInterface to BlockContentInterface, so that all custom blocks implement it? Also, I think this should be \LogicException.

17. +++ b/core/modules/block_content/src/Entity/BlockContent.php
    @@ -200,6 +204,11 @@ public static function baseFieldDefinitions(EntityTypeInterface $entity_type) {
    +    $fields['reusable'] = BaseFieldDefinition::create('boolean')
    +      ->setLabel(t('Reusable'))
    +      ->setDescription(t('A boolean indicating whether this block is reusable.'))
    +      ->setDefaultValue(TRUE);
    

    Why is this different from the field definition in the update hook?

18. +++ b/core/modules/block_content/src/Entity/BlockContent.php
    @@ -282,6 +291,21 @@ public function setRevisionLogMessage($revision_log_message) {
    +    $this->set('reusable', $reusable);
    +    return $this;
    

    I think $this->set() already returns $this, so the second line is redundant.

Great work @tedbow!

1. +++ b/core/modules/block_content/block_content.install
   @@ -138,3 +138,47 @@ function block_content_update_8400() {
   +function block_content_update_8600() {
   ...
   +function block_content_update_8601() {
   

   Why do we need two separate update hooks?

2. +++ b/core/modules/block_content/block_content.install
   @@ -138,3 +138,47 @@ function block_content_update_8400() {
   +  if ($view->get('base_table') !== $base_table or $view->isNew()) {
   

   or => ||

3. +++ b/core/modules/block_content/src/BlockContentAccessControlHandler.php
   @@ -19,10 +20,24 @@ class BlockContentAccessControlHandler extends EntityAccessControlHandler {
   +      if (empty($dependee)) {
   +        return AccessResult::forbidden("Non-reusable blocks must set an access dependee for access control.")->addCacheableDependency($dependee);
   

   Would it make sense to make this optional? i.e. I have a block that should not be re-usable (... or derivable) but is not used by a dependee. I'm not entirely sure when (and if) that would be applicable though.

4. +++ b/core/modules/block_content/src/Plugin/Derivative/BlockContent.php
   @@ -43,7 +43,7 @@ public static function create(ContainerInterface $container, $base_plugin_id) {
   +    $block_contents = $this->blockContentStorage->loadByProperties(['reusable' => TRUE]);
   

   I think it was asked for a profiling of this change in the earlier issue.

#10

Here is edge case where I think we want the AccessForbidden by default.

So the problem with that edge case is that we break that either way, if the block is denied access there will be no block rendered at all. Though I agree, it's probably better to NOT render it, than rendering something that SHOULD not be rendered.

+++ b/core/modules/block_content/block_content.module
@@ -105,3 +108,63 @@ function block_content_add_body_field($block_type_id, $label = 'Body') {
+function block_content_query_block_content_access_alter(AlterableInterface $query) {
+  $data_table = \Drupal::entityTypeManager()->getDefinition('block_content')->getDataTable();
+  if ($query instanceof SelectInterface && $query->hasAllTags('entity_query_block_content', 'entity_reference') && array_key_exists($data_table, $query->getTables())) {
+    $reusable_conditions = array_filter($query->conditions(), '_block_content_has_reusable_condition');
+    if (empty($reusable_conditions)) {
+      // If no reusable condition create a condition set to TRUE.
+      $query->condition("$data_table.reusable", TRUE);
+    }
+  }
+}

Is this hook implementation necessary? Entity queries check access by default and the access control handler for block content has been modified to deny access unless an access dependency is present.

Just about everything I found is a doc comment thing. I think this makes a lot of sense, and it's clean! However, we are going to need a test of the update path.

1. +++ b/core/lib/Drupal/Core/Access/AccessDependentInterface.php
   @@ -0,0 +1,41 @@
   + * @code
   + * $accessible->getAccessDependee()->access($op, $account, TRUE);
   + * @endcode
   

   getAccessDependee() no longer exists.

2. +++ b/core/lib/Drupal/Core/Access/AccessDependentInterface.php
   @@ -0,0 +1,41 @@
   +interface AccessDependentInterface {
   

   Should this extend AccessibleInterface?

3. +++ b/core/lib/Drupal/Core/Access/AccessDependentInterface.php
   @@ -0,0 +1,41 @@
   +   * @param \Drupal\Core\Access\AccessibleInterface $access_dependency
   +   *   The access dependency.
   

   Can we rephrase the description: "The object upon which access depends"?

4. +++ b/core/lib/Drupal/Core/Access/AccessDependentInterface.php
   @@ -0,0 +1,41 @@
   +  public function setAccessDependency(AccessibleInterface $access_dependency);
   

   Should this method return $this?

5. +++ b/core/modules/block_content/block_content.install
   @@ -138,3 +138,47 @@ function block_content_update_8400() {
   +  // Make sure the view's 'base_table' is correct and the View is not new. The
   +  // default view could have been deleted and possibly the view ID could be used
   +  // for a different view.
   

   Can we rephrase the second sentence for clarity -- something like "The default view may have been replaced by a completely different one with the same ID."

6. +++ b/core/modules/block_content/block_content.module
   @@ -105,3 +108,69 @@ function block_content_add_body_field($block_type_id, $label = 'Body') {
   +    if (array_key_exists($data_table, $query->getTables())) {
   +      if (!_block_content_has_reusable_condition($query->conditions())) {
   

   These can be combined: if (array_key_exists(...) && !_block_content_has_reusable_condition(...)

7. +++ b/core/modules/block_content/block_content.module
   @@ -105,3 +108,69 @@ function block_content_add_body_field($block_type_id, $label = 'Body') {
   +    foreach ($condition as $nested_condition) {
   +      if (is_array($nested_condition)) {
   

   We can remove a level of nesting if we change this to: foreach (array_filter($condition, 'is_array')) as $nested_condition)

8. +++ b/core/modules/block_content/block_content.module
   @@ -105,3 +108,69 @@ function block_content_add_body_field($block_type_id, $label = 'Body') {
   +    return strpos($field_parts[0], $data_table) === 0 && $field_parts[1] === 'reusable';
   

   Can we add a comment explaining why strpos() is called?

9. +++ b/core/modules/block_content/src/BlockContentAccessControlHandler.php
   @@ -19,10 +20,24 @@ class BlockContentAccessControlHandler extends EntityAccessControlHandler {
   +        return AccessResult::forbidden("Non-reusable blocks must set an access dependency for access control.")->addCacheableDependency($dependency);
   

   Why are we adding $dependency as a cacheable dependency if it's empty?

10. +++ b/core/modules/block_content/src/Entity/BlockContent.php
    @@ -75,7 +77,9 @@
    +class BlockContent extends EditorialContentEntityBase implements BlockContentInterface, AccessDependentInterface {
    

    Can we get away with having BlockContentInterface extend AccessDependentInterface? Or would that be a BC break? (I think such a change might be covered under the 1:1 class/interface rule...?)

11. +++ b/core/modules/block_content/tests/src/Kernel/BlockContentDeriverTest.php
    @@ -0,0 +1,66 @@
    +/**
    + * Tests block content plugin deriver.
    + *
    + * @group block_content
    + */
    +class BlockContentDeriverTest extends KernelTestBase {
    

    O_O Did we not already have coverage for this?!

12. +++ b/core/modules/block_content/tests/src/Kernel/BlockContentDeriverTest.php
    @@ -0,0 +1,66 @@
    +  /**
    +   * Tests that reusable blocks only are derived.
    +   */
    

    For clarity: "Tests that only reusable blocks are derived".

13. +++ b/core/modules/block_content/tests/src/Kernel/BlockContentDeriverTest.php
    @@ -0,0 +1,66 @@
    +      'description' => "Provides a block type that increases your site's spiffiness by up to 11%",
    

    Yeah, but this one goes up to 11. ;)

• Can we expand on test coverage for the views and BlockContentListBuilder to make sure they don't expose non-reusable blocks?
• Re #10 and #8 we still need to do that profiling
• I think we need tests for the update hooks. i.e BlockContentUpdateTest

Looks great to me, +1 to RTBC this.

I think this needs a retitle, as this issue seems decoupled from LB (with that work occurring in #2957425: Allow the inline creation of non-reusable Custom Blocks in the layout builder).

1. +++ b/core/lib/Drupal/Core/Access/AccessDependentInterface.php
   @@ -0,0 +1,43 @@
   +  public function setAccessDependency(AccessibleInterface $access_dependency);
   ...
   +  public function getAccessDependency();
   

   What if there is more than one dependency?

2. +++ b/core/modules/block_content/block_content.module
   @@ -105,3 +108,65 @@ function block_content_add_body_field($block_type_id, $label = 'Body') {
   +function block_content_query_entity_reference_alter(AlterableInterface $query) {
   

   There is hook_entity_query_TAG_alter which would be a more focussed hook?

3. +++ b/core/modules/block_content/block_content.module
   @@ -105,3 +108,65 @@ function block_content_add_body_field($block_type_id, $label = 'Body') {
   +function _block_content_has_reusable_condition(array $condition) {
   

   wow, this sucks, entity query sure doesn't support introspection does it

   feels like we should add such an api (follow up)

4. +++ b/core/modules/block_content/block_content.post_update.php
   @@ -0,0 +1,39 @@
   +  foreach ($config_factory->listAll('views.view.') as $view_config_name) {
   

   We have a config entity updater utility class now, this should use that

5. +++ b/core/modules/block_content/tests/modules/block_content_view_override/config/install/views.view.block_content.yml
   @@ -4,6 +4,8 @@ dependencies:
   +_core:
   +  default_config_hash: gkRJCqHr3uSO8ALHLatX-7YKfX0lWEgkC5qMBtCf_Sg
   

   do we normally have these in core yml files?

6. +++ b/core/modules/block_content/tests/src/Kernel/BlockContentEntityReferenceSelectionTest.php
   @@ -0,0 +1,166 @@
   +      'description' => "Provides a block type that increases your site's spiffiness by up to 11%",
   

   scientists agree

So, whilst I get the intent of the patch, I have a question.

Are we sure we're not forcing a square peg into a round hole.

If we need a limited use content entity that is for layout plugins only, why don't we just create one and avoid bolting this functionality onto an existing entity type. Content entities are cheap and easy to create in Drupal 8.

Doing so would avoid the need for the access dependency thing and all of the entity query/views stuff.

We already have a way to say 'don't show this block in the normal block UI', because the field plugin deriver does that. Those field blocks are available to layout builder, but not to the normal 'block layout' page. That sounds exactly like what we want here. A new content entity, with a block deriver, but limited only to layout builder usage.

I know that is a dramatic change of direction - but it feels better to purpose build something and have complete control over it than to bolt something onto existing code.

Thoughts?

Having multiple dependencies interface used on BlockContent is misleading, because it would only ever use one.

Maybe we could rename it better to explain the composition we're actually after?

EDIT: Redacted.

1. +++ b/core/modules/block_content/block_content.install
   @@ -138,3 +138,29 @@ function block_content_update_8400() {
   +    ->setLabel(t('Parent entity type'))
   

   oh for DER in core :)

2. +++ b/core/modules/block_content/block_content.module
   @@ -105,3 +108,67 @@ function block_content_add_body_field($block_type_id, $label = 'Body') {
   + * These queries should only return with no parent blocks unless a condition on
   + * parent is explicitly set.
   

   english is off here

3. +++ b/core/modules/block_content/block_content.module
   @@ -105,3 +108,67 @@ function block_content_add_body_field($block_type_id, $label = 'Body') {
   + * create a instance of
   

   an instance?

4. +++ b/core/modules/block_content/block_content.module
   @@ -105,3 +108,67 @@ function block_content_add_body_field($block_type_id, $label = 'Body') {
   +function _block_content_has_parent_entity_condition(array $condition) {
   

   feels like this should be something generic we could use

5. +++ b/core/modules/block_content/block_content.post_update.php
   @@ -0,0 +1,39 @@
   +  foreach ($config_factory->listAll('views.view.') as $view_config_name) {
   +    $view = $config_factory->getEditable($view_config_name);
   +    if ($view->get('base_table') != $data_table) {
   

   we have the config updater service for this

6. +++ b/core/modules/block_content/src/BlockContentAccessControlHandler.php
   @@ -19,10 +19,23 @@ class BlockContentAccessControlHandler extends EntityAccessControlHandler {
   +        $access = $access->andIf($parent_entity->access($operation, $account, TRUE))->addCacheableDependency($entity);
   

   should we ->addCacheableDependency($parent_entity) too?

   if so, missing test coverage for that?

7. +++ b/core/modules/block_content/src/Entity/BlockContent.php
   @@ -200,6 +210,24 @@ public static function baseFieldDefinitions(EntityTypeInterface $entity_type) {
   +    // @todo Is there a core issue to add
   +    //   https://www.drupal.org/project/dynamic_entity_reference
   

   ha - jinx!

8. +++ b/core/modules/block_content/src/Entity/BlockContent.php
   @@ -290,4 +318,40 @@ protected static function invalidateBlockPluginCache() {
   +      return \Drupal::entityTypeManager()->getStorage($this->get('parent_entity_type')->value)->load($this->get('parent_entity_id')->value);
   

   should we catch plugin not found exception here in case parent_entity_type is mangled/references a deleted entity type?

9. +++ b/core/modules/block_content/src/Entity/BlockContent.php
   @@ -290,4 +318,40 @@ protected static function invalidateBlockPluginCache() {
   +    // If either parent field value is set then the block is considered to have
   +    // a parent.
   +    return !empty($this->get('parent_entity_type')->value) || !empty($this->get('parent_entity_id')->value);
   

   shouldn't we need both for it to be valid?

10. +++ b/core/modules/block_content/tests/src/Functional/Update/BlockContentParentEntityUpdateTest.php
    @@ -0,0 +1,168 @@
    +    $this->runUpdates();
    

    we should assert that the field and filter aren't present before we run the updates, to prevent people inadvertently updating the fixture and rendering this test redundant

11. +++ b/core/modules/block_content/tests/src/Functional/Update/BlockContentParentEntityUpdateTest.php
    @@ -0,0 +1,168 @@
    +    file_put_contents('/Users/ted.bowman/Sites/www/test.html', $this->getSession()->getPage()->getOuterHtml());
    

    debug?

Can't we just add a property to manage the state of the entity if it should be deleted or not, like File entity (file_managed) does?

Didn't really review yet, but one scenario we might want to explicitly test are private files. That's AFAIK the main use case where entity are being checked for view access outside of their usual context that's actually testable from the perspective of a user. So create a block type with private field and then make sure the file can be accessed as expected, e.g. not when it only exists as a pending revision on a node.

That said, there's AFAIK actually a bug with having it working at all on non-default revisions of an entity, that might prevent that kind of test for now, not sure.

I reviewed 80% of the patch, I didn't review all the test coverage in detail yet. I think you'll find helpful feedback in here already:

1. +++ b/core/lib/Drupal/Core/Access/AccessDependentInterface.php
   @@ -0,0 +1,43 @@
   + * Objects should implement this interface when their access depends on access
   + * to another object that implements \Drupal\Core\Access\AccessibleInterface.
   ...
   +interface AccessDependentInterface {
   

   Woah! Impressive. We've long needed this.

   To my ears, DependentAccessInterface sounds better, but that's perhaps because I'm a non-native speaker.

2. +++ b/core/lib/Drupal/Core/Access/AccessDependentInterface.php
   @@ -0,0 +1,43 @@
   + * dependency object. Objects that implement this interface are responsible for
   + * checking access of the access dependency because the dependency may not take
   + * effect in all cases. For instance an entity may only need the access
   

   Can you think of consequences for other parts in Drupal? AFAICT this handily shifts responsibility to every class that implements this interface; because it causes any AccessibleInterface implementation to need to update their ::access() method to take this into account. I think that's fine.

3. +++ b/core/lib/Drupal/Core/Access/AccessDependentInterface.php
   @@ -0,0 +1,43 @@
   +  public function setAccessDependency(AccessibleInterface $access_dependency);
   

   Ideally this interface would not have any setters.

   I checked and … the only caller of this method is a test! So it's totally possible to do this without the setter 🤞

4. +++ b/core/lib/Drupal/Core/Access/AccessDependentInterface.php
   @@ -0,0 +1,43 @@
   +  public function getAccessDependency();
   

   What if there are multiple?

   Also: shouldn't this interface extend AccessibleInterface?

5. +++ b/core/modules/block_content/block_content.module
   @@ -105,3 +108,72 @@ function block_content_add_body_field($block_type_id, $label = 'Body') {
   + * as entity reference values. A module can still
   + * create an instance of
   

   Whitespace nit.

6. +++ b/core/modules/block_content/block_content.module
   @@ -105,3 +108,72 @@ function block_content_add_body_field($block_type_id, $label = 'Body') {
   + * a condition on either the 'reusable' field.
   

   Incomplete sentence?

7. +++ b/core/modules/block_content/block_content.module
   @@ -105,3 +108,72 @@ function block_content_add_body_field($block_type_id, $label = 'Body') {
   +      // If no reusable condition create a condition set to TRUE.
   

   Not very clear. This is already explained in the docblock. Let's just remove this.

8. +++ b/core/modules/block_content/block_content.module
   @@ -105,3 +108,72 @@ function block_content_add_body_field($block_type_id, $label = 'Body') {
   +function _block_content_has_reusable_condition(array $condition, array $tables) {
   

   I wish there was an API for this sort of thing, because this code definitely looks both complex and wonky. Those two adjectives in one sentence make it scary.

   🙈 + 📚 = 😱

9. +++ b/core/modules/block_content/block_content.post_update.php
   @@ -0,0 +1,46 @@
   + * Adds a 'reusable' filter to Custom Block views.
   

   s/to/to all/

10. +++ b/core/modules/block_content/src/BlockContentEvents.php
    @@ -0,0 +1,28 @@
    +  const INLINE_BLOCK_GET_DEPENDENCY = 'block_content.get_dependency';
    

    This is the only mention of "inline" in both this patch and in the block_content module. Let's rename that.

11. +++ b/core/modules/block_content/src/Entity/BlockContent.php
    @@ -282,6 +302,20 @@ public function setRevisionLogMessage($revision_log_message) {
    +  public function setReusable($reusable = TRUE) {
    

    This should not accept a parameter. We need:

    public function setReusable();
    public function setUnreusable();
    

    This is the pattern that \Drupal\Core\Entity\EntityPublishedInterface introduced.

12. +++ b/core/modules/block_content/tests/modules/block_content_view_override/config/install/views.view.block_content.yml
    @@ -468,7 +468,7 @@ display:
    -      max-age: 0
    +      max-age: -1
    
    @@ -493,5 +493,110 @@ display:
    -      max-age: 0
    +      max-age: -1
    

    Why this change? (I like it, but why is it happening in this issue/patch? What change is triggering it?)

1. +++ b/core/lib/Drupal/Core/Access/DependentAccessInterface.php
   @@ -0,0 +1,43 @@
   + * $accessible->getAccessDependency()->access($op, $account, TRUE);
   ...
   +  public function setAccessDependencies(array $access_dependencies);
   

   Plural. Also worth noting that this is the perfect place to introduce variadics. It's sad to see us creating a new interface that could benefit so much from that a version before we have it. :-( oh well d9.

   For myself, I'd prefer an "addAccessDependency()" method that guarantees the AccessibleInterface object is passed, but once I've gone that far, I start to wonder about and/or logic and how we have to iterate the getAccessDependencies() method to do this properly. It leaves me wanting 2 classes that just implement AccessibleInterface for and/or that take an array of dependencies in the constructor and just call the access() method.

2. +++ b/core/lib/Drupal/Core/Access/DependentAccessTrait.php
   @@ -0,0 +1,32 @@
   +    $this->accessDependencies = $access_dependencies;
   

   No error checking to ensure AccessibleInterface.

3. +++ b/core/modules/block_content/block_content.module
   @@ -105,3 +108,73 @@ function block_content_add_body_field($block_type_id, $label = 'Body') {
   +function _block_content_has_reusable_condition(array $condition, array $tables) {
   

   Any good reason not to make this a service?

4. +++ b/core/modules/block_content/src/BlockContentAccessControlHandler.php
   @@ -2,27 +2,87 @@
   +        $event = new BlockContentGetDependenciesEvent($entity);
   +        $this->eventDispatcher->dispatch(BlockContentEvents::BLOCK_CONTENT_GET_DEPENDENCIES, $event);
   +        $dependencies = $entity->getAccessDependencies();
   

   This is great, and is probably more evidence to support an addAccessibleDependency() method on the DependentAccessInterface interface above. That being said, I still think there's an opportunity here to use custom dependent access and/or classes in this case to simplify things, but that could totally be done in a follow up or at some later date.

5. +++ b/core/modules/block_content/src/BlockContentInterface.php
   @@ -48,6 +49,28 @@ public function setInfo($info);
   +  public function setNonreusable();
   

   maybe setNotReusable() ??

6. +++ b/core/modules/block_content/src/Event/BlockContentGetDependenciesEvent.php
   @@ -0,0 +1,40 @@
   +  public function getBlockContent() {
   

   getEntity() maybe? I know it's a block content entity, but I'd be lost first time I saw this. Maybe it doesn't matter.

   I'd totally add a addAccessDependency() method to this event and use that to interact with the entity properly. That actually means you could abstract away from entities doing all this work later and just introspect the entity to make decision (if at some point that makes sense to do).

   Obviously there are some other methods that might make sense to add on the event once you do that.

All in all I really like this patch and I think it's super close to being ready. We're probably too late in the cycle to seriously consider my DependentAccessAnd/Or class suggestion, but other than that, I think the rest is pretty straight forward and on point. Super excited about this.

Eclipse

#68: 👍👍

#68.3: Okay, so #2957425: Allow the inline creation of non-reusable Custom Blocks in the layout builder also has some calls to the setter. Is it actually necessary though? Every single time I've worked on a value object and added setters to it, I've regretted it later. I'd like to help us not introduce more of that. Immutable value objects prevents lots of frustrating bugs/debugging.

(Funny that I independently made several remarks that @larowlan also made :) And yay for consulting @tim.plunkett to speed up consensus building!)

#69:

1. For myself, I'd prefer an "addAccessDependency()" method that guarantees the AccessibleInterface object is passed,

   +1

   This is also relevant wrt #68.3.

   interface CacheableDependencyInterface {
     public function getCacheContexts();
     public function getCacheTags();
     public function getCacheMaxAge();
   }
   

   +

   interface RefinableCacheableDependencyInterface extends CacheableDependencyInterface {
     …
     public function addCacheableDependency($other_object);
   }
   

   i.e. the default interface is immutable, the optional extra interface adds mutability. That'd be much more comfortable.

   So then you end up with:

   interface DependentAccessInterface {
     public function getAccessDependencies()
   }

   +

   interface RefinableDependentAccessInterface {
     public function addAccessDependency()
   }
   

   _{Although to address Kris' point, you could as well do assert(Inspector::assertAll(function($v) { return $v instanceof AccessibleInterface; }, $dependencies));.}

#70:

I think the code above $dependency->in_preview also points to an assumption that the dependencies are going to entities […]

This is fine, use assert(Inspector::assertAll(…)); to verify that your precondition holds true. It's fine here because this is code specifically dealing with BlockContent entities. If this were generic code, it would be a problem.

Right now we don't have a way to group AccessibleInterface implementations but what if introduced(in a follow up) AccessibleGroupInterface which would extend AccessibleInterface.

… this is looking a lot like repeating the structure \Drupal\Core\Config\Entity\Query\Condition.

Patch review:

+++ b/core/lib/Drupal/Core/Access/DependentAccessInterface.php
@@ -25,19 +25,19 @@
-  public function getAccessDependencies();
+  public function getAccessDependency();

Huh? Why did this change back to single instead of multiple? I guess that's what you were getting at in #70.1, but I thought you were just theorizing there.

It doesn't make sense for me to review this patch in detail until we agree on single vs multiple.

I'd like to get @tim.plunkett's thoughts on this.

It takes the need to understand how combine multiple instances of AccessibleInterface out of access handlers

This is IMHO the best argument.

Ok I would love to get consensus on this.

+1 — can we get feedback from @tim.plunkett on this?

This matches my understanding of our conversation, I think it looks great!
Deferring to Wim for the final sign-off.

But it doesn't want \Drupal\Core\Access\RefinableDependentAccessInterface to only have setAccessDependency() meaning you would be always erase previous access unless you did. He wanted to make sure that it also had mergeAccessDependency() so you can easily merge an accessible without erasing existing ones.

I both agree and disagree with that!

• I agree setAccessDependency() is bad, for the reason he cited: it overwrites (erases) previous data.
• I agree with the concept of mergeAccessDependency() but disagree with its name: I think addAccessDependency() is better. I explained in detail why in #71. In short: because it then follows the example set by RefinableCacheableDependencyInterface::addCacheableDependency()

We went back and forth over add vs merge for a while.
Interesting that addCacheableDependency() is a merge operation and exists on the same interface as mergeCacheMaxAge()!

I'm fine with either name.

Time for detailed patch review including nits!

1. +++ b/core/lib/Drupal/Core/Access/AccessGroupAnd.php
   @@ -0,0 +1,17 @@
   +  protected function doCombineAccess(AccessResultInterface $accumulatedAccess, AccessResultInterface $dependencyAccess) {
   

   s/fooBar/foo_bar/ in parameter names.

2. +++ b/core/lib/Drupal/Core/Access/AccessibleGroupBase.php
   @@ -0,0 +1,64 @@
   +   * Combines the access result of one dependency to previous dependencies.
   ...
   +  abstract protected function doCombineAccess(AccessResultInterface $accumulatedAccess, AccessResultInterface $dependencyAccess);
   

   I think compute would be clearer than do combine?

3. +++ b/core/lib/Drupal/Core/Access/AccessibleGroupBase.php
   @@ -0,0 +1,64 @@
   +   *   The combine access result of previous dependencies.
   

   Accumulated? Or combined?

   In array_reduce() this is called the carry.

4. +++ b/core/lib/Drupal/Core/Access/AccessibleGroupInterface.php
   @@ -0,0 +1,29 @@
   + * Extends AccessibleInterface to allow for access objects that have multiple
   + * dependencies.
   

   80 col.s

5. +++ b/core/lib/Drupal/Core/Access/RefinableDependentAccessInterface.php
   @@ -0,0 +1,45 @@
   +  public function setAccessDependency(AccessibleInterface $access_dependency);
   

   I thought this was supposed to no longer be there?

6. +++ b/core/lib/Drupal/Core/Access/RefinableDependentAccessInterface.php
   @@ -0,0 +1,45 @@
   +   * AccessibleGroupInterface the dependency well added to the existing access
   

   s/well/will be/

7. +++ b/core/lib/Drupal/Core/Access/RefinableDependentAccessTrait.php
   @@ -0,0 +1,48 @@
   + * Implements \Drupal\Core\Access\RefinableDependentAccessInterface.
   

   Trait for FCQN_OF_INTERFACE.

8. +++ b/core/lib/Drupal/Core/Access/RefinableDependentAccessTrait.php
   @@ -0,0 +1,48 @@
   +    if (!$this->accessDependency instanceof AccessibleGroupInterface) {
   

   elseif

9. +++ b/core/modules/block_content/block_content.module
   @@ -105,3 +108,73 @@ function block_content_add_body_field($block_type_id, $label = 'Body') {
   + * @todo Replace this a function to a call to the API in
   + *   https://www.drupal.org/project/drupal/issues/2984930
   

   s/this a function to a call/this function with a call/

10. +++ b/core/modules/block_content/src/BlockContentAccessControlHandler.php
    @@ -2,27 +2,87 @@
    +    if ($entity->isReusable() === FALSE) {
    

    This is inspecting the entity. Therefore you must precede it with $access->addCacheableDependency($entity) to ensure the access result varies correctly.

11. +++ b/core/modules/block_content/src/BlockContentAccessControlHandler.php
    @@ -2,27 +2,87 @@
    +      if (!$dependency instanceof EntityInterface || empty($dependency->in_preview)) {
    

    I don't understand the rationale behind this if-test. It's probably obvious to you because you're so deep in this problem space. I think a brief comment would help a lot :)

12. +++ b/core/modules/block_content/src/BlockContentEvents.php
    @@ -0,0 +1,28 @@
    +   * \Drupal\Core\Access\AccessDependentTrait::getAccessDependency has not been
    +   * called.
    

    1. It's DependentAccessInterface
    2. I don't think you mean the getter? :)

13. +++ b/core/modules/block_content/src/BlockContentInterface.php
    @@ -48,6 +49,28 @@ public function setInfo($info);
    +   * Sets the block to be non-reusable.
    ...
    +  public function setNotReusable();
    

    Non vs not, let's be consistent.

14. +++ b/core/modules/block_content/src/Entity/BlockContent.php
    @@ -282,6 +302,27 @@ public function setRevisionLogMessage($revision_log_message) {
    +    return (bool) $this->get('reusable')->value;
    

    return $this->get('reusable')->getCastedValue();
    

15. +++ b/core/modules/block_content/tests/src/Functional/Rest/BlockContentResourceTestBase.php
    @@ -92,6 +92,11 @@ protected function getExpectedNormalizedEntity() {
    +      'reusable' => [
    +        [
    +          'value' => TRUE,
    +        ],
    +      ],
    

    New field added, hence also exposed via REST. Makes sense!

    (Let's also mention this in the CR.)

1. +++ b/core/lib/Drupal/Core/Access/AccessibleGroupBase.php
   @@ -0,0 +1,64 @@
   +    $access_result = NULL;
   ...
   +      if ($access_result === NULL) {
   

   Would it be simpler to initialize this as a neutral and then use ->combineAccess everytime? or would that not work?

2. +++ b/core/lib/Drupal/Core/Access/RefinableDependentAccessTrait.php
   @@ -0,0 +1,48 @@
   +    elseif (!$this->accessDependency instanceof AccessibleGroupInterface) {
   

   we returned above so we don't need elseif here

My only remaining question is - do we absolutely want to put those access interfaces in the core namespace. It means we only have one go to get them right. Layout builder is still beta but if we put those classes in core, we are fairly locked in to what we do with them. Should we consider marking them internal? Should the event be internal for the time being? Thoughts?

#85

• 2. 👍
• 5. Why do you need to be able to replace all the dependencies?
• 11. 👍 EXCELLENT comment! Thank you!

#86 It means we only have one go to get them right Agreed with this concern.

#87:

1. 👍 Nice, even better!
2. See patch review below.
3. 👍 LGTM
4. Heh. I found it confusing to read, but I defer to the core committer's opinion of course :)
5. See patch review below.

Patch review

1. +++ b/core/lib/Drupal/Core/Access/AccessibleGroupBase.php
   @@ -28,15 +28,12 @@ public function addDependency(AccessibleInterface $dependency) {
   +      return AccessResult::forbidden();
   
   +++ b/core/tests/Drupal/Tests/Core/Access/AccessGroupTest.php
   @@ -34,6 +34,10 @@ public function testGroups() {
   +    // Ensure that groups with no dependencies return a forbidden access result.
   +    $this->assertTrue((new AccessGroupOr())->access('view', $this->account, TRUE)->isForbidden());
   +    $this->assertTrue((new AccessGroupAnd())->access('view', $this->account, TRUE)->isForbidden());
   

   Should be AccessResult::neutral().

2. +++ b/core/lib/Drupal/Core/Access/AccessibleGroupInterface.php
   @@ -4,6 +4,8 @@
   + * @internal
   
   +++ b/core/lib/Drupal/Core/Access/DependentAccessInterface.php
   @@ -19,6 +19,8 @@
   + * @internal
   
   +++ b/core/lib/Drupal/Core/Access/RefinableDependentAccessInterface.php
   @@ -4,6 +4,8 @@
   + * @internal
   

   This only partially addresses @larowlan's concern. The fact that it already lives in core/lib is a problem though; it should live in the Layout Builder or Block Content module. Otherwise many developers will start using it anyway.

1. +++ b/core/modules/block_content/src/BlockContentEvents.php
   @@ -0,0 +1,29 @@
   +final class BlockContentEvents {
   ...
   +  const BLOCK_CONTENT_GET_DEPENDENCY = 'block_content.get_dependency';
   
   +++ b/core/modules/block_content/src/Event/BlockContentGetDependencyEvent.php
   @@ -0,0 +1,68 @@
   +class BlockContentGetDependencyEvent extends Event {
   

   Shouldn't this then also become @internal?

   Also: CR should be updated to not mention these @internal things.

2. +++ b/core/modules/block_content/src/BlockContentInterface.php
   @@ -48,6 +49,28 @@ public function setInfo($info);
   +  public function isReusable();
   +
   +  /**
   +   * Sets the block to be reusable.
   +   *
   +   * @return $this
   +   */
   +  public function setReusable();
   +
   +  /**
   +   * Sets the block to be non-reusable.
   +   *
   +   * @return $this
   +   */
   +  public function setNonReusable();
   
   +++ b/core/modules/block_content/src/Entity/BlockContent.php
   @@ -200,6 +212,14 @@ public static function baseFieldDefinitions(EntityTypeInterface $entity_type) {
   +    $fields['reusable'] = BaseFieldDefinition::create('boolean')
   

   These are the key changes in this issue, and it has a CR 👍

1. Now 100% of this patch is scoped to core/modules/block_content.
2. All "reusability"-related changes are the actual API/data model changes (well, additions). They're communicated in the CR
3. All access-related things are @internal.

Thanks for helping to descope based on input from @larowlan and me!

Blocks a major (#2957425: Allow the inline creation of non-reusable Custom Blocks in the layout builder)

This looks great to me. Adding credit to all the reviewers. Thank you!

I pushed this to 8.7.x. Since alpha is this week and this patch is not trivial and might have side effects to stable core code that we haven't foreseen, leaving at RTBC for 8.6.x for a release manager to evaluate if and when to cherry pick to 8.6.

Kind of late to the party, but I see that this patch has quite some code dealing with entity reference and an upgrade path, so here's a review:

1. +++ b/core/modules/block_content/block_content.module
   @@ -105,3 +108,73 @@ function block_content_add_body_field($block_type_id, $label = 'Body') {
   +function block_content_query_entity_reference_alter(AlterableInterface $query) {
   

   I don't understand why this was implemented like this, wouldn't it be easier to write a custom entity reference selection plugin for the block_content entity type and add a default condition in buildEntityQuery()?

   Is it because of the nested logic? If so, selection handlers also have a entityQueryAlter() which gives you access to the underlying db query.

2. +++ b/core/modules/block_content/block_content.post_update.php
   @@ -0,0 +1,46 @@
   +  $data_table = \Drupal::entityTypeManager()
   +    ->getDefinition('block_content')
   +    ->getDataTable();
   

   Some sites might have disabled translatability of custom blocks, so we need to do the "is translatable ? get data table : get base table" dance, like everywhere else in Views :)

   Also, we should get the table names from the storage (after making sure that the entity type is using core's SqlContentEntityStorage), because the table names specified in the entity type definition are about to be deprecated.

3. +++ b/core/modules/block_content/block_content.post_update.php
   @@ -0,0 +1,46 @@
   +        $display['display_options']['filters']['reusable'] = [
   +          'id' => 'reusable',
   +          'plugin_id' => 'boolean',
   +          'table' => $data_table,
   +          'field' => 'reusable',
   +          'value' => '1',
   +          'entity_type' => 'block_content',
   +          'entity_field' => 'reusable',
   +        ];
   

   I thought we were supposed to include the full plugin definition in update functions, otherwise any change in defaults for that plugin would result in a config difference.

4. +++ b/core/modules/block_content/src/Access/AccessGroupAnd.php
   @@ -0,0 +1,50 @@
   +class AccessGroupAnd implements AccessibleInterface{
   +
   +
   +  /**
   

   Extra empty line here and a missing space before { :)

5. +++ b/core/modules/block_content/src/BlockContentAccessControlHandler.php
   @@ -2,27 +2,85 @@
   +    $access->addCacheableDependency($entity);
   

   Why do we need to add the entity as a cacheable dependency unconditionally here?

   Shouldn't we only add in the if below?

6. +++ b/core/modules/block_content/src/Entity/BlockContent.php
   @@ -200,6 +212,14 @@ public static function baseFieldDefinitions(EntityTypeInterface $entity_type) {
   +      ->setInitialValue(TRUE);
   

   We shouldn't include the initial value in the base field definition, it has no effect here because it's only supposed to be used in the update function which adds the field.

7. +++ b/core/modules/block_content/src/Entity/BlockContent.php
   @@ -282,6 +302,27 @@ public function setRevisionLogMessage($revision_log_message) {
   +    return $this->get('reusable')->first()->get('value')->getCastedValue();
   

   We usually call (bool) ... instead of ->getCastedValue(). In fact, this is the first time I see a call to getCastedValue() in a content entity class.

8. +++ b/core/modules/block_content/tests/src/Functional/Update/BlockContentReusableUpdateTest.php
   @@ -0,0 +1,155 @@
   + * @group Update
   + * @group legacy
   + */
   +class BlockContentReusableUpdateTest extends UpdatePathTestBase {
   

   This should also be in the block_content group.

9. +++ b/core/modules/block_content/tests/src/Functional/Update/BlockContentReusableUpdateTest.php
   @@ -0,0 +1,155 @@
   +   * @see block_content_update_8600
   +   * @see block_content_post_update_add_views_reusable_filter
   

   Needs () at the end of the method names.

10. +++ b/core/modules/block_content/tests/src/Functional/Update/BlockContentReusableUpdateTest.php
    @@ -0,0 +1,155 @@
    +    $this->container->get('module_installer')->install(['block_content_view_override']);
    

    I'm not sure it's safe to install a module before running the updates, we usually add/alter views and other config objects in a special "db preparation" file, like core/modules/field/tests/fixtures/update/drupal-8.views_entity_reference_plugins-2429191.php for example.

11. +++ b/core/modules/block_content/tests/src/Kernel/BlockContentEntityReferenceSelectionTest.php
    @@ -0,0 +1,189 @@
    +class BlockContentEntityReferenceSelectionTest extends KernelTestBase {
    

    We have EntityReferenceSelectionAccessTest for testing all of our custom selection handlers, can we move this one over there in a testBlockContent() method? Otherwise it would be a lot harder to find...

12. +++ b/core/modules/block_content/tests/src/Unit/Access/DependentAccessTest.php
    @@ -0,0 +1,160 @@
    +    $this->assertEquals('I have no opinion', $accessResultNeutral->getReason());
    +
    +  }
    

    Extra empty line :)

+++ b/core/modules/block_content/block_content.install
@@ -138,3 +138,19 @@ function block_content_update_8400() {
+function block_content_update_8600() {

Also, if this patch doesn't end up in 8.6.x after all, we need to rename this update function to block_content_updae_8700() otherwise we'll end up with the same confusing situation as we had with block_content_update_8400(), which only exists in 8.5.x :/

1. +++ b/core/modules/block_content/block_content.post_update.php
   @@ -6,18 +6,28 @@
   +  if ($storage instanceof SqlContentEntityStorage) {
   

   Is SqlEntityStorageInterface not good enough here? What's magic about that specific implementation?

2. +++ b/core/modules/block_content/block_content.post_update.php
   @@ -6,18 +6,28 @@
   +    // The storage is not an instance SqlContentEntityStorage we cannot
   +    // correctly determine the table from the storage.
   

   Missing some words here? "If the storage is not an instance of..."

   Also might be worth moving above the if

3. +++ b/core/modules/block_content/block_content.post_update.php
   @@ -6,18 +6,28 @@
   -    if ($view->get('base_table') != $data_table) {
   +    if ($view->get('base_table') != $table) {
   

   Might as well switch to !==

Other changes look good!

This issue #2232465: Deprecate table names from entity definitions deals with the methods but doesn't move them to an interface as far as I can tell.

See #2960037: Add a way to get the primary table of an entity type from TableMappingInterface

That's about replacing those methods with a method on TableMappingInterface.

There's many SqlContentEntityStorage-related warts - this meta issue is about fixing them: #2960147: Finalize the entity storage

@tedbow, re #100:

since there no access checking on the autocomplete for entity reference selection (except nodes?)

On the contrary, the main purpose of entity reference selection plugins is query-level access checking for all entity reference queries, at least until we get #2909970: Implement a query-level entity access API in core. See the buildEntityQuery() method of various selection handlers, for example UserSelection, FileSelection or CommentSelection.

So I really think that this query access logic should live in a BlockContentSelection plugin, because that should the canonical "source of truth" for any custom block_content selection plugin.

You do bring up a good point about possible pre-existing custom selection plugin implementations, but I would prefer if we can change block_content_query_entity_reference_alter to only cater for those cases, i.e. by checking if the selection plugin class is different than (and doesn't extend) Drupal\block_content\Plugin\EntityReferenceSelection\BlockContentSelection which should be introduced by this patch.

But also the plugins would be able to select not reusable block but then on edit the access checking would happen and you would get an error on your previous selection.

The access checking on edit for pre-existing references should not be a problem since #2791269: Allow saving pre-existing references to inaccessible items.

Because we inspecting the entity in the if we need to add the cacheablity because the fact that we are testing isReusable() at all means the access should be cached on the entity.

Right, so this means we can remove the addCacheableDependency($entity) call from the first if, since we're adding it unconditionally below.

Also, It would be a bit more clear if we move the line with $access->addCacheableDependency($entity); below the if ($entity->isReusable() === FALSE) { block, since that implies a thinking like "we determined access based on a property of of the entity, so we need to add it as a cacheable dependency". I think that's the common practice.. at least in core.

Is there a reason to use (bool), a cast, when we have an explicit method to get the casted value of our typed data?

A (bool) cast will make that code always return a boolean value, even in cases when the underlying field might change for whatever reason, and getCastedValue() would not return a boolean anymore.

11. It seems weird to move it because it is covers code in the block_content. Also that is BrowserTestBase and this is kernel test.

That test class handles all the existing access-related code from selection handlers (node, user, comment) and taxonomy will soon be added there as well in #2981887: Add a publishing status to taxonomy terms. Having it all in one place makes it easier to maintain, for me at least. As for it being a browser test, I just opened #2987084: Convert EntityReferenceSelectionAccessTest to a kernel test to fix that.

--

About #104.1: This whole area is very confusing, I know. It's because the table names from the entity type definition are about to be deprecated, but we don't yet have an "official" way of getting them, at least not until #2960037: Add a way to get the primary table of an entity type from TableMappingInterface lands.

I discussed this with @catch and he and I are both ok with this going into 8.6 before beta, so removing the "Needs release manager review" tag. That's still assuming though that #107 or something like it lands by the end of this week. If it does, I'll cherry pick both that commit and the #96 commit to 8.6. If at all possible, I'd love to do that by tomorrow or Thursday instead of Friday, just to give a bit of extra time for anyone who's testing the 8.6 branch tip to report back on if they run into any unanticipated problems with this code prior to beta getting tagged.

I think between #2987159: Create an entity reference selection plugin for custom blocks that filters out non-reusable blocks and this patch here, @tedbow has sufficiently addressed @amateescu's feedback.
This change will make it that much better for backporting to 8.6

FWIW, I'm very happy with the changes from #107 and the followup issue to use a proper selection plugin.

Adding issue credit to @amateescu. Thanks!

Pushed #107 to 8.7.x and cherry picked #96 and #112 to 8.6. Thanks for the great work and reviews on this!

Also published the CR and tagging for the release notes.

This made one of my upgrade tests fail:

1) Drupal\Tests\lingotek\Functional\Update\LingotekTargetStatusFormatterUpdate8209Test::testUpgrade
Schema key views.view.block_content:display.default.display_options.filters.reusable.group failed with: variable type is string but applied schema class is Drupal\Core\TypedData\Plugin\DataType\IntegerData

Found out the culprit here. The attached patch fixed it. Maybe we are missing some coverage in the upgrade tests?

Thank you for discovering that and reporting it! Setting to Needs Review for evaluation. If the bug and fix are correct, then yes we need a test for it too.

I raised that followup to Critical. Setting this back to Fixed.

It makes sense to mention this as a highlighted feature, but is there some disruption or important update information that requires a release notes mention?

Reading the CR, doesn't look like there's anything to mention in the 8.6.0 release notes for this fix. Thanks!