Problem/Motivation

Provide a condition plugin system and unify before incompatible contrib APIs (CTools + Rules)

Proposed resolution

The implementation adds a condition system below \Drupal\Core\Condition, whereas general aspects factored out into \Drupal\Core\Executable. This can serve as common base for conditions and action plugins. This does not include "processors" yet, which are plugins that process data before it is passed to individual executables - think of token running token replacements. This is in particular necessary for actions, so it would be better flushed separately. Action API refactoring will happen at #1846172: Replace the actions API.

Follow-ups tasks

User interface changes

None. Conditions come with the possibility to provide a simple configuration form for their configuration options, which is to be embedded by the caller. Some usage of that can be seen at #1912452: Condition Group User Interface.

API changes

Well, it adds a new condition API :-) No changes to existing APIs.

Original report by EclipseGC

Blocks have a notion of visibility right now that we need to continue supporting, however the existing solution is completely ignorant of any sort of context, and needs to largely be rebuilt. As a replacement for this, a Condition Plugin type needs to be created, along with supporting condition plugins. The first pass of this could utilize data-source-less plugins that can rely on things like the request object (for example, determining which page your on to allow for blocks to appear on blog/* or something similar).

CTools' Access plugins are a virtually identical to what we want here. Rules & Context modules have a very similar analog. All 3 of these patterns are virtually interchangeable, and core has many use cases for this same tool. Blocks & Layouts absolutely needs this as well.

Eclipse

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

fago’s picture

Great! Dumping my thoughts about how this should work here:

Obviously, conditions (or actions) should be plugins. Then, I think the definition should look somehow as it does for Rules now, in particular it needs to describe the required parameter. E.g. this is how it looks in Rules now:

$items['node_is_of_type'] = array(
    'label' => t('Content is of type'),
    'parameter' => array(
      'node' => array('type' => 'node', 'label' => t('Content')),
    ),
    'group' => t('Node'),
    'help' => t('Evaluates to TRUE if the given content is of one of the selected content types.'),
    'base' => 'rules_condition_node_is_of_type',
  );

I guess most of it will be covered by plugins already, so parameters really is what we need to take care of. Ideally, this should work inline with the way we describe entity properties. See #1696640: Implement API to unify entity properties and fields.

EclipseGc’s picture

How to:

  1. Create a module to hold this plugin type, or identify an existing module that would be a good fit.
  2. Create the Plugin Manager class for Conditions. Utilize AnnotatedClassDiscovery, DerivativeDecorator and the DefaultFactory.
  3. Reuse the Plugin UI (to be built) to display Condition plugins administration.
  4. Add to the block.admin.inc the ability to set visibility settings on the block from the main administration & remove the old visibility settings in the main block configuration form.
  5. Write the condition plugins that will replace the various visibility settings and put them in their corresponding modules.
amateescu’s picture

Status: Active » Needs review
FileSize
7.57 KB

The attached patch provides the initial structure for this plugin and an example of how a condition will be implemented.

I'm posting it in this state because I'll be away for at least one week and maybe someone wants to take it over :)

klonos’s picture

jide’s picture

Nice to see this first stab.

I know the implementation is just an example, but I'm worried by the way this is implemented.
Will we implement all kinds of little plugins providing precise cases like "node is of type" ?

Couldn't we imagine using a more generic approach like :
entity -> of type "node" -> whose bundle is "blog" ?

And we could go even further :
entity -> of type "node" -> whose bundle is "blog" -> and has a property "title" which contains "This is a test" etc.

Obviously, that's more work on the UI side, but it's worth it IMHO.

I've always thought that a lot of Drupal paradigms around these definitions have something wrong when we have to declare in code things we already know through APIs.

jide’s picture

Ahem... I realize @fago mentionned the forthcoming Entity Property API and how the Condition Plugin System should play nice with it, but I'm still curious about how all of you see this on the plugins implementation side, and how generic this could eventually be.

Gábor Hojtsy’s picture

If this is made pluggable / reusable elsewhere, it could become a really useful system for things like #1787942: Allow assigning layouts to pages.

EclipseGc’s picture

That is 100% the intent. I already have a plugin UI in the #1535868: Convert all blocks into plugins patch with the intent of reusing it here and allowing for very generic condition groups to be used across the entire system.

Eclipse

jide’s picture

Great !

EclipseGc’s picture

Issue tags: +Blocks-Layouts

Tagging

mitchell’s picture

Issue tags: +Plugin system

I searched for a library that provides type-checking and found Underscore.php. I think it makes sense to use this, because TypeData API doesn't provide this required functionality.

The library doesn't add support for 'node is of type' but it has many operations that are useful in conditions and condition groups, like isDate, isString, isEmpty, isNull, ..., union, intersection, and difference, and more. For the proposal, see #1803734: Add Underscore.php to Core or another fluent array manipulation wrapper library, and for a Rules use case, see #1809470: Use Underscore.php for data and list operations.
---

The "AND" operation in #5, when implemented in condition groups, could be built similar to Rules' evaluation engine or using Underscore's intersection (logical conjunction). "OR" would be union or (logical disjunction). (Edit: linked to set-theoretic functions.)

In Rules, condition groups are called, "components", which I'm hoping that despite the difficulty in arranging to call these plugin-instances "components", it will considered. IMHO, it could transform Drupal's 'site builder' interface paradigm into that of an 'application composer', where components are built and streamed together within the admin UIs.

I find the prospect of using Plugin UI and Entity Forms (with ConfigEntity) very interesting as a foundation, and I'm sure others who have worked on meta-module UIs in D7 would agree. Perhaps with Backbone.js and jQuery UI, we could more easily build graphical interfaces, as well.
----

Also, beyond the obviously interest from a Rules perspective, the condition plugins are also being mentioned as having uses in additional contexts. Perhaps not far from #7, is an idea I've had about using Rules + Conditional Rules, or something like it, when entities are built to create "complex entities" & "dynamic fields". For example: 'on user profile edit form view, if request-method is [x], and user is of group [y], then add field [z] to form [a], otherwise use default value of [v].'

chx mentioned in his blog at http://www.drupal4hu.com/node/338 that "Back in Paris bojanz had this crazy plan of unifying the condition interfaces of DBTNG, EFQ and Views." Perhaps with #1763974: Convert entity type info into plugins and 'if plugins are compiled to dependency injection's cache', this could all work at bootstrap.

With those ideas in mind, I asked about plugins being able to have routes in #1801570: DX: Replace hook_route_info() with YAML files and RouteBuildEvent. I think having these conditions' results as executables on knowable URLs would help with building something like Rules in D8. We could then leverage the Kernel as an evaluation framework instead of porting Rules' state and data evaluation engine.

fmizzell’s picture

Issue tags: -Plugin system

@mitchell I am not very familiar with underscore.php/js, but I am not sure that I understand what "TypeData API doesn't provide [type checking]" means. We have classes that represent our primitives, so we could easily require that data of a given type is used.

function operation(Integer $x, Integer $y){}

But to have a general conditions system, I guess that we need the Type Data API to support comparison operators, so we could simply do

$i = new Integer(8);
$bool = $i.isEqual(new Integer(12));

So, lets say that we have a condition "node is of type article", our general coditions system will do the following:

//this will get us the node type, which is a string primitive
$param1 = magicthatknowshowtogetstuffoutofentities($node, "type");
$param2 = new String("article");

//here comes the is part of the condition
return $param1.isEqual($param2);

I guess the point is that we don't want to have to create a plugin for every type of question that we want to ask the system, but instead we want that magic method/subsystem that knows how to retrieve the necessary info from our entities or from any other complex data that is using the property api system to expose the structure of its data, and maybe an extension to the typed data api to allow for comparisons. So it sounds to me like we need a way to store the definitions of "conditions", but not necessarily what the current code does: allow the retrieval of code that was written representing a condition.

fago’s picture

EclipseGc’s picture

FileSize
24.77 KB

OK, this patch provides what I think is likely to be a good starting point here. It provides a UI, depends on #1886616: Multistep Form Wizard and has a working API at its core.

Next steps:

We need to separate the UI from the API. I'd like to keep the plugin type in the conditions module. We just need to come up with a logical way of removing the UI and putting it in a follow up patch.

Only node_type has been converted, and we likely should stay with just that on this first run through.

Conditions should be unit testable, so we need some unit tests on the current plugin.

Eclipse

podarok’s picture

#14 looks good

xjm’s picture

Let's add those tests. Also, there are some whitespace issues. Leaving at NR though for architectural review. Hopefully @sun will approve of my use of his tag. ;)

EclipseGc’s picture

I didn't realize that was the spell to summon him. Duly noted.

Eclipse

sun’s picture

Component: other » plugin system
effulgentsia’s picture

Category: task » feature
Priority: Normal » Major
EclipseGc’s picture

FileSize
18.33 KB

Ok, no tests, but this is the basic api. I think I managed to remove all the UI components. I'll post those in a follow up.

This provides a node_type condition implementation. The form validate and submit components are all there but won't be usable until the UI is in. This patch depends on (and slightly modifies) #1896076: Contextual Plugins and supporting code. I'm not 100% sure of that modification as of yet and am waiting for feedback from the TypedData experts, but for the sake of this patch in its current state, it works.

If you had an existing node and wanted to validate that that node was an article, the condition api could be invoked as follows:


$is_article = condition('node_type')
    ->setConfig('bundles', array('article'))
    ->setContext('node', $node)
    ->execute();

There are a number of other ways this same condition could be invoked, but this is the clearest and easiest to read.

Eclipse

Status: Needs review » Needs work

The last submitted patch, condition-api.patch, failed testing.

EclipseGc’s picture

This is still dependent upon #1896076: Contextual Plugins and supporting code (which I'm hoping is committed very very soon). Added in execute tests for node_type condition. I need to add summary tests and then we need to expand the docs on processors and probably add one of those (or fago needs to concede that we can add it later, I have no opinion since I've not used it, so I need him to weigh in here and give guidance).

These patches aren't going to work until #1896076: Contextual Plugins and supporting code lands, so if you want to play with this stuff you need to be applying that first.

This is up to date with that patch as of comment #56.

Eclipse

EclipseGc’s picture

Status: Needs work » Needs review
FileSize
1.9 KB
21.87 KB

Added some summary tests and got this patch working against comment #59 in #1896076: Contextual Plugins and supporting code

Eclipse

Status: Needs review » Needs work

The last submitted patch, condition-api.patch, failed testing.

fago’s picture

+++ b/core/modules/node/lib/Drupal/node/Plugin/Core/Condition/NodeType.php
@@ -0,0 +1,93 @@
+class NodeType extends ConditionPluginBase {

Should be "NodeTypeCondition" for making sense of the class name alone.

Architecturally, let's discuss some points and figure out to deal with them. Not necessarily in this patch, but just to have a plan:

We previously had something as the following pseudo-code:

  $action = $action_manager->createInstance('name')
    ->setArgumentBySelector('node', 'user2')
    ->setArgument('text','new title')
    ->execute();

I guess we'd have to extend the context classes for supporting selectors?

This is what we had for processors:

  list($variable1, $variable2) = $action_manager->createInstance('name')
    ->setArgumentBySelector('node', 'node.referenced_node.entity')
    ->setArgument('text','new title')
    ->addProcessor('text', 'token') // done automatically somehow, or by the form code?
    ->addProcessor('date', 'offset', '+1 day') // done by the form code?
    ->execute();

I think we should implement the token processor as first core-example.

  $form = $action_manager->form($action);
  $action_manager->formValidate($action, $form, $form_state);
  $action_manager->formSubmit($action, $form, $form_State);

We need form generation to be pluggable/overridable such that Rules can squeeze all in its features afterwards.

Also, do we need a conditions module or shouldn't it be just an (core) API component?

   * @see \Drupal\condition\ConditionInterace::form()
   * @see \Drupal\condition\ConditionInterace::submit()
   */
  public function validate($form, &$form_state);

We should base validation upon typed-data validation and do form-validation based upon that. So we can validate any provide plugin-configuration is correct. This could also be the base of a sanity-check to verify configurations are still valid after module enable/disable.

Let's discuss :-)

fago’s picture

Issue tags: -Needs tests

Note: working on it :)

fago’s picture

\Drupal\Core\Processor\ProcessorManager

Should we enable processors to work generally with contexts or tie them executables only? I guess there is no reason to tie them to executables only, so what about moving it to Drupal\Core\Plugin\ContextProcessor ? -> they really just process context values.

fago’s picture

FileSize
9.38 KB

ok, discussed quite a while with EclipseGC on IRC. We figured out already quite some details, we need to discuss form-generation more though.

Here is a first interdiff which takes care of moving execution control to the managers. I'll take a stab on validation afterwards.

Note: I've started using Git for rolling this patches, and just added it to my entity-api sandbox:
http://drupal.org/sandbox/fago/1497344

Branches:
condition-base (8.x base)
condition-fago
context (depending patch)

Feel free to use condition-yourname branches there for any changes.

fago’s picture

Status: Needs work » Needs review
FileSize
53.99 KB
61.41 KB
33.48 KB

ok, so finally here is working patch with validation re-worked based upon symfony validator.

Other changes:
- It turned out the 'bundles' parameter did not use the context yet. I fixed that, what resulted in problems as typed data validation is not yet able to properly validate that due to missing generic list-classes and choice validation. Turned out fixing that is too much to incorporate here, so moved that to its own issue: #1913334: Support choice based validation
- So, for being able to make any interim context definition that validates for the bundles context, I've introduced an "any" data type which validates with anything. Having such a data available is generally a good idea imo. For adding a generic list type that would allow us to do a more specific context definition here I've added #1913328: Provide general list and map classes.
- I've added @throws documentation to the methods throwing exceptions, however it turned out it is pretty nasty having them all throwing exceptions when a value should be pre-configured during configuration time. For now I've re-worked and documented getContextValues() to not throw exceptions so you can use that during configuration time. However, we still miss a way to initially set a value without firing validation, what would be needed in extractFormValues(). We could support getting empty (NULL) context objects if they are not set and just throw the exception in getContextValue() ? Or we remove auto-validation from the plugin-base setContextValue()?

Note: This probably needs an update in UI code to use extractFormValues() and trigger form errors for validation violations from validate(). They do not contain correct property paths yet (code has a todo), I think this can be taken care of as a follow-up.

EclipseGc’s picture

#23: condition-api.patch queued for re-testing.

podarok’s picture

+++ b/core/modules/condition/lib/Drupal/condition/Plugin/ConditionInterface.phpundefined
@@ -0,0 +1,43 @@
+  public function setNegate($boolean);
+
+  /**
+   * Determines whether condition result will be negaeted.

a small typo negaeted.

fago’s picture

However, we still miss a way to initially set a value without firing validation, what would be needed in extractFormValues(). We could support getting empty (NULL) context objects if they are not set and just throw the exception in getContextValue() ? Or we remove auto-validation from the plugin-base setContextValue()?

ok, after letting this settle a while, I think we should do the following:
- Have $context wrapper instances even if the context is not set, but have a NULL value in this case. This would be inline with what typed data (and PHP) does, NULL means not set. However, having the objects available always results in simpler code and allows us to do the following:
- Use $context wrapper for accessing context values without any auto-magic exceptions or validations - but have simple API methods for every operation, i.e. get, set, validate. Those don't throw exceptions if validation fails or the context is unset, so we can use this API safely during configuration-time of the plugin.
- Have all the auto-magic exceptions and validation logic in $plugin context methods like $plugin->getContextValue() and setContextValue(), such that you can simply rely on them during execution time and everything unexpected throws an exception.

Makes sense?

fago’s picture

Next, I gave the context mapping case some thought also. What about this:

  • Context mapping should not be the business of the context aware plugin, but introducing a separate mapping layer just for that seems to be too much to me either. Instead, I think baking the mapping-logic into the $context wrapper objects makes sense.
  • For context mapping we need to have parent $contexts available. For that we could use the parent context-aware plugin - however sometimes you won't need a parent plugin while you still need a parent context. Consider a vbo executing a single action, you just want to define a single entity as context and have your action configured from that. Thus, I'd suggest to rely on the $context_s_ array as representation of the parent context only, as this could be manually created also.
  • Also, I think it would be architecturally very clean if we could have the child-plugin working without having any knowledge of the parent plugin itself or in its context objects. So instead of passing a reference on the parent plugin in the context wrappers for mapping, we can simply pass in the parent $contexts and have it nicely decoupled. (This made me slightly worried of having to pass on a lot of contexts just for instantiating the objects, but well - you do not have to instantiate child plugin classes if you might not use/execute them.)
  • So then the question is how we introduce the parent-context and mapping notion in the ContextInterface. Should it be an extended interface, e.g. SelectableContextInterface or just bake it in? I think we could just bake it in and throw an exception if someone tries to select/map a context without having the necessary parent context specified.
  • Terminology: Should we talk about mapping context or selecting context? API-wise mapping contexts makes sense, while for the exposed API and UI-wise "selecting context" makes sense to me. In particular for the UI "mapping context" seems to be too technically to me as imho you just think about selecting some data to pass on, i.e. you select a context value. Thus, I'd tend to use "selectContextValue()". Thoughts?
fago’s picture

ok, talked through some details and differences of our understanding with EclipseGC on skype. Conceptually, Rules works different of what EclipseGc has in mind in that it treats all parameters as the same while EclipseGc wants to clearly separate values stemming from context and configuration.

In order to move on here we agreed upon:

  • Adding typed data definitions as metadata for configuration values just as for context.
  • Adding a way to pass in context and configuration values that by-pass run-time validation to the condition plug-in during construction.
  • Add test-coverage for that and figure out further details on the way.

For validation we'd agreed upon leaving plugin-level configuration validation out for now. This is something Rules 2 already does and so will continue to do in Drupal 8, but maybe this could be covered via config-entity validation already.

This, together with the already discussed possibilities to

  • hook into execution of individual executable plugins
  • hook into form generation of executable plugins

always Rules to continue its job in contrib, but based upon *the same* condition system. That way we would reach the ultimate goal to avoid ending up with to separate conditions API which developers have to choose from.

It would probably

  • leverage the existing condition API for executing conditions but build an additional and more powerful API for configuring the execution of conditions/actions/rules
  • build its own UI for managing rules, while integrating usual conditions and actions
  • override executable's configuration forms to provide the usual Rules data selection widget for contexts
  • verride executable's configuration forms to continue to allow switching input modes for configuring context/configuration/parameter values or adding input processor options. Possibly only via an opt-in basis so we know how to replace which form values.

After writing this I end up seeing a potentially problematic issue we did not discuss during the call: Processors. If conditions and actions written end-up having to care about stuff like token replacements theirself, Rules won't be able to apply it's pluggable processors without having stuff like that done twice. I guess we could implement a processor for tokens that gets configured via the regular plugin configuration?

Also, I still hope/think that the result of this ends up being usable for actions also, such that we can reproduce the same API for actions: #1846172: Replace the actions API

EclipseGc’s picture

FileSize
19.58 KB
22.23 KB

as Fago mentioned, we had a long conversation today. This patch does not contain everything we talked about and is just an incomplete hybridization of my code and his. I will continue this tomorrow pushing towards the details that fago laid out above. I'm doing this for myself so that I know all the changes that he's made and why I adopted them.

In this patch a lot of code was moved around. I abandoned the condition module in favor of Drupal\Core\Condition which is probably the appropriate thing to do here. I embraced a few things fago has done like the ExecutableManagerInterface and moving execute() up into the manager. The primary reason for this is to that system can take over these high profile methods in certain situations.

Yet to embrace:

Moving the validation of context around. I'm pretty much on board with fago at this point. What we specced was that constructor injection will not validate, and method injection will validate. We both essentially get what we want this way. I'm still reading through his code on this and will need to do a little additional hybridization in order to meet the goals set forth here.

Need to document the configuration's data type definition in the annotation. This shouldn't add significantly to most condition annotations and most only have one or two fields. This may get more interesting in more complicated scenarios. I think this deserves a bit more discussion in the long term, but in the short term it makes sense to flesh it out and see where this goes.

I didn't include every little bit of everything in fago's previous patches. This doesn't mean I'm rejecting it, it just means I'm trying to make sure I understand everything, and there are definitely some pieces of code that we would both agree don't belong any longer. I'm just trying to make sure I weed that stuff out appropriately. Again this is in a transitional state.

The interdiff is from my patch in #23.

Eclipse

Status: Needs review » Needs work

The last submitted patch, condition-api.patch, failed testing.

fago’s picture

+++ b/core/lib/Drupal/Core/Executable/ExecutableInterface.php
@@ -35,4 +35,62 @@ public function getProcessors();
+   * @see \Drupal\Core\Executable\ExecutableInterace::submit()
+   */

This should be called formValidate() so it clearly differs from validation not tied to forms which usually is called validate() already, e.g. we'll have $entity->validate() which is not form validation.

In this patch a lot of code was moved around. I abandoned the condition module in favor of Drupal\Core\Condition which is probably the appropriate thing to do here.

Yes! Good move, thanks.

I guess we could implement a processor for tokens that gets configured via the regular plugin configuration?

So what about that?

sun’s picture

Meh, as predicted... I already mentioned in #1913084-20: Introduce a Form interface for building, validating, and submitting forms that the new FormInterface should use buildForm(), validateForm(), and submitForm() method names, so that they are self-descriptive and do not clash with other build() and validate() method names. The ExecutableInterface here should really just re-use the FormInterface.

EclipseGc’s picture

@sun

I've used the FormInterface pretty heavily thus far and I have some reservations about plugins in general extending it. They may be unwarranted, but I'd like to experiment some more before we just make that a blanket suggestion. Still renaming the method names is probably appropriate in this case. I'll play a little more and see where this goes.

Odd that we got so many failures. They all look random to me except for the actual node type condition test, and that passes locally for me so... weird.

Eclipse

fago’s picture

After thinking more about whether Rules could work without switching input modes and so configuration metadata, I think it would work in some simple cases like some optional boolean options of an action/condition. However, there are use-cases where Rules needs it: For example the "Send an e-mail" action: Usually message subject or message text are pre-configured by the users (and include tokens), but Rules allows you to switch input modes and pass in another e-mail text from your context, e.g. from some message-template entities maintained outside of rules. Having this possibility without the need of creating another action for that is really was makes Rules so flexible and powerful.
So no, we won't get away without that.

@EclipseGc: So in the above example, the message mail text being pre-configured but having token replacements / processors, would that be context or configuration for you?

EclipseGc’s picture

It's configuration, but there's nothing preventing you from injecting configuration at run time. In fact you are very welcome and encouraged to do so if that fits for your use case. You don't HAVE to configure at creation time, and you can override existing configuration at any point before execution, so it's really just a matter of which method you use to do it. Since your example is saying "something outside rules can invoke the rule and then override its configuration" I'm 100% ok with saying plugins can handle that today. as an example, assuming rules is ultimately a configuration entity, and that entity has an actions property that contains preconfigured plugins you want to fire:


  $actions = $rule->get('actions');
  if (isset($actions['send_email']) {
    $plugin = $actions['send_email'];
    // 'message' was already configured to whatever the rule configuration
    // said it should be, the next line reconfigures it to something specific 
    // to the implementation
    $plugin->setConfig('message', $message);
  }

The plugin's now updated, so when the rule fires, the email message will be what the implementation specified. I assume there'd be a $rules->execute() or something at some point that checks conditions and fires actions. Long as this happens first, you should be golden.

Eclipse

EclipseGc’s picture

Alrighty, this is a LOT of changes, but I think we're actually closing in on the mark now. This allows for opt-in documentation of the configuration values in the plugin definition. The node_type plugin can't use it yet since it requires a list and I haven't seen any plugin in typed data that handles that. In scenarios such as these validation of the configuration value will automatically pass.

Configuration validation is done the same way context validation is. This will only happen when using the setConfig() method. Constructor injected configuration and contexts will bypass validation completely. Setter injection will cause validation to be run before the context/config key specified is set.

I removed all processor stuff. It's completely not fleshed out yet and it has wider implications for any plugin that might want to use context processing. I think we can totally justify doing it in a follow up task.

I've provided two interdiffs: one from the patch in 35 and another from the patch in 23 (just to see how this has changed). This adopts all of fago's changes that moved the automated validation from the Context classes to the ContextAwarePluginBase classes. I think this is pretty close to where we need to be to satisfy everyone's concerns. I don't have testing coverage on a few things in this yet, but in general it should work. I am completely baffled by why the patch in 35 failed, it passes locally so... w/e I guess. We can track that down next.

Eclipse

EclipseGc’s picture

Status: Needs work » Needs review
EclipseGc’s picture

OH! this main patch is also dependent upon the patch in #38 of #1913084: Introduce a Form interface for building, validating, and submitting forms

Status: Needs review » Needs work

The last submitted patch, condition-api.patch, failed testing.

EclipseGc’s picture

No surprise there since it depends on another patch.

Eclipse

sun’s picture

Priority: Major » Critical

It's unusual to bump a feature request to critical, but it is my sincere belief that the lack of these condition plugins will only cause us to duct-tape and doctor around architectural limitations for the coming 1-2 years until release.

The condition plugins in combination with the recently committed context plugins will allow us to solve architectural challenges that would be very hard to solve otherwise. I'm primarily thinking of #1839516: Introduce entity operation providers and similar issues. That may seem to be a far stretch based on this issue's perspective, but from what I can tell, these issues are complementary to each other from an architectural perspective.

I think we absolutely need this facility in order to make (more) sense of D8.

jthorson’s picture

Status: Needs work » Needs review

Bot is re-testing #42 now ... the patch apply message was a bot failure.

Status: Needs review » Needs work

The last submitted patch, condition-api.patch, failed testing.

andypost’s picture

2 broken tests needs re-roll

EclipseGc’s picture

Included fago's updates to the context tests I missed. Also one patch is with the form interface and the other is without. hopefully the one with will pass all tests now.

Eclipse

EclipseGc’s picture

Status: Needs work » Needs review
fago’s picture

I worked over the patch and mostly concentrated on documentation and ensuring everything makes sense. I found a few glitches on the way which the attached patch corrects, see attached interdiff.

Fortunately the FormInterface patch got committed meanwhile, so attaching just the patch + the interdiff.

Status: Needs review » Needs work
Issue tags: -Needs architectural review, -Blocks-Layouts

The last submitted patch, d8_condition.patch, failed testing.

EclipseGc’s picture

Status: Needs work » Needs review
Issue tags: +Needs architectural review, +Blocks-Layouts

#51: condition-api.patch queued for re-testing.

fago’s picture

FileSize
43.48 KB

Patch applies for me to latest HEAD, thus re-uploading. Maybe the test-bot has some sort of caching and misses the latest changes?

fago’s picture

I've added an issue summary, please review/correct.

fago’s picture

Issue summary: View changes

Updated issue summary.

Status: Needs review » Needs work

The last submitted patch, d8_condition.patch, failed testing.

fago’s picture

Status: Needs work » Needs review

"file_put_contents(): Only 0 of 93 bytes written, possibly out of free disk space", grml looks like test-bot node is broken.

fago’s picture

#56: d8_condition.patch queued for re-testing.

EclipseGc’s picture

I've not added in fago's changes here yet. This just adds buildForm/validateForm/submitForm coverage and uses the NodeType as the example. Digging through fago's changes now. The interdiff is from my last patch.

Eclipse

EclipseGc’s picture

+++ b/core/lib/Drupal/Component/Plugin/ContextAwarePluginInterface.phpundefined
@@ -8,34 +8,36 @@
+   *   If the requested context ist not defined.

is not ist, that's a Diablo II rune. ;-)

fago’s picture

+++ b/core/modules/system/lib/Drupal/system/Tests/Condition/ConditionFormTest.php
@@ -0,0 +1,35 @@
+ * Definition of Drupal\system\Tests\Condition\ConditionFormTest.

Contains \Drupal..

+++ b/core/modules/system/tests/modules/condition_test/condition_test.module
@@ -0,0 +1,18 @@
+
+function condition_test_node_info() {

implements hook.

+++ b/core/modules/system/tests/modules/condition_test/lib/Drupal/condition_test/FormController.php
@@ -0,0 +1,50 @@
+
+  protected $condition;

Missing docs.

+++ b/core/modules/system/tests/modules/condition_test/lib/Drupal/condition_test/FormController.php
@@ -0,0 +1,50 @@
+
+  public function getFormID() {

Implements interface docs.

fago’s picture

Actually "ist" is German for "is" - damn. ;-)

EclipseGc’s picture

FileSize
24.76 KB
48.99 KB

this patch includes all of fago's changes in the previous interdiff he posted, I fixed the minor typo I mentioned in the previous comment, and added and corrected documentation to all my form test coverage. Much of fago's previous patch includes some common sense changes to the context aware plugin system, and since conditions is the first plugin to make use of that, I generally support doing most of this here.

I think we're pretty close at this point. Feedback very very welcome.

Interdiff is from my last patch and will include much of fago's previous interdiff.

Eclipse

EclipseGc’s picture

if this passes, my next patch will include fago's "Contains" doc issue, otherwise I got the others.

Eclipse

fago’s picture

hm, an interdiff to my patch would have helped me more. Anyway, I looked at the whole patch again and stumbled over that:

+++ b/core/lib/Drupal/Core/Executable/ExecutableException.php
@@ -0,0 +1,16 @@
+/**
+ * Generic action exception class.

executable plugin exception class

+++ b/core/lib/Drupal/Core/Executable/ExecutablePluginBase.php
@@ -0,0 +1,138 @@
+   *
+   * @see \Drupal\Component\Plugin\PluginBase::$configuration

I missed removing this @see before. $configuration is protected and so does not matter to the API consumer at all.

Anyway no reason to hold this up on this on this minor documentation fixes.
I think this is overall ready and I'd call it RTBC from my side given tests come back green.

EclipseGc’s picture

I have one more pass to make on this that adds in the configured form execution test so I'll get those changes in it. It'd be good to have that test, still it won't be any architectural change over what's here, just an extra test and cleanup.

Eclipse

EclipseGc’s picture

ok, what I hope is the last patch. Adds in the test I missed previously and fixes fago's doc issues above.

Eclipse

EclipseGc’s picture

and per fago's rtbc, I'd like to suggest that extends to this patch since there aren't any architectural changes, and all I did was add a new test and fix some docs.

Eclipse

effulgentsia’s picture

Some of this might be follow up material or invalid feedback, so not changing status, but here's my thoughts upon reading this patch for the first time. Overall, great work! Very much excited about seeing this land.

+++ b/core/lib/Drupal/Component/Plugin/ContextAwarePluginBase.php
@@ -66,44 +63,67 @@ public function getContexts() {
+  /**
+   * Implements \Drupal\Core\Executable\ExecutableInterface::valdidate().
+   */
+  public function validate() {
+    $violations = new ConstraintViolationList();
+    // @todo: Implement symfony validator API to let the validator traverse
+    // and set property paths accordingly.
+
+    foreach ($this->getContextDefinitions() as $name => $definition) {
+      // Validate any set values.
+      if (isset($this->context[$name])) {
+        $violations->addAll($this->context[$name]->validate());
+      }
+      // @todo: If no value is set, make sure any mapping is validated.
+    }
+    return $violations;
+  }

ExecutableInterface doesn't include validate(), so doc is wrong. Beyond that, I don't see why we want validate() on ContextAwarePluginBase. It doesn't look like the patch makes use of it. It seems out of scope for this issue: can we defer it to a follow up?

+++ b/core/lib/Drupal/Component/Plugin/ContextAwarePluginInterface.php
@@ -45,47 +53,64 @@ public function getContexts();
+  /**
+   * Validates the set values for the defined contexts.
+   *
+   * @return \Symfony\Component\Validator\ConstraintViolationListInterface
+   *   A list of constraint violations. If the list is empty, validation
+   *   succeeded.
+   */
+  public function validate();

As above, this seems odd for ContextAwarePluginInterface.

+++ b/core/lib/Drupal/Core/Executable/ExecutableInterface.php
@@ -0,0 +1,47 @@
+namespace Drupal\Core\Executable;

Maybe I'm missing something, but the entire Drupal\Core\Executable package seems like over-abstraction to me. What are use cases for it other than Condition?

+++ b/core/lib/Drupal/Core/Executable/ExecutablePluginBase.php
@@ -0,0 +1,138 @@
+    $context = array();
+    if (isset($configuration['context'])) {
+      $context = $configuration['context'];
+      unset($configuration['context']);
+    }
+    parent::__construct($configuration, $plugin_id, $discovery);
+    foreach ($context as $key => $value) {
+      $context_definition = $this->getContextDefinition($key);
+      $this->context[$key] = new Context($context_definition);
+      $this->context[$key]->setContextValue($value);
+    }

This seems useful to push down to ContextAwarePluginBase.

+++ b/core/lib/Drupal/Core/Executable/ExecutablePluginBase.php
@@ -0,0 +1,138 @@
+   * Gets an array of definitions of available configuration options.
+   *
+   * @todo: This needs to go into an interface.
+   *
+   * @return array
+   *   An array of typed data definitions describing available configuration
+   *   options, keyed by option name.
+   */
+  public function getConfigDefinitions() {
+    $definition = $this->getDefinition();
+    if (!empty($definition['configuration'])) {
+      return $definition['configuration'];
+    }
+    return array();
+  }
+
+  /**
+   * Gets the definition of a configuration option.
+   *
+   * @todo: This needs to go into an interface.
+   *
+   * @return array
+   *   The typed data definition describing the configuration option, or FALSE
+   *   if the option does not exist.
+   */
+  public function getConfigDefinition($key) {
+    $definition = $this->getDefinition();
+    if (!empty($definition['configuration'][$key])) {
+      return $definition['configuration'][$key];
+    }
+    return FALSE;
+  }
+
+  /**
+   * Gets all configuration values.
+   *
+   * @todo: This needs to go into an interface.
+   *
+   * @see \Drupal\Component\Plugin\PluginBase::$configuration
+   *
+   * @return array
+   *   The array of all configuration values, keyed by configuration option
+   *   name.
+   */
+  public function getConfig() {
+    return $this->configuration;
+  }
+
+  /**
+   * Sets the value of a particular configuration option.
+   *
+   * @param string $name
+   *   The name of the configuration option to set.
+   * @param mixed $value
+   *   The value to set.
+   *
+   * @todo This doesn't belong here. Move this into a new base class in
+   *   http://drupal.org/node/1764380.
+   * @todo This does not set a value in config(), so the name is confusing.
+   *
+   * @return \Drupal\Core\Executable\ExecutablePluginBase.
+   *   The executable object for chaining.
+   */
+  public function setConfig($key, $value) {
+    if ($definition = $this->getConfigDefinition($key)) {
+      $typed_data = typed_data()->create($definition, $value);
+
+      if ($typed_data->validate()->count() > 0) {
+        throw new PluginException("The provided configuration value does not pass validation.");
+      }
+    }
+    $this->configuration[$key] = $value;
+    return $this;
+  }

There are so many todos here that this just doesn't seem baked yet. Can we remove all of this and deal with it in a follow up?

+++ b/core/lib/Drupal/Core/Plugin/Context/Context.php
@@ -54,21 +70,31 @@ public function getTypedContext() {
+  /**
+   * Overrides \Drupal\Component\Plugin\Context\Context::getConstraints().
+   */
+  public function validate() {

Doc is wrong.

EclipseGc’s picture

Discussed this at length with effulgentsia on skype. I tried to justify a number of these things and overall he seemed ok to file follow ups, however the validate() method on ContextAwareInterface didn't make sense as is because it's disconnected from the unvalidated __construct that was on ExecutablePluginBase. As such I moved that construct to the Core and Component ContextAwarePluginBase classes, and renamed validate() to validateContexts(). I also fixed the docs issue effulgentsia found and fixed one I also found.

Eclipse

effulgentsia’s picture

Status: Needs review » Reviewed & tested by the community
Issue tags: -Needs architectural review

I filed #1920816: Factor getConfig(), setConfig(), getConfigDefinition(), and getConfigDefinitions() out of ExecutablePluginBase and #1920822: Decouple ExecutableInterface from Conditions and FormInterface for the two things I still think are problematic, but those will need input from fago, and it's very late night / early morning in Europe now, and I don't think they should hold up this really valuable addition to Drupal (see #47). It'll be great to get block visibility and actions API using this in core, and Panels, Context, Rules, etc. all reusing this from contrib. Therefore, RTBC, assuming bot is happy.

webchick’s picture

Title: Condition Plugin System » Change notice: Condition Plugin System
Category: feature » task
Status: Reviewed & tested by the community » Active
Issue tags: +Needs change record

I'm re-categorizing this as a major task since it seems to block a major task, according to #47.

Kris took a good half hour to walk me through this patch on IRC. While I can't guarantee that I understand every line of it, most of it was pretty straight-forward. This also blocks other stuff and has the effulgentsia seal of approval! Ergo...

Committed and pushed to 8.x. Thanks!

Back to critical task + active for the change notice.

webchick’s picture

Issue summary: View changes

Updated issue summary.

podarok’s picture

Issue tags: +actions, +rules, +VDC
fago’s picture

Category: task » feature
Status: Active » Needs review

woohoo - thanks everyone! I responded to the created issues.

jibran’s picture

Category: feature » task
Status: Needs review » Active

change record is still pending.

jibran’s picture

Issue summary: View changes

Updated issue summary. Added follow-up

EclipseGc’s picture

Issue summary: View changes

adding follow up condition plugins.

jibran’s picture

+++ b/core/modules/node/lib/Drupal/node/Plugin/Core/Condition/NodeType.phpundefined
@@ -0,0 +1,92 @@
+/**
+ * Provides a 'Node Type' condition.
+ *
+ * @Plugin(
+ *   id = "node_type",
+ *   label = @Translation("Node Bundle"),
+ *   module = "node",
+ *   context = {
+ *     "node" = {
+ *       "type" = "entity",
+ *       "constraints" = {
+ *         "EntityType" = "node"
+ *       }
+ *     }
+ *   }
+ * )

As NodeType condition is created here. So I created #1932810: Add entity bundles condition plugin for entities with bundles which is related and possible followup.

podarok’s picture

Status: Active » Needs work

tag

xjm’s picture

Status: Needs work » Active

There's still no change record draft here. :)

EclipseGc’s picture

Title: Change notice: Condition Plugin System » Condition Plugin System

Change notice is here: http://drupal.org/node/1961370

EclipseGc’s picture

Priority: Critical » Normal
Status: Active » Fixed
ParisLiakos’s picture

Category: task » feature
Issue tags: -Needs change record

restoring category and removing tag

Status: Fixed » Closed (fixed)

Automatically closed -- issue fixed for 2 weeks with no activity.

fago’s picture

fago’s picture

Issue summary: View changes

adding form component follow ups

jibran’s picture

Issue summary: View changes

Added 1932810 to summary.

josephcheek’s picture

Issue summary: View changes
Anybody’s picture

I'd like to use the condition plugin system for other entity types than blocks. To be precise I'd like to add ot to a node type (header image) to determine when a view (filter) shell show that node as header image in a slider.

Is there any documentation how to use that side of the condition plugin? I found out how to add custom conditions but nothing about that part yet. I could imagine to create a condition field for that as general module solution with a view filter evaluating it agains the current page context.

Can someone help with a link or code example?

larowlan’s picture