Add entity bundles condition plugin for entities with bundles

I do want to do this, but I think we need to get all the existing visibility logic for blocks encapsulated as plugins before I can justify doing it.

I disagree with having a generic entity bundle condition. Just having entity-generic actions/conditions is confusing to end-users as they are used to think in terms of content/comment/term/... So entity-type specific conditions/actions are much easier to grasp then, e.g. have "Create a node, Create a comment" instead of a single, generic "Create an entity" action. I went that generic route already with Rules 7.x-2.x and I must say it's a point I regret.

However, it would make sense to have derivates for generating a condition for each entity type.

I would never attempt this without derivatives. ;-)

We do this in ctools today. I'd be taking the exact same approach updated to D8.

Eclipse

>I would never attempt this without derivatives. ;-)

Good!

I think this needs to be moved to D9 now, as it would require an API change and is purely new feature. Move it back if you disagree, and in that case we'll check with a core maintainer.

I do NOT disagree. Much as I'd like to see it in, given the changes still going on with entities and typed data, I don't feel the slightest bit comfortable pushing this as an agenda item and we have much bigger fish to fry still.

9.x ++

Eclipse

If anybody is interested or finds this issue googling "entity block visibility" like I did, I have made a contrib module that does this in 8.x

https://drupal.org/project/entity_block_visibility

It's pretty simple for now but works against 8.x HEAD.

Looking at the entity_block_visibility.module I think that it's easy to backport to core in case the entity module will stay in core

Inventing a new tag.

We can't remove node type, but we could add entity bundle generically in a minor release, and possibly hide node type for new sites.

I've already written code for this in ctools. Would love to discuss migrating it in.

Eclipse

Needs tests and the UI adjustments mentioned in #11.

Although I generally think this a good I think UI is going to be very confusing to a lot of users

Luckily this is already out in C-Tools for Drupal 8 so we have testing in the wild with this feature.

Case in point an experienced Drupal trainer trying to figure what is going with showing blocks on "custom blocks"

After I explained what was going on he decided

But really in 99% of sites this not going to useful since the canonical link is the edit form.

Plus there is user mis-interpretations of this condition such as

1. Block shows when a custom block of the type is shown on the current page
2. Block is some how is shown as part of the these custom blocks

that would seems much more logic than what it will actually do(show the block on the edit form if placed using the admin theme)

Custom Menu Links and Short Cut Sets seem to also by 1% use cases that are also very open to misinterpretations.

• Does the block show on links in the short cut/menu link?

Luckily these types of entities are easy to weed out there canonical links are the same as there edit links.

Here is patch that does this.
The patch will weed out entities:

• block_content
• contact_message
• shortcut
• menu_link_content

I didn't even think about contact_message because the label, Contact form bundle, made me think of the actual form page but of course it is the message which in core in unviewable.

Ted,

Much of what you said was true... up until yesterday when #2632434: Conditions are not limited by available contexts. got committed. Now if there's not a context for a visibility condition, it won't show in core's UI any longer. And with this replacing the ctools condition... we get greater control over how this is implemented. Looks like I'm going to be writing an update hook when 8.3 is released.

Eclipse

It is certainly helpful to have generic conditions, but none of them would be useable when there is no corresponding route context provider. Currently core just ships with a node one, but in order to make all those conditions useful, this patch would have to ship with one for any entity type. Not sure how easy this would be to implement, but I guess it could be doable.

As far as I can tell this only works with entities that use other entities as bundles, so will not work with statically defined bundles or bundles as plugins, as Entity API in contrib is striving for. Is there a reason for this?

As far as I can tell this only works with entities that use other entities as bundles, so will not work with statically defined bundles or bundles as plugins, as Entity API in contrib is striving for. Is there a reason for this?

I would just assume that this is caused by simply forgetting about this specific flexibility of the system. IMHO we should totally able to use the bundle info api, as it seems to be the patch is just using bundle ID and label, which are both exposed from the bundle info API itself.

@EclipseGc in #12:

I've already written code for this in ctools. Would love to discuss migrating it in.

Code that I've rewritten to do exactly as @dawehner suggested in #21. So we should be able to copy over \Drupal\ctools\Plugin\Condition\EntityBundle and \Drupal\ctools\Plugin\Deriver\EntityBundle

Confirmed that #17 resolves #16 perfectly, no workaround needed.

I recreated this patch directly from the CTools code as suggested by #22 and then fixed PHPCS errors, but that didn't really change anything meaningful for what @tstoeckler was referring to.

So I didn't really move this forward very far...
Updating the IS with next steps.

Note that this blocks a stable release of Pathauto.

1. +++ b/core/lib/Drupal/Core/Entity/Plugin/Condition/Deriver/EntityBundle.php
   @@ -0,0 +1,89 @@
   +  public function getDerivativeDefinitions($base_plugin_definition) {
   +    foreach ($this->entityTypeManager->getDefinitions() as $entity_type_id => $entity_type) {
   +      if ($entity_type->hasKey('bundle')) {
   +        $this->derivatives[$entity_type_id] = $base_plugin_definition;
   +        $this->derivatives[$entity_type_id]['label'] = $this->getEntityBundleLabel($entity_type);
   

   As mentioned in IRC, I still think that the best first step would be to exclude node here, to avoid having both node_type and entity_bundle:node visibility conditions.

   Then we can figure out what to do about that in a follow-up issue, because I fear that trying to deprecate or even remove node_type will be complicated.

   Pathauto already explicitly uses the node_type condition and entity_bundle:XY for the other entity types, because I expected ctools would eventually do that to solve the duplicate-condition problem that exists right now.

2. +++ b/core/lib/Drupal/Core/Entity/Plugin/Condition/Deriver/EntityBundle.php
   @@ -0,0 +1,89 @@
   +  protected function getEntityBundleLabel(EntityTypeInterface $entity_type) {
   +    if ($label = $entity_type->getBundleLabel()) {
   +      return $this->t('@label', ['@label' => $label]);
   +    }
   +
   +    $fallback = $entity_type->getLabel();
   

   Wondering if it would make sense to have a follow-up to offer this method somewhere, have used this more than once in contrib.

To explain the pathauto problem a bit more, pathauto currently depends on ctools for this, which is only alpha and will likely remain like that for a while or at most get to beta in a reasonable time (The 8.x version was meant as an incubator for new core features, so the expecation is kind of that it can never really get stable as features will be removed as they are added to core). So I can't really do a stable release while depending on ctools, I'd either have to copy this stuff but then I have conflicts with ctools or get it into core.

1. getEntityTypeLabels() does not help us here. This about the label for the bundle of an entity type, so for node we want Content type, for Term vocabulary and so on. That method just returns a list of labels, we just need a specific one, no need to get the whole list.

For #25

There's some points
- maybe deprecate NodeType here as well?
- how that will affect config schema?
- for BC better to allow to use that condition for entity types without "bundle_label" annotation

1. +++ b/core/lib/Drupal/Core/Entity/Plugin/Condition/Deriver/EntityBundle.php
   @@ -0,0 +1,91 @@
   +      // @todo Stop excluding node_type once
   +      //   \Drupal\node\Plugin\Condition\NodeType is removed.
   +      if ($entity_type->hasKey('bundle') && $entity_type_id != 'node') {
   

   needs follow-up

2. +++ b/core/lib/Drupal/Core/Entity/Plugin/Condition/EntityBundle.php
   @@ -0,0 +1,140 @@
   +  public function submitConfigurationForm(array &$form, FormStateInterface $form_state) {
   +    $this->configuration['bundles'] = array_filter($form_state->getValue('bundles'));
   

   I bet this needs config schema

3. +++ b/core/lib/Drupal/Core/Entity/Plugin/Condition/EntityBundle.php
   @@ -0,0 +1,140 @@
   +      return $this->t('@bundle_type is @bundles or @last', [
   +        '@bundle_type' => $this->entityType->getBundleLabel(),
   ...
   +    return $this->t('@bundle_type is @bundle', [
   +      '@bundle_type' => $this->entityType->getBundleLabel(),
   

   this method could return NULL, needs check for BC

4. +++ b/core/modules/system/tests/modules/entity_test/src/Entity/EntityTestWithBundle.php
   @@ -12,6 +12,7 @@
     *   id = "entity_test_with_bundle",
     *   label = @Translation("Test entity with bundle"),
   + *   bundle_label = @Translation("Test entity bundle"),
   

   What if this remain empty?

About BC, I'm not exactly sure how we'd deprecate the NodeType plugin since it is a plugin and referenced in configuration, it's not something for developers.

The only option that I see is to expose both in the UI and change the label of the existing one to "(deprecated)" so that people have time to update their configuration until Drupal 9.

Note that just not exposing it is a problem for those people that already use ctools in contrib and have existing configuration that uses the generic plugin.

Also, the block form has some special handing for the node type condition that we need to discuss.

1. +++ b/core/lib/Drupal/Core/Entity/Plugin/Condition/Deriver/EntityBundle.php
   @@ -0,0 +1,91 @@
   +      if ($entity_type->hasKey('bundle') && $entity_type_id != 'node') {
   

   If we were to add a little logic to the default plugin manager, we could probably include a "deprecated = TRUE" in annotations and hide the plugin from being returned by getDefinitions() but still allow it to be instantiated. That would let us deprecate old plugin from the UI so that users aren't using it going forward and people can, over time unify on this plugin. We can then address anywhere core uses plugins for the 9.x migration when the time comes, or even add in extra logic to the ConditionManager that when "node_type" is requested, we instantiate "entity_bundle:node" instead. Something along these lines should be pretty doable.

2. +++ b/core/lib/Drupal/Core/Entity/Plugin/Condition/EntityBundle.php
   @@ -0,0 +1,157 @@
   +    $this->entityTypeBundleInfo = $entity_type_bundle_info;
   +    $this->entityTypeManager = $entity_type_manager;
   

   Nit pick, can we put these in the same order they're being passed in as parameters?

So, overall this is pretty much line for line what CTools does today. The exception of course is the constraint modification which (separately from this) I need to get into core. That being said, this looks pretty good.

Eclipse

This looks good to me.

Eclipse

1. +++ b/core/lib/Drupal/Core/Entity/Plugin/Condition/EntityBundle.php
   @@ -0,0 +1,157 @@
   +    foreach ($bundles as $id => $info) {
   +      $options[$id] = $info['label'];
   +    }
   

   nit: could just use array_column here

2. +++ b/core/lib/Drupal/Core/Entity/Plugin/Condition/EntityBundle.php
   @@ -0,0 +1,157 @@
   +    $entity = $this->getContextValue($this->entityType->id());
   +    return !empty($this->configuration['bundles'][$entity->bundle()]);
   

   Shouldn't this check the negated flag too?

3. +++ b/core/lib/Drupal/Core/Entity/Plugin/Condition/EntityBundle.php
   @@ -0,0 +1,157 @@
   +      return $this->t('@bundle_type is @bundles or @last', [
   ...
   +    return $this->t('@bundle_type is @bundle', [
   

   When negation is active, should this say is not instead of is?

4. +++ b/core/tests/Drupal/KernelTests/Core/Entity/EntityBundleConditionTest.php
   @@ -0,0 +1,82 @@
   +  public function testConditions() {
   

   doesn't appear to be any negation tests here

5. +++ b/core/tests/Drupal/KernelTests/Core/Entity/EntityBundleConditionTest.php
   @@ -0,0 +1,82 @@
   +    // Grab the node type condition and configure it to check against node type
   +    // of 'article' and set the context to the page type node.
   ...
   +    $this->assertFalse($condition->execute(), 'Page type nodes fail node type checks for articles.');
   ...
   +    // Set the node type check to page.
   ...
   +    $this->assertTrue($condition->execute(), 'Page type nodes pass node type checks for pages');
   ...
   +    // Set the node type check to page or article.
   ...
   +    $this->assertTrue($condition->execute(), 'Page type nodes pass node type checks for pages or articles');
   ...
   +    // Set the context to the article node.
   ...
   +    $this->assertTrue($condition->execute(), 'Article type nodes pass node type checks for pages or articles');
   ...
   +    // Set the context to the test node.
   ...
   +    $this->assertFalse($condition->execute(), 'Test type nodes pass node type checks for pages or articles');
   

   c/p references to node here?

Looks very good, +1 for RTBC but let's have some more feedback. Thank you for your great work so far!

Looks good to me, non-node tests that use page/article are IMHO a bit weird, I would have used some other strings but not really a problem.

It looks like the issue summary update needs to be updated here; for example, the IS says this issue should make the scenario possible without writing any code, but now that's all scoped to followups -- right?

From @tedbow and @tim.plunkett:

have you been following https://www.drupal.org/node/1932810 is this actually change anything on block config screen?

i have. it shouldn't

https://www.drupal.org/node/2914188 is the followup
there's a line in the deriver to NOT replace the node type one
so the IS is out of date
because it just adds conditions for every other entity type
and punts the swap for nodes to a follow-up

Hm, that IS and title don't really explain why we'd want this then (vs. why it can't just live in CTools).

IMHO, ctools and entity are both modules that contain more or less experimental code that should eventually move into core so that other modules don't have to depend on those anymore. So in a way, that alone is enough justification.

One example is pathauto which currently depends on ctools module for this and a single method with ~20 lines of code and I've seen 5+ issues already about people asking to remove the ctools dependency.

That said, IMHO your title made perfect since to me, while all those other things *also* use condition plugins, your example is exactly what it would be used for in core. (To be exact, to show only on taxonomy pages for terms of a certain vocabulary, which is actually a pretty useful thing). So I'd vote to restore the title, which did have a "e.g.", to highlight that it was just one example :)

The node-centric-ness of the existing code is only because we ported what was in core already, because making it generic was out of scope.
There's no reason to have this be generic.

Similar issue:
#2916740: Add generic entity actions

1. +++ b/core/lib/Drupal/Core/Entity/Plugin/Condition/Deriver/EntityBundle.php
   @@ -0,0 +1,91 @@
   +      $fallback = $this->entityTypeManager->getDefinition($bundle_entity_type)->getLabel();
   ...
   +    return $this->t('@label bundle', ['@label' => $fallback]);
   

   Is this intentional? Now Node does speficy a bundle label, but if it didn't the resulting label would be "Content type bundle" here instead of just "Content type". It seems the bundle entity type on its own is a reasonable fallback. Only if there is neither a bundle label nor a bundle entity type, I think we should fall back to $entity_type->label() . ' bundle', so for example "Content bundle" in the case of Node.

2. +++ b/core/lib/Drupal/Core/Entity/Plugin/Condition/EntityBundle.php
   @@ -0,0 +1,173 @@
   +    if (!$bundle_type = $this->entityType->getBundleLabel()) {
   +      if ($bundle_entity_type_id = $this->entityType->getBundleEntityType()) {
   +        $bundle_type = $this->entityTypeManager->getDefinition($bundle_entity_type_id)
   +          ->getLabel();
   +      }
   +      else {
   +        $bundle_type = $this->t('@label bundle', ['@label' => $this->entityType->getLabel()]);
   +      }
   +    }
   

   It seems this logic is a duplication of $this->pluginDefinition['label'], no?

   Interestingly, here the " bundle" part is actually only appended if there is no bundle entity type, like I suggested above.

Yes, the logic should be the same in both cases, I think I also suggested at some point to have a method on EntityType to do that, then we don't have to duplicate it? Maybe I did that before we had two different implementations of that logic?

+++ b/core/lib/Drupal/Core/Entity/Plugin/Condition/Deriver/EntityBundle.php
@@ -0,0 +1,91 @@
+    if ($label = $entity_type->getBundleLabel()) {
+      return $this->t('@label', ['@label' => $label]);
+    }
+

This does not need a $this->t(), just return $label, there's nothing you can translate about that :)

I also figured out another issue with this over the weekend, that I'm not quite sure how to handle, but we might want to wait on #2922451: [policy no patch] Make it possible to mark plugins as deprecated.

This is currently in ctools. ctools unfortunately exposes this also for nodes, so that means that all sites that have ctools installed currently have *two* Content Type visibility conditions and they probably used one of the two pretty randomly. Meaning, a lot of sites out there have entity:node_type conditions in their configuration. In blocks but also page manager and other places.

If we add it to core without entity:node_type and then ctools removes its code, then those sites will either break completely or at least their visibility conditions will no longer work.

Given that, we might need to deal with the deprecation of the node_type condition already here, probably based on whatever the other issue adds exactly.

100% agree, we should deprecate the existing core one. I'll focus some more attention on the deprecation issue.

Eclipse

The only option that I can think of is to use whatever API we come up with over there and then also add a (deprecated) or so to the actual plugin label (No, I don't see how not listing the plugin at all is an option, if anything we can try to hide it if it's not enabled).

What about this?

This removes the node type condition, exposes both and adds the same deprecated method to the constructor as we e.g. did in \Drupal\node\Plugin\Action\PublishNode. I also update the label with a (deprecated) suffix.

Then updated BlockForm to hide the deprecated plugin unless there is configuration for it. We could generalize that either here or in #2922451: [policy no patch] Make it possible to mark plugins as deprecated to a @deprecated = TRUE annotation. We could also add an update function that updates the block config entities, but we won't be able to deal with usages of it elsewhere, e.g. in pathauto. I kind of went in the wrong direction there with my implementation, expecting that ctools would remove entity_bundle:node, but that never happened so far.

I also tested that \Drupal\Tests\node\Functional\NodeBlockFunctionalTest doesn't trigger the deprecation message with this. We might want to add explicit test coverage for this deprecated handling though.

This also means that #2914188: Deprecate \Drupal\node\Plugin\Condition\NodeType plugin in favor of \Drupal\Core\Entity\Plugin\Condition\EntityBundle can be closed and is handled in this again. I think that's easier in the end.

This is how it looks with/without the deprecated condition being visible.

Fixing more tests, added an upgrade path and also fixed a problem this showed in the umami profile, the condition plugins need to have the right provider so that config dependencies are correct. Also added explicit test coverage for that.

@Berdir: Thank you very much for your work. What I'm missing on the screenshots is the very important "NOT" condition.

Yes, I added the same logic in BlockForm as the existing node type (and all other core conditions) have to hide that. That is a UI change and I didn't want to make the decision here on showing it, it was hidden on purpose on the existing plugin and *only* inside the block visibility UI.

The reason that it was hidden is AFAIK that the behavior of NOT is not clear on a non-node page.. should it be displayed there or not. As it is NOT an article, it possibly should, but the way it currently works, that's not possible as the context will be missing and access is denied to the block.

I'd suggest you open a separate issue about that, I'd like to avoid UI changes here as much as possible. It is also not specific to the node type condition, we do the same change on other conditions as well (roles, language, ..)

Also, looks like we kind of have some existing test coverage but probably doesn't hurt to have something more explicit too although we could put it as additional assertions in the existing tests too.

I do have one question, what should happen with the existing class, should it be marked as deprecated?

1. +++ b/core/lib/Drupal/Core/Entity/Plugin/Condition/Deriver/EntityBundle.php
   @@ -0,0 +1,67 @@
   +
   +  use StringTranslationTrait;
   +
   ...
   +    $this->stringTranslation = $string_translation;
   ...
   +      $container->get('string_translation')
   

   I don't see how this dependency is needed in this file. Am I blind?

2. +++ b/core/lib/Drupal/Core/Entity/Plugin/Condition/EntityBundle.php
   @@ -0,0 +1,165 @@
   +    $bundles = $this->entityTypeBundleInfo->getBundleInfo($this->entityType->id());
   ...
   +      '#options' => array_combine(array_keys($bundles), array_column($bundles, 'label')),
   

   Is there a reason we don't use array_map here?

1. Added the deprecation message also directly on the class. I guess it was kind of covered already in the constructor but this is more explicit for code inspection.
2. Yeah, you're totally history-blind, can't you see that we needed it in an earlier version of the patch? :) Removed.
3. As discused, didn't really seem easier to me to use array_map() here.

3. As discused, didn't really seem easier to me to use array_map() here.

Agreed

Created a draft record: https://www.drupal.org/node/2983299

We still need to figure out how this plays out exactly when ctools is installed, we can remove quite a bit of code now from it but we need to make sure exisiting sites don't break.

Confirmed that this doesn't do anything when ctools is installed, it still uses the classes defined by that. Problem there is that ctools actually adds two additional methods from an additional interface to those classes, so we can't just remove the plugins there, also not for BC.

@Berdir mentioned here wants to talk more about this issue with me - so setting back to needs review until that happens.

So we talked, and while nothing is broken right now, @alexpott pointed out that there is a risk that there could be a mismatch in the future that would result in sites being broken if they install/uninstall ctools.

To minimize that risk, I prepared a patch for ctools that refactors the current code there and only adds those additional methods to the classes and inherits everything else from core: #2857279: Duplicate node type visibility condition in block settings.

@alexpott agreed that it's OK to special case the node_type plugin deprecation here and replace it with a more generic approach in the existing open issue, added an explicit @todo for that. Also reordered some injected dependency arguments so the standard arguments come first, which is more common.

@berdir Are you happy with the patch now?

+++ b/core/lib/Drupal/Core/Entity/Plugin/Condition/EntityBundle.php
@@ -56,8 +52,12 @@ class EntityBundle extends ConditionPluginBase implements ContainerFactoryPlugin
    */
-  public function __construct(EntityTypeManagerInterface $entity_type_manager, EntityTypeBundleInfoInterface $entity_type_bundle_info, array $configuration, $plugin_id, $plugin_definition) {
+  public function __construct(array $configuration, $plugin_id, $plugin_definition, EntityTypeManagerInterface $entity_type_manager, EntityTypeBundleInfoInterface $entity_type_bundle_info) {
     parent::__construct($configuration, $plugin_id, $plugin_definition);
     $this->entityTypeManager = $entity_type_manager;
     $this->entityTypeBundleInfo = $entity_type_bundle_info;
@@ -69,11 +69,11 @@ public function __construct(EntityTypeManagerInterface $entity_type_manager, Ent

@@ -69,11 +69,11 @@ public function __construct(EntityTypeManagerInterface $entity_type_manager, Ent
    */
   public static function create(ContainerInterface $container, array $configuration, $plugin_id, $plugin_definition) {
     return new static(
-      $container->get('entity_type.manager'),
-      $container->get('entity_type.bundle.info'),
       $configuration,
       $plugin_id,
-      $plugin_definition
+      $plugin_definition,
+      $container->get('entity_type.manager'),
+      $container->get('entity_type.bundle.info')
     );
   }

That's good to know/remember!

There is an issue regarding negated conditions in ctools: #2823356: Negated conditions are always false for Views pages . I think that is also relevant here.

If I want my block to show up on all pages except articles, the logical way to configure that would be to select the article bundle and negate it. Since views (or other pages) are not articles, I would expect my block to show up there.

+++ b/core/lib/Drupal/Core/Entity/Plugin/Condition/EntityBundle.php
@@ -0,0 +1,165 @@
+  public function evaluate() {
+    if (empty($this->configuration['bundles']) && !$this->isNegated()) {
+      return TRUE;
+    }
+    /** @var \Drupal\Core\Entity\ContentEntityInterface $entity */
+    $entity = $this->getContextValue($this->entityType->id());
+    return !empty($this->configuration['bundles'][$entity->bundle()]);
+  }

Rerolled

+++ b/core/lib/Drupal/Core/Entity/Plugin/Condition/EntityBundle.php
@@ -0,0 +1,165 @@
+    return !empty($this->configuration['bundles'][$entity->bundle()]);

why isNegated() is not used in result?

This should fix some tests after #2932462: Add EntityContextDefinition for the 80% use case

1. +++ b/core/modules/block/block.install
   @@ -117,3 +117,24 @@ function block_update_8003() {
   +function block_update_8600() {
   ...
   +  $config_factory = \Drupal::configFactory();
   +  foreach ($config_factory->listAll('block.block.') as $block_config_name) {
   

   iirc config updates should live in post update and use sandbox https://www.drupal.org/node/2949630

2. +++ b/core/modules/node/tests/src/Kernel/NodeConditionTest.php
   @@ -10,6 +10,7 @@
   + * @group legacy
   ...
    class NodeConditionTest extends EntityKernelTestBase {
   

   it needs to add expected deprecation message

1. Configuration *entities*, yes, this uses the config API directly, that is allowed.

The only failing test Drupal\Tests\block\Functional\Update\BlockNodeTypeVisibilityUpdateTest but no idea hot to fix it

Maybe we can just ignore the legacy notices with because we are creating a bunch of deprecation notices which is where it's failing....
@group legacy

Rusty... forgot the fix in the patch.

Rerolled because context became context_definitions on core/modules/node/src/Plugin/Condition/NodeType.php

I think this may fail less... but I think there is still some errors outside the legacy errors produced.

Having a PITA time with functional testing locally, unit tests are running fine...

The reroll for context_definition is here:
#3014949: Deprecate 'context' on Block and Condition plugin annotations in favor of 'context_definitions'

Whoops typo :s/context_definition/context_definitions/ 😖

Seems like the issue reported in #79 hasn't been addressed.

If I set the visibility rule as: Article -- Negate
I expect the block to be shown everywhere except Article entity page which currently is not the case.

Obviously, this needs test coverage but can anyone either confirm or deny the approach?

+++ b/core/lib/Drupal/Core/Entity/Plugin/Condition/Deriver/EntityBundle.php
@@ -49,7 +49,7 @@ public function getDerivativeDefinitions($base_plugin_definition) {
         $this->derivatives[$entity_type_id]['context_definitions'] = [
-          $entity_type_id => EntityContextDefinition::fromEntityType($entity_type),
+          $entity_type_id => EntityContextDefinition::fromEntityType($entity_type)->setRequired(FALSE),
         ];

This change makes the test fail. The output is very confusing (see #2978261: \Drupal\Tests\system\Functional\Cache\AssertPageCacheContextsAndTagsTrait::assertCacheContexts() is unhelpful after conversion to phpunit), but basically, by making the context optional, it is no longer assigned in \Drupal\Core\Plugin\Context\ContextHandler::applyContextMapping(), and as a result, we don't get route cache context passed through. This is a problem because it will break accessing such a page first and then one that does have a node.

+++ b/core/modules/block/src/BlockForm.php
@@ -253,12 +262,15 @@ protected function buildVisibilityInterface(array $form, FormStateInterface $for
     }
+    if (isset($form['entity_bundle:node'])) {
+      $form['entity_bundle:node']['negate']['#type'] = 'value';
+      $form['entity_bundle:node']['negate']['#title_display'] = 'invisible';
+      $form['entity_bundle:node']['negate']['#value'] = $form['entity_bundle:node']['negate']['#default_value'];
+    }
     if (isset($form['user_role'])) {

The existing node_type plugin has the same issue, and we're taking over the existing UI code to hide the negate option for now, also so that this issue does not affect the UI. That means at least for the block visibility UI, it won't be possible to use the negate option anyway.

We likely need to open a separate issue to look into the cache context issue with optional contexts, it feels like the code there *should* be able to handle that but something isn't working. This stuff is pretty complex, it's not the first issue around that.

.

The output is very confusing (see #2978261: \Drupal\Tests\system\Functional\Cache\AssertPageCacheContextsAndTagsTrait::assertCacheContexts() is unhelpful after conversion to phpunit), but basically, by making the context optional, it is no longer assigned in \Drupal\Core\Plugin\Context\ContextHandler::applyContextMapping(), and as a result, we don't get route cache context passed through.

Does this have a standalone bug report?
I feel like every time someones really thinks through applyContextMapping() they find some really basic misstep in the assumptions :(

Rerolling #95 for 8.9.8.

The attached patch is an updated version of #107 that addresses the Cspell failure.

Re-rolled patch against 9.2.x and addressed more custom command failures (block.es6.js was not updated).

Please could you provide a comment here?

+    if (empty($this->configuration['bundles']) && !$this->isNegated()) {
+      return TRUE;
+    }

It'll be clear to read the code

• Added code comment suggested by @RenatoG in #110
• Addressed test failures and deprecations

Whoops, fixed spelling typo.

Should address remaining test failures.

Rerolled the patch in #114, thanks!

Fixed the coding standards issue and updated the deprecation message.

1. +++ b/core/lib/Drupal/Core/Entity/Plugin/Condition/EntityBundle.php
   @@ -0,0 +1,166 @@
   + * Provides a 'Entity Bundle' condition.
   

   guess it should be - Provides an entity bundle condition.

2. +++ b/core/lib/Drupal/Core/Entity/Plugin/Condition/EntityBundle.php
   @@ -0,0 +1,166 @@
   +   * @var \Drupal\Core\Entity\EntityTypeInterface
   ...
   +  protected $entityType;
   ...
   +    $this->entityTypeManager = $entity_type_manager;
   ...
   +    $this->entityType = $entity_type_manager->getDefinition($this->getDerivativeId());
   ...
   +    $bundles = $this->entityTypeBundleInfo->getBundleInfo($this->entityType->id());
   ...
   +    $entity = $this->getContextValue($this->entityType->id());
   

   If the conditions will be serialized better to cache the derivativeId itself instead of the whole definition, moreover this is only usage of injected ETM

3. +++ b/core/modules/block/block.install
   @@ -23,3 +23,24 @@ function block_install() {
   +function block_update_9200() {
   ...
   +  $config_factory = \Drupal::configFactory();
   +  foreach ($config_factory->listAll('block.block.') as $block_config_name) {
   

   better move it to post update hook

4. +++ b/core/modules/node/src/Plugin/Condition/NodeType.php
   @@ -70,7 +76,7 @@ public function buildConfigurationForm(array $form, FormStateInterface $form_sta
   -      '#title' => $this->t('Node types'),
   +      '#title' => $this->t('Content types'),
   

   looks out of scope

5. +++ b/core/tests/Drupal/KernelTests/Core/Entity/EntityBundleConditionTest.php
   @@ -0,0 +1,106 @@
   +    $this->expectDeprecation('Passing context values to plugins via configuration is deprecated in drupal:9.1.0 and will be removed before drupal:10.0.0. Instead, call ::setContextValue() on the plugin itself. See https://www.drupal.org/node/3120980');
   

   why 9.1.0 and I bet message should be 's/will be/is'

1. Was wondering about that too. I went with provides the entity bundle condition to avoid, and there's only one condition plugin/class.
2. Good point, I removed $this->entityType. There was one call to $this->entityType->getBundleLabel() but that's actually the same as the plugin label, so I've just used that instead, like the form already does. Also unsure what to do about the @bundle_type and @bundle in context of that other issue that replaces it with subtype.
3. Yep.
4. Maybe? This is super weird. In BlockForm, the label is then again overwritten to Content types in HEAD, I'm removing that. But I'm keeping the other code there for node type, so we'll need to remove that snippet anyway in D10. Unsure. But you're right, ti's probably better to avoid any not strictly necessary change. Reverted it, lets see if something else breaks.
5. This is actually not the deprecation of this issue, but it's copied from NodeTypeTest. But yes, I think we don't need to copy that into the new class. That deprecation will be removed at the same time as NodeType, so we can keep the test coverage only there.

Also cleaned up the test a bit, fixed assertEquals() argument order and removed $this->manager as it was only used once.

Thank you! looks great, except one question

+++ b/core/lib/Drupal/Core/Entity/Plugin/Condition/EntityBundle.php
@@ -124,14 +104,14 @@ public function summary() {
-          '@bundle_type' => $bundle_type,
+          '@bundle_type' => $this->pluginDefinition['label'],

Is this label translatable?

build fails with

FILE: ...ml/core/lib/Drupal/Core/Entity/Plugin/Condition/EntityBundle.php
----------------------------------------------------------------------
FOUND 0 ERRORS AND 1 WARNING AFFECTING 1 LINE
----------------------------------------------------------------------
 7 | WARNING | [x] Unused use statement

The label is translatable (it is e.g. "Content type", entity type label). But it should work in this case, if not, then the plugin label itself would also not work.

The reason it does work is because it should always be a TranslatableMarkup object that's only "translated" when cast to a string. So we cache the object and translate it when it's actually displayed. Just reviewed #3038717: Block/menu/views derivative labels are always shown in the first language they are view in after cache rebuild, which is one case where this assumption isn't working, but this should IMHO be fine.

Fixed the use. Would be nice if you could do a pre-diff hook to check coding standards ;) Or work with merge requests I suppose.

Oops. Fixed the post update function name.

We can remove this unused else

+      if (empty($this->configuration['negate'])) {
+        return $this->t('@bundle_type is @bundles or @last', [
+          '@bundle_type' => $this->pluginDefinition['label'],
+          '@bundles' => $bundles,
+          '@last' => $last,
+        ]);
+      }
+      else {
+        return $this->t('@bundle_type is not @bundles or @last', [
+          '@bundle_type' => $this->pluginDefinition['label'],
+          '@bundles' => $bundles,
+          '@last' => $last,
+        ]);
+      }

If true it will return in the first conditional. If not it'll enter in the second return. It's not necessary "else" on this case

Example:

From:

if (condition) { 
  return foo; 
}
else {
  return bar; 
}

To:

if (condition) { 
    return foo; 
} 
return bar;

Removed unnecessary else

I'm not so convinced that we should remove that. Yes, it's strictly speaking not required, but with those long return statements, it IMHO helps with readability of the code to see that there is a clear if/else case, especially because there are actually 4 cases in total. But no strong feelings.

1. +++ b/core/modules/node/src/Plugin/Condition/NodeType.php
   @@ -13,11 +13,16 @@
   + * @deprecated in drupal:8.7.0 and is removed from drupal:9.0.0.
   + *   Use \Drupal\Core\Entity\Plugin\Condition\EntityBundle instead.
   

   Should be updated to 9.3.0 and 10.0.0.

2. +++ b/core/modules/node/src/Plugin/Condition/NodeType.php
   @@ -13,11 +13,16 @@
   + * @see https://www.drupal.org/node/2919303
   

   I guess this should link to the change record? https://www.drupal.org/node/2983299

3. +++ b/core/modules/node/src/Plugin/Condition/NodeType.php
   @@ -45,6 +50,7 @@ class NodeType extends ConditionPluginBase implements ContainerFactoryPluginInte
   +    @trigger_error('\Drupal\node\Plugin\Condition\NodeType is deprecated in drupal:9.3.0 and is removed from drupal:10.0.0. Use \Drupal\Core\Entity\Plugin\Condition\EntityBundle instead. See https://www.drupal.org/node/2919303', E_USER_DEPRECATED);
   

   The URL should also link to the change record: https://www.drupal.org/node/2983299

4. I agree with @berdir, the changes made in #128 don't improve readability.

Updated link and drupal version as suggested in #130

Working on it...

I addressed everything pointed in comment #130 and fixed the deprecation test fail in #131.

I tried the patch and it's working for me. All feedback is resolved and there is test coverage, so looks good to me.

+++ b/core/modules/node/src/Plugin/Condition/NodeType.php
@@ -13,11 +13,16 @@
- *   label = @Translation("Node Bundle"),
+ *   label = @Translation("Content type (Deprecated)"),

I think we should maintain the label of the thing we're deprecating so we only add (Deprecated) so that anyone who is used to the UI can identify this as the same as the previous thing called "Node Bundle".

One thought is should we move up a level of abstraction so the can also replace \Drupal\user\Plugin\Condition\UserRole? All of this is about entity reference fields. Obviously we don't want to derive a condition plugin field for every entity reference field but maybe we could add a field storage setting that means we're create a condition plugin derivative for such an entity reference field. We could set this for bundle fields and the roles field on the user and then it'd be a really nice extension point for a module - like you could configure it in the entity reference field storage settings or something like that. OTOH the current implementation works for bundles based on entity references and bundles based on things like state so maybe this is oaky. On the other other hand how many real use cases are there for non-entity bundles?

Restoring the plugin label, but as mentioned in slack, the label was never visible in core anyway, because we overwrote it in BlockForm instead of fixing it at the source :-/ The previous patch had removed those two lines in BlockForm but I restored it earlier without checking, so the (deprecated) wasn't visible. Fixed that and also added explicit test coverage of this.

And, as also mentioned in slack, I don't think that UserRole is the same thing. We can't get completely rid of it anyway, that cache context customization is _very_ important and unique to user role.

We'd need to basically start from scratch on the implementation, the storage definition setting idea would require changes on base field definitions, sounds like a lot of pain for a somewhat theoretical use case. What happens if someone enables that on a ER field pointing to nodes and you have thousands of nodes? We'd need to dynamically switch the UI and.. .. please no? :)

@Berdir good points in #138. Let's proceed here with special casing the bundle. At least it is less special case than the NodeType plugin.

Feedback of #136 is resolved, so back to RTBC.

This is looking pretty good. Can only see one thing that is missing. That's condition.plugin.node_type in node.schema.yml needs to be deprecated as well. Here's an example from core...

    attributes:
      deprecated: "The tour.tip 'attributes' config schema property is deprecated in drupal:9.2.0 and is removed from drupal:10.0.0. Instead of 'data-class' and 'data-id' attributes, use 'selector' to specify the element a tip attaches to. See https://www.drupal.org/node/3204093"
      type: sequence
      label: 'Attributes'
      sequence:
        type: string
        label: 'Attribute'

I added a deprecation property to condition.plugin.node_type

+++ b/core/modules/node/config/schema/node.schema.yml
@@ -91,6 +91,7 @@ block.settings.node_syndicate_block:
 
 condition.plugin.node_type:
+  deprecated: "The 'condition.plugin.node_type' config schema is deprecated in drupal:9.3.0 and is removed from drupal 10.0.0. Use the 'condition.plugin.entity_bundle:node' config schema instead. See https://www.drupal.org/node/2983299."
   type: condition.plugin

You don't really "use" the config schema. So similar to the example, I'd say "Use the entity_bundle:node_type key instead to define a node type condition".

It's a bit special because we deprecate a whole structure but it's still a part of a another thing. In an actual config example, you just have to replace node_type with entity_bundle:node_type, the parts above depend on where it's embedded.

I guess we'd also want to have this message tested. In my new test method, temporarily remove the @group legacy and then you should get the exact deprecation message that this prints (not sure if it's just this or more) and can then add it.

I'll do it tonight if nobody gets to it first.

Updated the deprecation message and added test coverage.

I would expect I'm already triggering the deprecation message by initially saving the block, should trigger if you put it at the top of the method?

Also, by fixing the typo the comment is at 81 characters, maybe leave out the ' to get below that again?

hmm, I probably made a typo in the expectDeprecation message, because now it's working locally.

There is a good practice called Early Return to help our code to be easier to read

You can read more here:
https://medium.com/swlh/return-early-pattern-3d18a41bba8

Example:
From:

function foo($bar, $baz) {
  if ($foo) {
    // Method logic goes here.
    return $calculated_value;
  } 
  else {
    return NULL;
  }
}

To:

function foo($bar, $baz) {
  if (!$foo) {
    return NULL;
  }
  // Method logic goes here.
  return $calculated_value;
}

Based on this, we are using this approach here:
That is Good:

+++ b/core/lib/Drupal/Core/Entity/Plugin/Condition/EntityBundle.php
+  public function evaluate() {
+    // Returns true if no bundles are selected and negate option is disabled.
+    if (empty($this->configuration['bundles']) && !$this->isNegated()) {
+      return TRUE;
+    }
+    /** @var \Drupal\Core\Entity\ContentEntityInterface $entity */
+    $entity = $this->getContextValue($this->getDerivativeId());
+    return !empty($this->configuration['bundles'][$entity->bundle()]);
+  }

We can improve this code here:

+++ b/core/lib/Drupal/Core/Entity/Plugin/Condition/Deriver/EntityBundle.php
+  public function getDerivativeDefinitions($base_plugin_definition) {
+    foreach ($this->entityTypeManager->getDefinitions() as $entity_type_id => $entity_type) {
+      if ($entity_type->hasKey('bundle')) {
+        $this->derivatives[$entity_type_id] = $base_plugin_definition;
+        $this->derivatives[$entity_type_id]['label'] = $entity_type->getBundleLabel();
+        $this->derivatives[$entity_type_id]['provider'] = $entity_type->getProvider();
+        $this->derivatives[$entity_type_id]['context_definitions'] = [
+          $entity_type_id => EntityContextDefinition::fromEntityType($entity_type),
+        ];
+      }
+    }
+    return $this->derivatives;
+  }

It can be impreoved to the same approach to be:

public function getDerivativeDefinitions($base_plugin_definition) {
    foreach ($this->entityTypeManager->getDefinitions() as $entity_type_id => $entity_type) {
      if (!$entity_type->hasKey('bundle')) {
        continue;
      }
      $this->derivatives[$entity_type_id] = $base_plugin_definition;
      $this->derivatives[$entity_type_id]['label'] = $entity_type->getBundleLabel();
      $this->derivatives[$entity_type_id]['provider'] = $entity_type->getProvider();
      $this->derivatives[$entity_type_id]['context_definitions'] = [
        $entity_type_id => EntityContextDefinition::fromEntityType($entity_type),
      ];
    }
    return $this->derivatives;
  }

It'll improve our code to be friendly and easier to ready

This is the patch with this #148 improvement in the code

Not convinced that's clearer. I'm all for early returns in long functions with multiple conditions but this is just a handful of lines and a single condition. This makes it a reverse condition, that's complexity too.

But I don't really care, either version is fine by me, I think the deprecation changes are fine, RTBC'ing that, which I think is OK.

I went for #147 because I find the negation and continue more complex too.

    foreach ($this->entityTypeManager->getDefinitions() as $entity_type_id => $entity_type) {
      if (!$entity_type->hasKey('bundle')) {
        continue;
      }

Committed 0f22eb1 and pushed to 9.3.x. Thanks!