Problem/Motivation

As part of the Condition Plugin api, we need various condition plugins available to be used. This issue is to create a vocabulary condition.

Proposed resolution

Remaining tasks

User interface changes

API changes

Files: 
CommentFileSizeAuthor
#52 2281659-vocabulary-condition-52.patch10.33 KBAnushka-mp
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 80,690 pass(es).
[ View ]
#52 2281659-vocabulary-condition-52.interdiff.txt750 bytesAnushka-mp
#48 2281659-vocabulary-condition-46.patch10.33 KBArla
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 79,396 pass(es).
[ View ]
#46 2281659-vocabulary-condition-46.interdiff.txt1.69 KBArla
#46 2281659-vocabulary-condition-46.patch1.69 KBArla
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch 2281659-vocabulary-condition-46.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#40 2281659-vocabulary-condition-40-interdiff.txt974 bytescbr
#40 2281659-vocabulary-condition-40.patch9.96 KBcbr
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 75,436 pass(es).
[ View ]
#23 interdiff.txt608 byteskim.pepper
#23 2281659-vocabulary-condition-23.patch9.94 KBkim.pepper
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 72,601 pass(es), 0 fail(s), and 1 exception(s).
[ View ]
#22 interdiff.txt956 byteskim.pepper
#22 2281659-vocabulary-condition-22.patch9.34 KBkim.pepper
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 72,596 pass(es).
[ View ]
#17 interdiff.txt593 byteskim.pepper
#17 2281659-vocabulary-condition-17.patch9.37 KBkim.pepper
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 72,511 pass(es), 41 fail(s), and 5 exception(s).
[ View ]
#14 interdiff.txt7.13 KBkim.pepper
#14 2281659-vocabulary-condition-14.patch9.34 KBkim.pepper
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 72,541 pass(es), 2 fail(s), and 2 exception(s).
[ View ]
#10 interdiff.txt1.22 KBkim.pepper
#10 2281659-vocabulary-condition-9.patch8.76 KBkim.pepper
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 72,300 pass(es).
[ View ]
#8 interdiff.txt781 byteskim.pepper
#8 2281659-vocabulary-condition-8.patch8.1 KBkim.pepper
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 72,011 pass(es).
[ View ]
#6 interdiff.txt1.46 KBkim.pepper
#6 2281659-vocabulary-condition-6.patch7.96 KBkim.pepper
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 71,898 pass(es).
[ View ]
#3 2281659-vocabulary-condition-3.patch7.99 KBkim.pepper
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 71,863 pass(es).
[ View ]

Comments

larowlan’s picture

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

kim.pepper’s picture

Discussed this with @tim.plunkett and agreed that #1932810: Replace NodeType Condition to EntityBundle Condition. is a feature, and that it's less likely to get considered at this point. In the meantime, creating plugins is a task, and fairly straight forward. I think it's best to move this discussion to that issue.

kim.pepper’s picture

StatusFileSize
new7.99 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 71,863 pass(es).
[ View ]

Initial implementation based on node type.

kim.pepper’s picture

Status:Active» Needs review
tim.plunkett’s picture

  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.

kim.pepper’s picture

StatusFileSize
new7.96 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 71,898 pass(es).
[ View ]
new1.46 KB

Thanks.

  1. Doh!
  2. Agree. The base class defines defaultConfiguration() as an empty array so we don't need to override it here.
tim.plunkett’s picture

I meant this:

  public function defaultConfiguration() {
    return array(
      'bundles' => array(),
    );
  }
kim.pepper’s picture

StatusFileSize
new8.1 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 72,011 pass(es).
[ View ]
new781 bytes

Ah yes, of course!

Fixes for #7.

tim.plunkett’s picture

+++ 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’s picture

StatusFileSize
new8.76 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 72,300 pass(es).
[ View ]
new1.22 KB

Fixes for #9

jibran’s picture

@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: Replace NodeType Condition to EntityBundle Condition. 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.

kim.pepper’s picture

Hi Jibran,

I believe @tim.plunkett has answered this on #2278541-64: Refactor block visibility to use condition plugins

Kim

Berdir’s picture

Status:Needs review» Needs work

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.

kim.pepper’s picture

Status:Needs work» Needs review
StatusFileSize
new9.34 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 72,541 pass(es), 2 fail(s), and 2 exception(s).
[ View ]
new7.13 KB

Thanks for the review, @Berdir. Fixes for #13.

Status:Needs review» Needs work

The last submitted patch, 14: 2281659-vocabulary-condition-14.patch, failed testing.

tim.plunkett’s picture

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

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

kim.pepper’s picture

Status:Needs work» Needs review
StatusFileSize
new9.37 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 72,511 pass(es), 41 fail(s), and 5 exception(s).
[ View ]
new593 bytes

Fixes #16.

Berdir’s picture

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

Status:Needs review» Needs work

The last submitted patch, 17: 2281659-vocabulary-condition-17.patch, failed testing.

Berdir’s picture

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..)

Berdir’s picture

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

kim.pepper’s picture

Status:Needs work» Needs review
StatusFileSize
new9.34 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 72,596 pass(es).
[ View ]
new956 bytes

Fixes for #20 and #21.

kim.pepper’s picture

StatusFileSize
new9.94 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 72,601 pass(es), 0 fail(s), and 1 exception(s).
[ View ]
new608 bytes

Don't show vocabulary condition on block visibility form.

Status:Needs review» Needs work

The last submitted patch, 23: 2281659-vocabulary-condition-23.patch, failed testing.

tim.plunkett’s picture

+++ 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

kim.pepper’s picture

Status:Needs work» Needs review
StatusFileSize
new9.93 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 72,769 pass(es).
[ View ]
new1001 bytes

Fixes for #25

tim.plunkett’s picture

Status:Needs review» Reviewed & tested by the community

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

jibran’s picture

kim.pepper’s picture

StatusFileSize
new9.96 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 72,875 pass(es).
[ View ]
new743 bytes
tim.plunkett’s picture

StatusFileSize
new9.98 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 75,236 pass(es), 1 fail(s), and 0 exception(s).
[ View ]

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

Status:Reviewed & tested by the community» Needs work

The last submitted patch, 30: 2281659-vocabulary-condition-30.patch, failed testing.

Status:Needs work» Needs review

jibran’s picture

Status:Needs review» Needs work
  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

kim.pepper’s picture

Chatting with @chx in IRC he suggested dropping the name "bundle" and "bundles" in favour of just "vocabulary" and "vocabularies" as it is confusing. Seems like a reasonable request.

Status:Needs work» Needs review
cbr’s picture

Assigned:kim.pepper» cbr
StatusFileSize
new8.72 KB
new10.71 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 75,217 pass(es).
[ View ]

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().

The last submitted patch, 30: 2281659-vocabulary-condition-30.patch, failed testing.

kim.pepper’s picture

Status:Needs review» Needs work

Thanks @cbr.

+++ b/core/modules/block/src/BlockBase.php
@@ -294,7 +294,7 @@ public function buildConfigurationForm(array $form, FormStateInterface $form_sta
-      $form['visibility']['node_type']['bundles']['#title'] = $this->t('Content types');
+      $form['visibility']['node_type']['vocabularies']['#title'] = $this->t('Content types');

This change is incorrect, as it is about content types, not vocabularies.

cbr’s picture

Status:Needs work» Needs review
StatusFileSize
new9.96 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 75,436 pass(es).
[ View ]
new974 bytes

Renamed content type vocabularies to bundles.

kim.pepper’s picture

Status:Needs review» Reviewed & tested by the community

Looks good to me.

Xano’s picture

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.

kim.pepper’s picture

EclipseGC argued against unit tests for the RequestPath condition plugin over here [#1921544#comment-8801739]. Not sure if that still applies here.

alexpott’s picture

Status:Reviewed & tested by the community» Needs work

@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.

undertext’s picture

@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.

Arla’s picture

Status:Needs work» Needs review
StatusFileSize
new1.69 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch 2281659-vocabulary-condition-46.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
new1.69 KB

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

Status:Needs review» Needs work

The last submitted patch, 46: 2281659-vocabulary-condition-46.patch, failed testing.

Arla’s picture

Status:Needs work» Needs review
StatusFileSize
new10.33 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 79,396 pass(es).
[ View ]

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

Berdir’s picture

Status:Needs review» Needs work
+++ 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')

pivica’s picture

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'));
   }

   /**

Anushka-mp’s picture

Status:Needs work» Needs review
StatusFileSize
new750 bytes
new10.33 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 80,690 pass(es).
[ View ]

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

alexpott’s picture

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: Replace NodeType Condition to EntityBundle Condition.?

Berdir’s picture

+++ 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.

Berdir’s picture

Version:8.0.x-dev» 8.1.x-dev
Assigned:cbr» Unassigned
Category:Task» Feature request

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: Replace NodeType Condition to EntityBundle Condition. then?