Create a vocabulary condition

Shouldn't we be refactoring the node type condition into a generic content entity bundle condition? Custom block type also in core but plenty of contrib module examples too

1. +++ b/core/modules/taxonomy/src/Plugin/Condition/Vocabulary.php
   @@ -0,0 +1,116 @@
   +      '#title' => t('Vocabularies'),
   ...
   +      return t('The vocabulary bundle is @bundles or @last', array(
   ...
   +    return t('The vocabulary bundle is @bundle', array('@bundle' => $bundle));
   

   $this->t()

2. +++ b/core/modules/taxonomy/src/Plugin/Condition/Vocabulary.php
   @@ -0,0 +1,116 @@
   +      '#default_value' => isset($this->configuration['bundles']) ? $this->configuration['bundles'] : array(),
   

   No isset needed if you put 'bundles' in defaultConfiguration()

The test looks good.

I meant this:

  public function defaultConfiguration() {
    return array(
      'bundles' => array(),
    );
  }

+++ b/core/modules/taxonomy/src/Plugin/Condition/Vocabulary.php
@@ -0,0 +1,125 @@
+    /* @var \Drupal\taxonomy\TermInterface $term */
+    $term = $this->getContextValue('term');
+    return !empty($this->configuration['bundles'][$term->getVocabularyId()]);

This shouldn't bother trying to get the context value if no bundles are configured. See the changes to NodeType in #2283929: Clean up condition plugins to have schema support and to allow them to be optional for an example.

This should also provide a schema entry, see that issue as well.

@kim.pepper and @tim.plunkett nice work on condition plugins.
Just like #1 I am unable to understand why are we adding and working on redundant code? We can do it in #1932810: Add entity bundles condition plugin for entities with bundles for all bundles. Tomorrow someone will need condition plugin for comment type, feeds or contact form categories those all are in core as well I know contrib can add it but why not contrib add this condition as well this is not used by core and I haven't seen any road map to add it to blocks visibility.

Lots of minor feedback.

Tested this with overriding a term page with page_manager and then displaying different variants based on the vocabulary. Works well with #2287621: Add tests for context mapping for blocks and context fixed.

1. +++ b/core/modules/taxonomy/src/Plugin/Condition/Vocabulary.php
   @@ -0,0 +1,128 @@
   + *     "term" = {
   + *       "type" = "entity:taxonomy_term"
   + *     }
   

   Would be more consistent to name the context "taxonomy_term"?

2. +++ b/core/modules/taxonomy/src/Plugin/Condition/Vocabulary.php
   @@ -0,0 +1,128 @@
   +class Vocabulary extends ConditionPluginBase implements ContainerFactoryPluginInterface {
   

   This is consistent with what we have, but I'm not a big fan of those super-short class names, VocabularyCondition would imho be better if we would chose freely, much easier to find than "Vocabulary" because you then have to pick the class in the right namespace.

3. +++ b/core/modules/taxonomy/src/Plugin/Condition/Vocabulary.php
   @@ -0,0 +1,128 @@
   +   * The entity storage.
   +   *
   +   * @var \Drupal\Core\Entity\EntityStorageInterface
   +   */
   +  protected $entityStorage;
   

   I'd suggest to name this vocabularyStorage?

4. +++ b/core/modules/taxonomy/src/Plugin/Condition/Vocabulary.php
   @@ -0,0 +1,128 @@
   +   * Creates a new NodeType instance.
   +   *
   +   * @param \Drupal\Core\Entity\EntityStorageInterface $entity_storage
   +   *   The entity manager.
   +   * {@inheritdoc}
   

   References the old name...

   Also, @inheritdoc doesn't work like that, you have to duplicate the @param's, it's not an inline placeholder..

5. +++ b/core/modules/taxonomy/src/Plugin/Condition/Vocabulary.php
   @@ -0,0 +1,128 @@
   +    $this->configuration['bundles'] = array_filter($form_state['values']['bundles']);
   +    parent::submitConfigurationForm($form, $form_state);
   

   parent usually comes first?

6. +++ b/core/modules/taxonomy/src/Tests/Plugin/Condition/VocabularyConditionTest.php
   @@ -0,0 +1,131 @@
   +    $this->installConfig(array('taxonomy'));
   

   Why do we need the taxonomy config?

7. +++ b/core/modules/taxonomy/src/Tests/Plugin/Condition/VocabularyConditionTest.php
   @@ -0,0 +1,131 @@
   +      'vid' => drupal_strtolower($this->randomName()),
   

   use Unicode::strtolower() ?

8. +++ b/core/modules/taxonomy/src/Tests/Plugin/Condition/VocabularyConditionTest.php
   @@ -0,0 +1,131 @@
   +      'langcode' => Language::LANGCODE_NOT_SPECIFIED,
   ...
   +      'langcode' => Language::LANGCODE_NOT_SPECIFIED,
   

   Should use the interface (LanguageInterface)

9. +++ b/core/modules/taxonomy/src/Tests/Plugin/Condition/VocabularyConditionTest.php
   @@ -0,0 +1,131 @@
   +      'weight' => mt_rand(0, 10),
   

   mt_rand() in tests has been problematic before, unless we need it somehow for the test, I would just leave it out, it's not necessary to specify it explicitly.

10. +++ b/core/modules/taxonomy/src/Tests/Plugin/Condition/VocabularyConditionTest.php
    @@ -0,0 +1,131 @@
    +    $term = entity_create('taxonomy_term', array(
    

    You can use Term::create() for this now.

+++ b/core/modules/taxonomy/src/Plugin/Condition/VocabularyCondition.php
@@ -0,0 +1,136 @@
+      'bundles' => array(),
+    );

This should end ) + parent::defaultConfiguration();

Update looks good to me, RTBC from me but @timplunkett should probably confirm the name change and if he's OK with that.

Actually...

+++ b/core/modules/taxonomy/src/Plugin/Condition/VocabularyCondition.php
@@ -92,7 +92,7 @@ public function buildConfigurationForm(array $form, array &$form_state) {
     return array(
-      'bundles' => array(),
+      'bundles' => array() + parent::defaultConfiguration(),
     );

That's the wrong, default configuration needs to be added to the top level array, not to bundles.

This is currently causing a notice on the block page (where this now automatically shows up..)

Also, the vocabulary checkboxes field is required, that probably breaks most of those tests, because it can't save block settings anymore.

+++ b/core/modules/block/src/BlockBase.php
@@ -316,6 +316,8 @@ public function buildConfigurationForm(array $form, array &$form_state) {
+    unset($form['visibility']['vocabulary']);

Hmm, I was wrong about doing this here. It should be done up in baseConfigurationDefaults, before its ever used for the form.

The reason the test fails is because it's still being called in submit

Nothing left to do here, IMO.
Thanks @kim.pepper!

I have added #2288861: Add vocabulary condition to block visibility.

Rerolled because the current theme condition also was unset()ing itself in the same spot.

1. +++ b/core/modules/taxonomy/src/Plugin/Condition/VocabularyCondition.php
   @@ -0,0 +1,134 @@
   + *     "taxonomy_term" = @ContextDefinition("entity:taxonomy_term", label = @Translation("Taxonomy Term"))
   

   Can we make it multi line?

2. +++ b/core/modules/taxonomy/src/Plugin/Condition/VocabularyCondition.php
   @@ -0,0 +1,134 @@
   +    /* @var \Drupal\taxonomy\VocabularyInterface[] $vocabularies */
   

   Doesn't it start with /**?

3. +++ b/core/modules/taxonomy/src/Tests/Plugin/Condition/VocabularyConditionTest.php
   @@ -0,0 +1,132 @@
   +  public static function getInfo() {
   

   Removed now.

4. +++ b/core/modules/taxonomy/src/Tests/Plugin/Condition/VocabularyConditionTest.php
   @@ -0,0 +1,132 @@
   +    // Assert term1 is in in either vocab1 or vocab2.
   

   in in

Rerolled and reviewed.

@jibran: I changed all your suggestions except 1, didn't find another example of ContextDefinition using multi line.
@kim.pepper: Changed bundle & bundles to vocabulary and vocabularies.

Did some minor updates i.e. added FormStateInterface to $form_state and renamed randomName() to randomMachineName().

Renamed content type vocabularies to bundles.

Why wasn't PHPUnit used to write the unit test? If there is no technical reason against it, we should probably set this issue back to needs work so the test can be converted.

@Xano: Well once KernelTestBase(ng) lands it will be a PHPUnit test :)

1. +++ b/core/modules/taxonomy/src/Plugin/Condition/VocabularyCondition.php
   @@ -0,0 +1,135 @@
   +  public function summary() {
   +    if (count($this->configuration['vocabularies']) > 1) {
   +      $vocabularies = $this->configuration['vocabularies'];
   +      $last = array_pop($vocabularies);
   +      $vocabularies = implode(', ', $vocabularies);
   +      return $this->t('The vocabulary bundle is @vocabularies or @last', array(
   +        '@vocabularies' => $vocabularies,
   +        '@last' => $last,
   +      ));
   +    }
   +    $vocabulary = reset($this->configuration['vocabularies']);
   +    return $this->t('The vocabulary bundle is @vocabulary', array('@vocabulary' => $vocabulary));
   +  }
   

   I think we need to check if the condition is negated to produce the correct summary test.

2. +++ b/core/modules/taxonomy/src/Plugin/Condition/VocabularyCondition.php
   @@ -0,0 +1,135 @@
   +    if (empty($this->configuration['vocabularies']) && !$this->isNegated()) {
   +      return TRUE;
   +    }
   

   I'm not sure I get this logic - if we don't set a vocabulary then all terms match. If you tests and call the summary method then the result is quite weird.

3. +++ b/core/modules/taxonomy/src/Tests/Plugin/Condition/VocabularyConditionTest.php
   @@ -0,0 +1,123 @@
   +  /**
   +   * Returns a new vocabulary with random properties.
   +   */
   +  protected function createVocabulary() {
   ...
   +  /**
   +   * Returns a new term with random properties in vocabulary $vid.
   +   */
   +  protected function createTerm(VocabularyInterface $vocabulary) {
   

   PHPDoc blocks for these functions need @param and @return as appropriate.

@alexpott : The logic of summary() and evaluate() methods is taken from NodeType condition. (\Drupal\node\Plugin\Condition\NodeType)
So we should create a new issue and fix NodeType condition too.

Rerolled and fixed some doc in VocabularyConditionTest, including #44.3.

Meh, last .patch was the interdiff. This is right.

+++ b/core/modules/taxonomy/src/Plugin/Condition/VocabularyCondition.php
@@ -0,0 +1,135 @@
+  public function submitConfigurationForm(array &$form, FormStateInterface $form_state) {
+    parent::submitConfigurationForm($form, $form_state);
+    $this->configuration['vocabularies'] = array_filter($form_state['values']['vocabularies']);
+  }

This should be $form_state->getValue('vocabularies')

For #51 here is a correct change

+++ b/core/modules/taxonomy/src/Plugin/Condition/VocabularyCondition.php
@@ -100,7 +100,7 @@ class VocabularyCondition extends ConditionPluginBase implements ContainerFactor
    */
   public function submitConfigurationForm(array &$form, FormStateInterface $form_state) {
     parent::submitConfigurationForm($form, $form_state);
-    $this->configuration['bundles'] = array_filter($form_state['values']['bundles']);
+    $this->configuration['bundles'] = array_filter($form_state->getValue('bundles'));
   }
 
   /**

$form_state['values']['vocabularies'] changed to $form_state->getValue('vocabularies')

This issue is a normal task that adds new functionality, and per #2350615: [policy, no patch] What changes can be accepted during the Drupal 8 beta phase?, we should discuss the priority of this issue and whether or not we should postpone until 8.1.x

Also is this not a feature given #1932810: Add entity bundles condition plugin for entities with bundles?

+++ b/core/lib/Drupal/Core/Block/BlockBase.php
@@ -112,7 +112,10 @@ protected function baseConfigurationDefaults() {
       return array('id' => $definition['id']);
     }, $this->conditionPluginManager()->getDefinitions());
+
+    // Exclude conditions that don't apply.
     unset($visibility['current_theme']);
+    unset($visibility['vocabulary']);

The only problem right now is this, only core can exclude certain conditions from the UI right now.

I actually need this feature, but I don't really care if it is in core or not. So if you don't want to add it to 8.0.x, then move the issue to 8.1.x and we'll start a small contrib module if people are interested or keep it in a custom module for now.

Ok, I agree that it is too late for this for 8.0.x.

As I am actually using this as mentioned, I created https://github.com/md-systems/vocabulary_condition

That's easier to track than a core patch until we can re-consider this for 8.1.x, but maybe we'll do #1932810: Add entity bundles condition plugin for entities with bundles then?

+++ b/core/modules/taxonomy/src/Plugin/Condition/VocabularyCondition.php
@@ -0,0 +1,135 @@
+   * Creates a new NodeType instance.

The NodeType is a lie. Let's just leave out the class name here.

Is there a reason why we do not provide a generic entity condition, or separate plugins using derivatives?

Before discovering this issue I built this functionality based on the NodeType condition. I went ahead and created a sandbox project for it: Vocabulary Condition.

It was only a few comments ago (#55) that another project was linked to for this functionality, and I hate to throw another in the mix instead of collaborating, but that was 2 years ago and that project no longer works. Neither does another alternative I came across, Entity Block Visibility.

I also hate to throw yet another project up instead of contributing a core patch, but again, after a year or two, the desire and direction for a core patch for this is unclear to me at this point. If a core committer were to say "yes, the approach taken in your project is something we want in core (or close to it)", I can take the time to create a patch for that (I'll need to add tests). Otherwise, if nothing else, perhaps others wanting this functionality will see this issue and be lead to the sandbox.

Closing as duplicate of the issue mentioned in #55.

ctools for example has generic entity bundle visibility conditions built in and it works fine.