In order to do things like conditions and smarter blocks, we need the ability to define if a context is required for a plugin instance. This is the beginning of an API that can help us define those things and check to make sure we're providing the appropriate contexts for our plugins. This system is not doing anything except supporting contexts that would be injected into a plugin for the time being. It provides some basic context wrappers that validate the data that is set as a context, contextually aware plugin abstracts, and Component and Core versions of all the above for greater Drupal specific uses.

Docs, testing and tokens support all needs to be added.

Eclipse

Files: 
CommentFileSizeAuthor
#59 context.patch29.59 KBEclipseGc
PASSED: [[SimpleTest]]: [MySQL] 49,420 pass(es).
[ View ]
#59 context-interdiff-do-not-test.patch7.83 KBEclipseGc
#56 context.patch31.04 KBEclipseGc
PASSED: [[SimpleTest]]: [MySQL] 49,394 pass(es).
[ View ]
#56 context-interdiff-do-not-test.patch3.66 KBEclipseGc
#50 context.patch28.92 KBEclipseGc
PASSED: [[SimpleTest]]: [MySQL] 49,138 pass(es).
[ View ]
#50 context-interdiff-do-not-test.patch3.45 KBEclipseGc
#47 context.patch28.97 KBEclipseGc
PASSED: [[SimpleTest]]: [MySQL] 49,129 pass(es).
[ View ]
#47 context-interdiff-do-not-test.patch5.22 KBEclipseGc
#42 d8_context.patch28.93 KBfago
PASSED: [[SimpleTest]]: [MySQL] 49,120 pass(es).
[ View ]
#42 d8_context.interdiff.txt4.65 KBfago
#40 d8_context.interdiff.txt8.69 KBfago
#40 d8_context.patch28.11 KBfago
PASSED: [[SimpleTest]]: [MySQL] 49,096 pass(es).
[ View ]
#38 context.patch27.95 KBEclipseGc
PASSED: [[SimpleTest]]: [MySQL] 49,086 pass(es).
[ View ]
#38 context-interdiff-do-not-test.patch18.58 KBEclipseGc
#36 context.patch27.82 KBEclipseGc
PASSED: [[SimpleTest]]: [MySQL] 49,159 pass(es).
[ View ]
#36 context-interdiff-do-not-test.patch22.35 KBEclipseGc
#31 context.patch26.64 KBEclipseGc
PASSED: [[SimpleTest]]: [MySQL] 49,068 pass(es).
[ View ]
#31 context-interdiff-do-not-test.patch22.79 KBEclipseGc
#28 context.patch27.1 KBEclipseGc
PASSED: [[SimpleTest]]: [MySQL] 48,984 pass(es).
[ View ]
#28 context-interdiff-do-not-test.patch18.63 KBEclipseGc
#21 context.patch22.11 KBEclipseGc
FAILED: [[SimpleTest]]: [MySQL] 49,176 pass(es), 1 fail(s), and 0 exception(s).
[ View ]
#19 conditions.patch24.77 KBEclipseGc
PASSED: [[SimpleTest]]: [MySQL] 49,359 pass(es).
[ View ]
#17 context.patch25.04 KBEclipseGc
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch context_5.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#17 context-interdiff-do-not-test.patch13.69 KBEclipseGc
#13 context.patch21.9 KBEclipseGc
PASSED: [[SimpleTest]]: [MySQL] 49,302 pass(es).
[ View ]
#13 context-interdiff-do-not-test.patch6.52 KBEclipseGc
#9 context.patch16.39 KBEclipseGc
FAILED: [[SimpleTest]]: [MySQL] 50,705 pass(es), 1 fail(s), and 0 exception(s).
[ View ]
#9 context-interdiff-do-not-test.patch7.25 KBEclipseGc
#4 context.patch12.89 KBEclipseGc
PASSED: [[SimpleTest]]: [MySQL] 50,689 pass(es).
[ View ]
#4 context-interdiff-do-not-test.patch9.22 KBEclipseGc
context.patch7.25 KBEclipseGc
PASSED: [[SimpleTest]]: [MySQL] 50,713 pass(es).
[ View ]

Comments

Crell’s picture

+++ b/core/lib/Drupal/Component/Plugin/Context/Context.php
@@ -0,0 +1,52 @@
+  /**
+   * The PSR-0 class or an array definition to which context should conform.
+   *
+   * @var mixed
+   *  A string or array.
+   */
+  protected $contextDefinition;

What does this mean? It's a string that is a class name? (PSR-0 is irrelevant here.) "This could be a class name or an array" sounds like a bad idea.

+++ b/core/lib/Drupal/Component/Plugin/Context/Context.php
@@ -0,0 +1,52 @@
+    if (is_string($this->contextDefinition)) {
+      if ($context instanceof $this->contextDefinition) {
+        return $context;
+      }

If we're not modifying $context, and a failed validation is always an exception, then we shouldn't return it and just let the exception happen.

+++ b/core/lib/Drupal/Component/Plugin/ContextualPluginBase.php
@@ -0,0 +1,87 @@
+abstract class ContextualPluginBase extends PluginBase {

This seems like it should be a PluginContextAwareInterface, so that the base class is just some default utilities.

+++ b/core/lib/Drupal/Core/Plugin/Context/Context.php
@@ -0,0 +1,40 @@
+    if (is_array($this->contextDefinition)) {
+      $typed_data = typed_data()->create($this->contextDefinition, $context);

Technical foul for calling a floating function from within the class. 5 yard penalty, still first down.

+++ b/core/lib/Drupal/Core/Plugin/ContextualPluginBase.php
@@ -0,0 +1,28 @@
+abstract class ContextualPluginBase extends PluginBase {

Why two of these?

Just from the code so far, if we hadn't been talking about this for the past 2 years I wouldn't have the slightest clue what to do with anything here. :-)

tim.plunkett’s picture

Technical foul for calling a floating function from within the class. 5 yard penalty, still first down.

I did a massive double take here, because while you receive a huge A+ for almost convincing me you knew things about football, technical fouls are in basketball.

EclipseGc’s picture

What does this mean? It's a string that is a class name? (PSR-0 is irrelevant here.) "This could be a class name or an array" sounds like a bad idea.

It's a class name or an array because the array defines a TypedData plugin. Not at all how I would have done it, but it's how TypedData works and I'm not in any position to try to rewrite that in the next 3+ weeks. I'm pretty sure this is a sane way to handle the problem for the moment.

If we're not modifying $context, and a failed validation is always an exception, then we shouldn't return it and just let the exception happen.

When a TypedData definition is passed for the context definition, and we get something passed as context that will validated for that TypedData , we return the TypedData class, and not the original context. That is why this is setup to return the context again, to allow the validation phase to modify what we're treating as context if it has passed validation.

Technical foul for calling a floating function from within the class. 5 yard penalty, still first down.

I'm not injecting that. It'd have to come from the ContextualPluginBase, through the Context classes, and then down to here. Supporting TypedData is one of the reasons that the Core version of the class exists, supporting a bunch of weird injection stuff I don't really need just for that seems very very odd, especially when we're discussing essentially injecting a plugin manager. I mean, propose solutions with code, but I'm sort of anti-injecting this particular thing. There have to be sane exceptions to that general rule.

Why two of these?

Because one is a component because I don't want to neglect that, and the other is for Drupal core.

Eclipse

EclipseGc’s picture

Status:Active» Needs review
StatusFileSize
new9.22 KB
new12.89 KB
PASSED: [[SimpleTest]]: [MySQL] 50,689 pass(es).
[ View ]

Added a getContextWrapper() to the ContextualPluginBase class in Component, this is actually the same code as what getContext() was doing, which was wrong, but useful. getContext() is now much simpler.

Added a bit of documentation to the classes.

Added a little bit of testing. This is a webtest for the moment, but if we supplied our own classes instead of leveraging entities, this would be trivial to turn into a unit test.

Eclipse

Crell’s picture

Status:Needs review» Active
+++ b/core/lib/Drupal/Component/Plugin/Context/Context.php
@@ -0,0 +1,52 @@
+  public function validate($context) {
+    if (is_string($this->contextDefinition)) {
+      if ($context instanceof $this->contextDefinition) {
+        return $context;
+      }
+      throw new ContextException("The context passed for $key was not an instance of $this->contextDefinition.");
+    }
+    throw new ContextException("An error was encountered while trying to validate the context for $key.");
+  }

I see nothing here that is creating a new context or typed data object. Is that coming later, then?

webchick’s picture

Status:Active» Needs review

assume that was a cross-post...

EclipseGc’s picture

Crell

Stop looking in Drupal/Component/Plugin/Context/Context.php and look in Drupal/Core/Plugin/Context/Context.php :-)

So, I'll explain a bit. The context system is less about creating contexts, and more about validating that what was passed is a valid context. The plugins document that they require a particular context (again, optional contexts are coming) and then this system validates that a context passed is a valid context for the plugin's purposes. WHERE that context comes from is a separate concern, and one I'm not beginning to delve into at all here because that depends, first and foremost on parameter upcasting and displays for the vast majority of use cases. Other use cases are totally feasible but not really something I'm concerned with. The plugin API doesn't care WHERE the context originally came from, so long as it gets passed to the plugin before it needs it.

In so far as typed data goes, the very line you penalized me on earlier does exactly that (creates a typed data object for the passed context). It will only create one of these if the context documentation on the plugin indicates that it should do so. For example: I can't represent a string as a class... cause they're strings, but I can indicate that the typed data plugin for strings should be loaded in order to validate that what was passed to me was indeed a string. Within the typed data string plugin, it's validate method should do the requisite is_string() check and then we procede with the TypedData class as our context, not the raw string. This should ultimately afford us the ability to ask a context what tokens it supports, and solve each context's tokens individually, and things like that. As I read the code, TypedData plugins don't seem to have really implemented their various validate() methods appropriate, so I can't test that yet. Fago led me to believe that was forthcoming shortly, so I suppose this is blocked on that (need to go find the issue number, sorry).

Hopefully this explains what's going on in this code a bit better.

Eclipse

EclipseGc’s picture

As a practical example (which the tests actually do this)

if you had a plugin of some sort that had a context notation on it:

<?php
/**
 * @Plugin(
 *   id = "some_id",
 *   context = {
 *    "user" = "Drupal/user/Plugin/Core/Entity/User"
 *   }
 */
class MyPlugin extends ContextualPluginBase {
  public function
getTitle() {
   
$user = $this->getContext('user');
    return
$user->label();
  }
}
?>

Then you could go about instantiating that plugin thus:

<?php
$plugin
= $manager->createInstance('some_id');
$plugin->getTitle();
?>

And this would be a completely unconfigured plugin. Calling getTitle() right now will throw the exception "The user context is not yet set." To do this properly we need to:

<?php
// get a $user by some methodology.
$plugin = $manager->createInstance('some_id');
$plugin
 
->setContext('user', $user)
  ->
getTitle();
?>

This will result in the title of the user being returned as expected because, we have a context set. If we were to have passed a $node object instead of a $user object, we'll get an exception telling us the $node isn't an instance of the user entity class. Typed data would work similarly, except instead of having a class name in the plugin definition, we'd have a short array detailing which TypedData plugin to use, and how it should be configured. I need to dig into that further and actually test it, but I'm pretty sure the basic principle is correct.

Hopefully these code examples helped.

Eclipse

EclipseGc’s picture

StatusFileSize
new7.25 KB
new16.39 KB
FAILED: [[SimpleTest]]: [MySQL] 50,705 pass(es), 1 fail(s), and 0 exception(s).
[ View ]

Ok, added a bunch more tests, a few doc tweaks and converted the system into a UnitTest. Still no coverage for TypedData in tests yet, but I've got all the major functionality covered with tests at this point and am grinding out the various helper methods that still need tests.

Eclipse

Status:Needs review» Needs work

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

EclipseGc’s picture

Status:Needs work» Needs review

Upgrade path test failure, not mine :-)

Status:Needs review» Needs work

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

EclipseGc’s picture

StatusFileSize
new6.52 KB
new21.9 KB
PASSED: [[SimpleTest]]: [MySQL] 49,302 pass(es).
[ View ]

Added some testing coverage for TypedData. In order to do this I had to add fill out the missing validate methods in most of the simple TypedData plugins. This is (for the moment) included in this patch for the purposes of keeping the tests passing. The relevant code is broken out in #1845546: Implement validation for the TypedData API. There is no testing coverage on that code yet, so the coverage included here is actually at least testing the validate method of the String TypedData plugin.

This is all pretty simple stuff, still managed to keep the test a DrupalUnitTest thanks to berdir. There's not a whole lot left to get coverage for, so really need to push on #1845546: Implement validation for the TypedData API and get my patch or something on that scale committed if possible.

Eclipse

EclipseGc’s picture

Status:Needs work» Needs review
Crell’s picture

With the explanation in #8, the patch in #13 makes a *lot* more sense. Thanks, Kris!

One important point to note, though, is that assigning Context this way effectively precludes mapping directly to a block as a controller from the routing system. There would need to be an intermediary that does func_get_args() and loops on setContext(), or similar. I already briefly discussed with Sam that we probably need that sort of intermediary as part of the display object system.

That in turn suggests that for cases where we want to render a block directly and not via a separate render strategy call (aka, "where ESI, hInclude, subreqests, and so on happen"), we probably don't even fire a subrequest and just call the block and call it a day. That may or may not be such a bad thing; I'm not sure at the moment.

That perhaps implies that we have a "whole page" controller (currently HtmlPageController, which Sam is supposed to be rewriting) and a "partial page" controller that only handles a single block from a subrequest/ESI/whatever. That may not be so bad either, especially as the handling of an in-process and out-of-process block may turn out to be quite different on the asset front. (One does collection and merging, the other renders to Drupal ajax commands.)

I have to think this through a bit more, but it almost sounds like I'm talking myself into never using subrequests at all, and only using ESI. Which is actually quite far off from where we started (originally I wanted to route directly to blocks from the routing system), but perhaps it will end up working better. As I said, needs more thought.

Some other code-level comments below:

+++ b/core/lib/Drupal/Component/Plugin/ContextualPluginBase.php
@@ -0,0 +1,97 @@
+/**
+ * Base class for plugins that use context.
+ */

This still needs an interface to go with the base class. I consider that a commit blocker. Also, we should probably follow the "aware" pattern: ContextAwarePluginInterface

+++ b/core/lib/Drupal/Component/Plugin/ContextualPluginBase.php
@@ -0,0 +1,97 @@
+  public function getContexts() {
+    if (empty($this->configuration['context'])) {
+      throw new PluginException("There are no set contexts.");
+    }
+    return $this->configuration['context'];

Should this really be an exception rather than just an empty array?

EclipseGc’s picture

Should this really be an exception rather than just an empty array?

That code is definitely wrong, good call, I need to check to see if this is a plugin that has required contexts in its definition, and if none of them are set, then throw an exception, otherwise, for plugins that extend a context aware base but that don't actually have defined contexts (this will happen from time to time) that can return an empty array. (more tests to write)

With regard to your other musings, yes, there will need to be some sort of mapping, so if we have a url like

block/{block}/{node}/{user} then {block} would upcast to the block entity, and we'd assume the names of the further parameters map directly to the names in the context definitions, so the following code would be totally normal:

<?php
// The following code makes a lot of assumptions, but I think you'll get it.
$block = $route['arguments']['block'];
unset(
$route['argument_map'][0]);
foreach (
$route['argument_map'] as $context) {
 
$block->getPlugin->setContext($context, $route['arguments'][$context]);
}
return
$block->renderBlock(); // or whatever it is now.
?>
EclipseGc’s picture

StatusFileSize
new13.69 KB
new25.04 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch context_5.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

Ok, added a bunch more tests around getContexts() and getContextsWrappers(). Tests around plugins using multiple plugins. There is a code answer to Crell's question about getContexts() at this point. I've not created an interface for ContextAwarePlugins yet, nor have I renamed things as Crell suggested. Those are perhaps tomorrow's tasks, we shall see.

Removed my mockentity/node/user classes in favor of real user and node entities since I've moved to DrupalUnitTestBase that is possible and easy now. It should give me greater flexibility in the tests later. We are still very very stuck on #1845546: Implement validation for the TypedData API. In that light I put up a big review on it today. Unfortunately it is blocked on a number of issues as well, so we need to unblock all of that to get this to move.

Will try to get an interface and the name change squared away in the next patch.

Eclipse

Status:Needs review» Needs work

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

EclipseGc’s picture

StatusFileSize
new24.77 KB
PASSED: [[SimpleTest]]: [MySQL] 49,359 pass(es).
[ View ]

This should apply to head. It won't pass until TypedData validation is in though.

Eclipse

EclipseGc’s picture

Status:Needs work» Needs review
EclipseGc’s picture

StatusFileSize
new22.11 KB
FAILED: [[SimpleTest]]: [MySQL] 49,176 pass(es), 1 fail(s), and 0 exception(s).
[ View ]

wtf, put conditions instead of context.

Status:Needs review» Needs work

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

EclipseGc’s picture

FTR that one exception was expected, and is continued to be expected until #1845546: Implement validation for the TypedData API lands.

Eclipse

effulgentsia’s picture

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

Status:Needs work» Needs review

#21: context.patch queued for re-testing.

Requeueing to prove that this passes test now.

Eclipse

Status:Needs review» Needs work

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

EclipseGc’s picture

Looks like a random failure to me :-D (if I'm allowed to be happy about those).

Eclipse

EclipseGc’s picture

Status:Needs work» Needs review
StatusFileSize
new18.63 KB
new27.1 KB
PASSED: [[SimpleTest]]: [MySQL] 48,984 pass(es).
[ View ]

I think this handles most of the feedback.

EclipseGc’s picture

I'll fix the missing newlines in the next patch, they're already fixed locally.

Eclipse

Crell’s picture

+++ b/core/lib/Drupal/Component/Plugin/Context/Context.php
@@ -0,0 +1,82 @@
+/**
+ * A generic context wrapper class.
+ */
+class Context {

Pedant: The fancy name is "adapter", which is the magic phrase that Earl used that convinced me this class has a legitimate reason to exist. :-) May as well use the formal name to keep the OO pedants happy.

Also, this class doesn't actually implement the ContextInterface. :-)

+++ b/core/lib/Drupal/Component/Plugin/Context/ContextInterface.php
@@ -0,0 +1,63 @@
+   * Set the context of this class.
+   *
+   * @param mixed $context
+   *   A context is some generally a class matching the definitions passed to
+   *   setContextDefinition.

Grammar hiccup. I think you mean "some value, generally a class matching..."

+++ b/core/lib/Drupal/Component/Plugin/Context/ContextInterface.php
@@ -0,0 +1,63 @@
+  /**
+   * Set the definition that the context must conform to.
+   *
+   * @param mixed $contextDefinition
+   *   A defining characteristic representation of the context against which it
+   *   can be validated. This is typically a class it must be an instance of.

Typically the name of a class? The comment is not clear on what I should be doing here.

+++ b/core/lib/Drupal/Component/Plugin/ContextuallyAwarePluginInterface.php
@@ -0,0 +1,91 @@
+/**
+ * Interface for defining contextually aware plugins.
+ */
+interface ContextuallyAwarePluginInterface extends PluginInspectionInterface {

"ContextuallyAware" sounds awkward. Why not just "ContextAwarePluginInterface"? (Vis, plugin that is aware of context.)

+++ b/core/lib/Drupal/Component/Plugin/ContextuallyAwarePluginInterface.php
@@ -0,0 +1,91 @@
+  /**
+   * Returns the wrappers for set values for all defined contexts.
+   *
+   * @return array
+   *   Returns the array of all set context wrappers.
+   */
+  public function getContextsWrappers();

The two plurals here is awkward. I think ContextWrappers is sufficient.

+++ b/core/lib/Drupal/Core/Plugin/Context/Context.php
@@ -0,0 +1,49 @@
+  public function validate($context) {
+    // Check to make sure we have a class name, and that the passed context is
+    // an instance of that class name.
+    if (is_string($this->contextDefinition)) {
+      if ($context instanceof $this->contextDefinition) {
+        return $context;
+      }
+      throw new ContextException("The context passed was not an instance of $this->contextDefinition.");
+    }
+    // Check to see if we have a typed data definition instead of a class name.
+    if (is_array($this->contextDefinition)) {
+      $typed_data_manager = new TypedDataManager();
+      $typed_data = $typed_data_manager->create($this->contextDefinition, $context);
+      // If we do have a typed data definition, validate it and return the
+      // typed data instance instead.
+      if ($typed_data->validate()) {
+        return $typed_data;
+      }
+      throw new ContextException("The context passed could not be validated through typed data.");
+    }
+    throw new ContextException("An error was encountered while trying to validate the context.");
+  }

I'm pretty sure this can be simplified to check for typed data first, and if it's not typed data (vis, not an array definition) just return parent::validate().

EclipseGc’s picture

StatusFileSize
new22.79 KB
new26.64 KB
PASSED: [[SimpleTest]]: [MySQL] 49,068 pass(es).
[ View ]

OK, taking care of Crell's issues with #30

Eclipse

fago’s picture

+++ b/core/lib/Drupal/Component/Plugin/Context/Context.php
@@ -0,0 +1,84 @@
+/**
+ * A generic context wrapper class.
+ */

generic context? That's very generic - can we be more specific on what I need this for?

+++ b/core/lib/Drupal/Component/Plugin/Context/ContextInterface.php
@@ -0,0 +1,64 @@
+   * @return mixed
+   *   Returns the object passed to it, or a class based representation of the
+   *   context.
+   */

How do I know whether validation fails?

+++ b/core/lib/Drupal/Component/Plugin/ContextAwarePluginInterface.php
@@ -0,0 +1,91 @@
+   * @return object
+   *   Returns a context wrapper object.
+   */

What can I do with a context wrapper? Maybe there should be an interface for them? Or if it's just internal it should not be part of the interface.

What exactly is the job of the context wrapper? If used with typed data, could we make the context wrappers to equal the typed data object to save a layer of abstraction?

+++ b/core/lib/Drupal/Component/Plugin/Context/ContextInterface.php
@@ -0,0 +1,64 @@
+/**
+ * A generic context wrapper class.
+ */

No, it isn't.

EclipseGc’s picture

How do I know whether validation fails?

In order to be successfully used for a context, it HAS to pass validation, so either you get the object back, or your get an exception. In the case of typed data, the typed data plugin validation is proxied to.

generic context? That's very generic - can we be more specific on what I need this for?

and

What can I do with a context wrapper? Maybe there should be an interface for them? Or if it's just internal it should not be part of the interface.

What exactly is the job of the context wrapper? If used with typed data, could we make the context wrappers to equal the typed data object to save a layer of abstraction?

Sure, we can be more specific. The context wrapper is a class designed to help you get and validate the context you're dealing with. The "Component" side of this code is just expecting a class name to validate against the passed context. The "Core" side to this code extends that further allowing for typed data integration. At some point, the Core version of this class will likely have token support for contexts that support tokens, and it gives us a platform from which to do other such things. In the case of typed data, we need this intermediary because if a string is set as context, it will be the raw string which is set. Through the validation process a TypedData plugin for the string will be instantiated and ultimately replace the raw string value passed to us. The wrappers are pretty lite, and there is an interface (as you seem to have found since I said it was a class, and you point out it isn't :-) ).

With regard to your first question, yes that docblock could probably be a little less generic.

Eclipse

fago’s picture

In order to be successfully used for a context, it HAS to pass validation, so either you get the object back, or your get an exception. In the case of typed data, the typed data plugin validation is proxied to.

Yep, this should really go in the interface docs though.

In the case of typed data, we need this intermediary because if a string is set as context, it will be the raw string which is set.

Yep, makes sense - sometimes we want to unwrap, sometimes not.

..and there is an interface (as you seem to have found since I said it was a class, and you point out it isn't :-)

Well, that's called ContextInterface so I did not connect it. I think the wording needs to be more clear on what a context is and what a contextwrapper. I'd assume getContext() to get me an object implementing contextInterface, but it sounds like getContext() gets me the un-wrapped context-value whereas getContextWappers() actually gets me the object implementing ContextInterface.

If so, we could rename ContextInterface to ContextWrapperInterface or - what's about that terminology:
getContext() -> returns an object that is an instance of ContextInterface (= context wrapper now)
getContextValue() -> returns the plain value of the context. -> A context object would always wrap the plain value. Thus we'd probably have getValue() methods in the ContextInterface then, what would map quite well to how typed data objects work.

fago’s picture

Reviewed documention style

+++ b/core/lib/Drupal/Component/Plugin/Context/Context.php
@@ -0,0 +1,84 @@
+ * Contains Drupal\Component\Plugin\Context\Context.

Misses a lead \ - multiple times.

+++ b/core/lib/Drupal/Component/Plugin/Context/Context.php
@@ -0,0 +1,84 @@
+   * Sets the contextDefinition for us without needing to make a call to

Should not exceed 80chars / 1 line - multiple times.

+++ b/core/lib/Drupal/Component/Plugin/ContextAwarePluginInterface.php
@@ -0,0 +1,91 @@
+ * Definition of Drupal\Component\Plugin\ContextAwarePluginInterface.

Contains + leading \ - multiple times.

+++ b/core/lib/Drupal/Component/Plugin/ContextAwarePluginInterface.php
@@ -0,0 +1,91 @@
+   * Returns the set values for all defined contexts.

This sounds complex to me - is this the general way we document that? I think I've used "Gets ..." + "Sets ..." until now.

+++ b/core/lib/Drupal/Component/Plugin/ContextAwarePluginInterface.php
@@ -0,0 +1,91 @@
+   *   The name of the context in the plugin definition.
+   *
+   * @param mixed $value

There should be no new line between two @params.

+++ b/core/lib/Drupal/Core/Plugin/Context/Context.php
@@ -0,0 +1,41 @@
+   * Override for Drupal\Component\Plugin\Context\Context::validate().

leading \.

+++ b/core/modules/system/tests/modules/plugin_test/lib/Drupal/plugin_test/Plugin/plugin_test/mock_block/MockComplexContextBlock.php
@@ -0,0 +1,24 @@
+ * @see Drupal\plugin_test\Plugin\MockBlockManager

\. Other @see notations also - multiple times.

EclipseGc’s picture

StatusFileSize
new22.35 KB
new27.82 KB
PASSED: [[SimpleTest]]: [MySQL] 49,159 pass(es).
[ View ]

Ok, I think I addressed all of fago's issues.

Eclipse

fago’s picture

Status:Needs review» Needs work

ok, reviewed it again:

+++ b/core/modules/system/lib/Drupal/system/Tests/Plugin/PluginTestBase.php
@@ -70,6 +70,28 @@ public function setUp() {
+        'class' => 'Drupal\plugin_test\Plugin\plugin_test\mock_block\MockUserNameBlock',
+        'context' => array(
+          'user' => 'Drupal\user\Plugin\Core\Entity\User'

All specified classes miss the leading \.

+++ b/core/modules/system/tests/modules/plugin_test/lib/Drupal/plugin_test/Plugin/plugin_test/mock_block/TypedDataStringBlock.php
@@ -0,0 +1,24 @@
+    $context = $this->getContextValue('string');
+    return $context->getValue();

It says get context value, so what you get is not $context. Maybe $value or $context_value;

+++ b/core/lib/Drupal/Component/Plugin/Context/Context.php
@@ -0,0 +1,83 @@
+   * The context.
+   *
+   * @var mixed
+   */

The class is already the context, so what is that?

+++ b/core/lib/Drupal/Component/Plugin/Context/Context.php
@@ -0,0 +1,83 @@
+  public function setContext($context) {

again, the object is already the context. setContextValue() maybe?

+++ b/core/lib/Drupal/Component/Plugin/Context/Context.php
@@ -0,0 +1,83 @@
+   * Implements ContextInterface::getContext().

should be fully qualified as the others

+++ b/core/lib/Drupal/Component/Plugin/Context/ContextInterface.php
@@ -0,0 +1,64 @@
+ * Contains \Drupal\Component\Plugin\Context\Context.

copy paste mistake?

+++ b/core/lib/Drupal/Component/Plugin/Context/ContextInterface.php
@@ -0,0 +1,64 @@
+ * A generic context interface.
+ */
+interface ContextInterface {

Could use the better docs from the class.

+++ b/core/lib/Drupal/Component/Plugin/ContextAwarePluginInterface.php
@@ -0,0 +1,90 @@
+   * @return mixed
+   *   The context definitions if set, otherwise NULL.

array|null ?

+++ b/core/lib/Drupal/Component/Plugin/ContextAwarePluginInterface.php
@@ -0,0 +1,90 @@
+   * Gets the wrappers for set values for all defined contexts.

Gets the defined contexts.?

+++ b/core/lib/Drupal/Component/Plugin/ContextAwarePluginInterface.php
@@ -0,0 +1,90 @@
+   * Gets the context wrapper for a defined context.

-wrapper. Also, why not just "Gets the defined context"?

+++ b/core/lib/Drupal/Component/Plugin/ContextAwarePluginInterface.php
@@ -0,0 +1,90 @@
+   * @return object
+   *   Returns a context wrapper object.

-wrapper + the returned interface needs to be documentated

+++ b/core/lib/Drupal/Component/Plugin/ContextAwarePluginInterface.php
@@ -0,0 +1,90 @@
+   * @return object
+   *   Returns instantiated object of a context.

The context value needs not be an object, not?

+++ b/core/lib/Drupal/Component/Plugin/Exception/ContextException.php
@@ -0,0 +1,15 @@
+ * Definition of Drupal\Component\Plugin\Exception\ContextException.

Contains + \

+++ b/core/lib/Drupal/Component/Plugin/Exception/ContextException.php
@@ -0,0 +1,15 @@
+namespace Drupal\Component\Plugin\Exception;

leading \

+++ b/core/lib/Drupal/Component/Plugin/Exception/ContextException.php
@@ -0,0 +1,15 @@
+ * Generic Plugin exception class to be thrown when no more specific class
+ * is applicable.

shuold not be two lines

+++ b/core/lib/Drupal/Core/Plugin/ContextAwarePluginBase.php
@@ -0,0 +1,33 @@
+ * the Context wrapper class. This code is exactly the same as what is in

- wrapper

+++ b/core/modules/system/lib/Drupal/system/Tests/Plugin/ContextPluginTest.php
@@ -0,0 +1,203 @@
+    // Try to get a valid context wrapper that has not been set.

/s/"context wrapper"/"context"

+++ b/core/modules/system/lib/Drupal/system/Tests/Plugin/PluginTestBase.php
@@ -70,6 +70,28 @@ public function setUp() {
+        'label' => 'User Name',

It's just for tests, but I guess it should capitilize as "User name" - others also.

+++ b/core/lib/Drupal/Component/Plugin/Context/Context.php
@@ -0,0 +1,83 @@
+   * The context.
+   *
+   * @var mixed
+   */

The class is already the context, so what is that?

+++ b/core/lib/Drupal/Component/Plugin/Context/Context.php
@@ -0,0 +1,83 @@
+  public function setContext($context) {

again, the object is already the context. setContextValue() maybe?

+++ b/core/lib/Drupal/Component/Plugin/Context/Context.php
@@ -0,0 +1,83 @@
+   * Implements ContextInterface::getContext().

should be fully qualified as the others

+++ b/core/lib/Drupal/Component/Plugin/Context/ContextInterface.php
@@ -0,0 +1,64 @@
+ * Contains \Drupal\Component\Plugin\Context\Context.

copy paste mistake?

+++ b/core/lib/Drupal/Component/Plugin/Context/ContextInterface.php
@@ -0,0 +1,64 @@
+ * A generic context interface.
+ */
+interface ContextInterface {

Could use the better docs from the class.

+++ b/core/lib/Drupal/Component/Plugin/ContextAwarePluginInterface.php
@@ -0,0 +1,90 @@
+   * @return mixed
+   *   The context definitions if set, otherwise NULL.

array|null ?

+++ b/core/lib/Drupal/Component/Plugin/ContextAwarePluginInterface.php
@@ -0,0 +1,90 @@
+   * Gets the wrappers for set values for all defined contexts.

Gets the defined contexts.?

+++ b/core/lib/Drupal/Component/Plugin/ContextAwarePluginInterface.php
@@ -0,0 +1,90 @@
+   * Gets the context wrapper for a defined context.

-wrapper. Also, why not just "Gets the defined context"?

+++ b/core/lib/Drupal/Component/Plugin/ContextAwarePluginInterface.php
@@ -0,0 +1,90 @@
+   * @return object
+   *   Returns a context wrapper object.

-wrapper + the returned interface needs to be documentated

+++ b/core/lib/Drupal/Component/Plugin/ContextAwarePluginInterface.php
@@ -0,0 +1,90 @@
+   * @return object
+   *   Returns instantiated object of a context.

The context value needs not be an object, not?

+++ b/core/lib/Drupal/Component/Plugin/Exception/ContextException.php
@@ -0,0 +1,15 @@
+ * Definition of Drupal\Component\Plugin\Exception\ContextException.

Contains + \

+++ b/core/lib/Drupal/Component/Plugin/Exception/ContextException.php
@@ -0,0 +1,15 @@
+namespace Drupal\Component\Plugin\Exception;

leading \

+++ b/core/lib/Drupal/Component/Plugin/Exception/ContextException.php
@@ -0,0 +1,15 @@
+ * Generic Plugin exception class to be thrown when no more specific class
+ * is applicable.

shuold not be two lines

+++ b/core/lib/Drupal/Core/Plugin/ContextAwarePluginBase.php
@@ -0,0 +1,33 @@
+ * the Context wrapper class. This code is exactly the same as what is in

- wrapper

+++ b/core/modules/system/lib/Drupal/system/Tests/Plugin/ContextPluginTest.php
@@ -0,0 +1,203 @@
+    // Try to get a valid context wrapper that has not been set.

/s/"context wrapper"/"context"

+++ b/core/modules/system/lib/Drupal/system/Tests/Plugin/PluginTestBase.php
@@ -70,6 +70,28 @@ public function setUp() {
+        'label' => 'User Name',

It's just for tests, but I guess it should capitilize as "User name" - others also.

+++ b/core/modules/system/lib/Drupal/system/Tests/Plugin/PluginTestBase.php
@@ -70,6 +70,28 @@ public function setUp() {
+        'class' => 'Drupal\plugin_test\Plugin\plugin_test\mock_block\MockUserNameBlock',
+        'context' => array(
+          'user' => 'Drupal\user\Plugin\Core\Entity\User'

All specified classes miss the leading \.

+++ b/core/modules/system/tests/modules/plugin_test/lib/Drupal/plugin_test/Plugin/plugin_test/mock_block/TypedDataStringBlock.php
@@ -0,0 +1,24 @@
+    $context = $this->getContextValue('string');
+    return $context->getValue();

It says get context value, so what you get is not $context. Maybe $value or $context_value;

EclipseGc’s picture

StatusFileSize
new18.58 KB
new27.95 KB
PASSED: [[SimpleTest]]: [MySQL] 49,086 pass(es).
[ View ]

All specified classes miss the leading \.

All the examples are. It's out of scope of this issue to change that, I'm just conforming with what's there for the time being.

It says get context value, so what you get is not $context. Maybe $value or $context_value;

These are generally named whatever I expect to come across here ($node, $user, etc) so I'm going with $string_context.

The class is already the context, so what is that?

Fair enough, changed to $contextValue and updated code appropriately.

again, the object is already the context. setContextValue() maybe?

Considering the previous change, yeah ok.

+++ b/core/lib/Drupal/Component/Plugin/ContextAwarePluginInterface.php
@@ -0,0 +1,90 @@
+   * @return object
+   *   Returns instantiated object of a context.

The context value needs not be an object, not?

Ehhh, we're only supporting classed data or typed data, and both will ultimately put an object of some sort into the context value, so you are getting an object back.

+++ b/core/lib/Drupal/Component/Plugin/Exception/ContextException.php
@@ -0,0 +1,15 @@
+namespace Drupal\Component\Plugin\Exception;

leading \

?? we don't generally put leading \ on namespace. No where I've seen at least.

+++ b/core/modules/system/lib/Drupal/system/Tests/Plugin/PluginTestBase.php
@@ -70,6 +70,28 @@ public function setUp() {
+        'class' => 'Drupal\plugin_test\Plugin\plugin_test\mock_block\MockUserNameBlock',
+        'context' => array(
+          'user' => 'Drupal\user\Plugin\Core\Entity\User'

All specified classes miss the leading \.

Tried, fails tests, reverted back.

I think this should largely solve your existing objections. I tried to take care of anything that seemed reasonable and notated things that needed some additional explanation.

Eclipse

EclipseGc’s picture

Status:Needs work» Needs review
fago’s picture

StatusFileSize
new28.11 KB
PASSED: [[SimpleTest]]: [MySQL] 49,096 pass(es).
[ View ]
new8.69 KB

ok, I had another look at it but there where still quite some glitches - so I went ahead and fixed them. Updated patch attached.

fago’s picture

?? we don't generally put leading \ on namespace. No where I've seen at least.

of course, sry for that.

Ehhh, we're only supporting classed data or typed data, and both will ultimately put an object of some sort into the context value, so you are getting an object back.

hm, I understood that differently. I thought we use the Context object abstraction layer to unwrap typed data objects as suiting, e.g. have them auto-unwrap for primitives. Else I fear the double-wrapping leads to bad DX and I'd so no purpose in the second abstraction layer?

I'd suggest to go with context value == anything and have the typed data implementation auto unwrap for primitives by default. Then, we can add another typed-data context specific getter that always gets you a typed data object.

Also, after digging through the code I'm a bit worried about the definitions optionally being just the class name. I fear it makes it ends up in unnecessary complex code as you often have to check for the array notation vs the string notation. Could we enforce typed data arrays for the drupal implementation here?

fago’s picture

StatusFileSize
new4.65 KB
new28.93 KB
PASSED: [[SimpleTest]]: [MySQL] 49,120 pass(es).
[ View ]

ok, took a look at unwrapping typed data primitives - see attached patch. As discussed above that way the context-wrapper layer makes sense to me and DX is better. Thoughts?

So far the patch should be imho ready then.

EclipseGc’s picture

This all seems completely fine to me. Can we get an RTBC, I wrote way too much of this to be the one to do it.

Eclipse

fago’s picture

Great - I think it's RTBC also.

fubhy’s picture

Status:Needs review» Needs work

You are going to hate me for this review then. Drive-By-Shooting incoming.

+++ b/core/lib/Drupal/Component/Plugin/Context/Context.phpundefined
@@ -0,0 +1,83 @@
+   * @var mixed
+   *  A string or array.

Eagle eyes! There is just one white space for the indendation! Also, if we know its a string or array why don't we have @var string|array.

+++ b/core/lib/Drupal/Component/Plugin/Context/Context.phpundefined
@@ -0,0 +1,83 @@
+  public function __construct($contextDefinition) {
...
+  public function setContextDefinition($contextDefinition) {

Do we normally use camelCase in method arguments? Even in this case? I think this should be $context_definition even though it ends up in $this->contextDefinition.

+++ b/core/lib/Drupal/Component/Plugin/Context/ContextInterface.phpundefined
@@ -0,0 +1,68 @@
+ * A generic context interface or wrapping data a plugin needs to operate.

*for* wrapping data a plugin needs to operate.

+++ b/core/lib/Drupal/Component/Plugin/ContextAwarePluginBase.phpundefined
@@ -0,0 +1,103 @@
+ * Base class for plugins that use context.

Maybe better "Base class for plugins that are context aware?"

+++ b/core/lib/Drupal/Component/Plugin/ContextAwarePluginInterface.phpundefined
@@ -0,0 +1,91 @@
+   * @return array
+   *   Returns the array of all set context objects.

It's the return statement, oh and it's an array, alright! No need to repeat yourself!

@return array "The set context objects"

+++ b/core/lib/Drupal/Component/Plugin/ContextAwarePluginInterface.phpundefined
@@ -0,0 +1,91 @@
+   * @return \Drupal\Component\Plugin\Context\ContextInterface
+   *   Returns a context object.

Same here. A plain "The context object" is enough.

+++ b/core/lib/Drupal/Component/Plugin/ContextAwarePluginInterface.phpundefined
@@ -0,0 +1,91 @@
+   * @return array
+   *   Returns the array of all set context values.

Same here...

+++ b/core/lib/Drupal/Component/Plugin/ContextAwarePluginInterface.phpundefined
@@ -0,0 +1,91 @@
+   * @return $this
+   *   Returns the plugin instance.

I am not sure how we usually document @return $this. But I'd expect it to look different. E.g. @return ContextAwarePluginInterface "The called object for chaining."

+++ b/core/lib/Drupal/Component/Plugin/Exception/ContextException.phpundefined
@@ -0,0 +1,14 @@
+<?php
+/**
+ * @file
+ * Contains \Drupal\Component\Plugin\Exception\ContextException.
+ */

Missing whitespace after opening php statement.

+++ b/core/lib/Drupal/Core/Plugin/Context/Context.phpundefined
@@ -0,0 +1,68 @@
+  /**
+   * Override for \Drupal\Component\Plugin\Context\Context::getContextValue().
+   */
...
+  /**
+   * Override for \Drupal\Component\Plugin\Context\Context::validate().
+   */

Should be "Overrides" and not "Override for".

+++ b/core/lib/Drupal/Core/Plugin/ContextAwarePluginBase.phpundefined
@@ -0,0 +1,32 @@
+<?php
+/**
+ * @file
+ * Contains \Drupal\Core\Plugin\ContextAwarePluginBase
+ */

Missing new line after opening php statements.

fubhy’s picture

Oh, but looks nice conceptually. So once those nitpicks are fixed +1 RTBC :)

EclipseGc’s picture

StatusFileSize
new5.22 KB
new28.97 KB
PASSED: [[SimpleTest]]: [MySQL] 49,129 pass(es).
[ View ]

ok, fixed fubhy's issues.

Eclipse

EclipseGc’s picture

Status:Needs work» Needs review
fubhy’s picture

Status:Needs review» Reviewed & tested by the community
+++ b/core/lib/Drupal/Component/Plugin/Context/Context.phpundefined
@@ -24,16 +24,16 @@ class Context implements ContextInterface {
+   * @var string|array
+   *   A string or array.

This is still a bit weird.

A matter of personal flavor I guess. RTBC anyways if this comes back green.

EclipseGc’s picture

StatusFileSize
new3.45 KB
new28.92 KB
PASSED: [[SimpleTest]]: [MySQL] 49,138 pass(es).
[ View ]

ok, one last patch to a.) fix fubhy's last comment in 49 and also to move away from $this->configuration['context'] to $this->context for where the actual context objects reside. This removes the constructor setting of context which was pretty difficult to get right anyway and can be reimplemented on a per plugin basis if necessary.

Eclipse

fubhy’s picture

ah yeah, good catch... still RTBC

fago’s picture

Improvements look good.

I'd have just called the new "protected $context;" $context-s, as it's an array of contexts - but that should not hold us up here.

I second that this is RTBC.

Crell’s picture

+++ b/core/lib/Drupal/Core/Plugin/Context/Context.php
@@ -0,0 +1,68 @@
+      $type_definition = typed_data()->getDefinition($typed_value->getType());

I won't block the RTBC on this at this point, but I am flagging this as problematic for reasons that have been elucidated before.

With that caveat, I'll third this. :-)

fago’s picture

ad #53: I totally agree, but I guess we need to figure out how we inject dependencies in plugins first and apply a similar solution to the individual context classes as it's the same problem.

See #1863816: Allow plugins to have services injected

catch’s picture

Status:Reviewed & tested by the community» Needs work

This looks good in general. Are there specific issues open that are blocked on this? Would be good to see how it integrates more.

A few things came up, nothing major:

+++ b/core/lib/Drupal/Component/Plugin/Context/Context.phpundefined
@@ -0,0 +1,82 @@
+  /**
+   * Implements \Drupal\Component\Plugin\Context\ContextInterface::validate().
+   */
+  public function validate($value) {
+    // Check to make sure we have a class name, and that the passed context is
+    // an instance of that class name.
+    if (is_string($this->contextDefinition)) {
+      if ($value instanceof $this->contextDefinition) {
+        return $value;
+      }
+      throw new ContextException("The context passed was not an instance of $this->contextDefinition.");
+    }
+    throw new ContextException("An error was encountered while trying to validate the context.");
+  }
+

$this->contextDefinition can be either a class name or an array, but this always throws an exception if it's an array. I got to where arrays are allowed below, but this needs better documentation as to why that's the case here, or possibly a bit of re-jigging.

+++ b/core/lib/Drupal/Component/Plugin/ContextAwarePluginBase.phpundefined
@@ -0,0 +1,110 @@
+  public function getContexts() {
+    $definition = $this->getDefinition();
+    // If there are no contexts defined by the plugin, return an empty array.
+    if (empty($definition['context'])) {
+      return array();

There's a getContextDefinition() method on this class, which handles some of this check, sort of, but it's not used here. That kind of suggests that some of the methods here aren't really needed by calling code either. However since it's not really used anywhere yet, maybe they are, so we could just see how it goes.

+++ b/core/lib/Drupal/Component/Plugin/Exception/ContextException.phpundefined
@@ -0,0 +1,15 @@
+/**
+ * An exception class to be thrown for context plugin exceptions.
+ */

extending exception already means implemention ExceptionInterface, not necessary to state that explicitly.

+++ b/core/lib/Drupal/Core/Plugin/Context/Context.phpundefined
@@ -0,0 +1,68 @@
+  /**
+   * Overrides \Drupal\Component\Plugin\Context\Context::getContextValue().
+   */
+  public function getContextValue() {
+    $typed_value = parent::getContextValue();
+    // If the data is of a primitive type, directly return the plain value.
+    // That way, e.g. a string will be return as plain PHP string.
+    if ($typed_value instanceof \Drupal\Core\TypedData\TypedDataInterface) {
+      $type_definition = typed_data()->getDefinition($typed_value->getType());
+      if (!empty($type_definition['primitive type'])) {
+        return $typed_value->getValue();
+      }
+    }
+    return $typed_value;
+  }
+
+  /**
+   * Gets the context value as typed data object.
+   *
+   * @return \Drupal\Core\TypedData\TypedDataInterface
+   */
+  public function getTypedContext() {
+    return parent::getContextValue();

OK this confused me and I had to read it more than once. The overridden method returns the raw value for consistency. Then there's a new method that returns the value as TypedData. It makes sense but I had to read back up to getContextValue() to figure it out. Could use more comments - i.e. explicitly point out that parent::getContextValue() skips the additional processing we had to add in getContextValue() which is stripping the TypedData stuff but that we're making it available both ways.

+++ b/core/lib/Drupal/Core/Plugin/Context/Context.phpundefined
@@ -0,0 +1,68 @@
+  /**
+   * Override for \Drupal\Component\Plugin\Context\Context::validate().
+   */
+  public function validate($value) {
+    // Check to see if we have a typed data definition instead of a class name.
+    if (is_array($this->contextDefinition)) {
+      $typed_data_manager = new TypedDataManager();
+      $typed_data = $typed_data_manager->create($this->contextDefinition, $value);
+      // If we do have a typed data definition, validate it and return the
+      // typed data instance instead.
+      $violations = $typed_data->validate();
+      if (count($violations) == 0) {
+        return $typed_data;
+      }
+      throw new ContextException("The context passed could not be validated through typed data.");
+    }
+    return parent::validate($value);

So this is where the array gets accepted. I don't really like that we have a mixed type hint on the component, with a base class that throws errors on one of the options when it validates, then have to override it to let it do what we actually wanted all along.

The only solution I can think of for this though would be making validate() abstract and doing all the work in here. Potentially with a protected validateClassDefinition() helper on the base class that's not part of the interface. That way the code is re-usable but it's not saying one thing on one method and doing another thing in the other. If there's some reason why that's not acceptable, then this needs a lot more documentation.

EclipseGc’s picture

StatusFileSize
new3.66 KB
new31.04 KB
PASSED: [[SimpleTest]]: [MySQL] 49,394 pass(es).
[ View ]

$this->contextDefinition can be either a class name or an array, but this always throws an exception if it's an array. I got to where arrays are allowed below, but this needs better documentation as to why that's the case here, or possibly a bit of re-jigging.

I attempted to just document this better. I changed the docblock type hint to simply mixed since we could support anything through subclassing, and then went on to say essentially "you can support anything through subclassing." Hopefully that clarifies it. Trying to maintain a non-drupal component logic and a Drupal Core logic that all work occasionally look weird, I'll admit. I initially said just string|array here because we are using annotation, but that's not actually a legitimate limitation, so I tried to document accordingly.

There's a getContextDefinition() method on this class, which handles some of this check, sort of, but it's not used here. That kind of suggests that some of the methods here aren't really needed by calling code either. However since it's not really used anywhere yet, maybe they are, so we could just see how it goes.

This seems totally legitimate to me. I spent a little time looking through that file trying to remove as much duplicate logic as possible and just use the methods on the class instead. There was at least one other instance where I was doing the same sort of thing. Fixed both.

extending exception already means implemention ExceptionInterface, not necessary to state that explicitly.

This is actually implementing Drupal\Component\Plugin\Exception\ExceptionInterface which is an empty interface that we can use within plugins to determine whether the exception we're getting (generically) is a plugin system specific exception or not.

OK this confused me and I had to read it more than once. The overridden method returns the raw value for consistency. Then there's a new method that returns the value as TypedData. It makes sense but I had to read back up to getContextValue() to figure it out. Could use more comments - i.e. explicitly point out that parent::getContextValue() skips the additional processing we had to add in getContextValue() which is stripping the TypedData stuff but that we're making it available both ways.

Added additional docs to hopefully make this less of a WTF.

So this is where the array gets accepted. I don't really like that we have a mixed type hint on the component, with a base class that throws errors on one of the options when it validates, then have to override it to let it do what we actually wanted all along.

The only solution I can think of for this though would be making validate() abstract and doing all the work in here. Potentially with a protected validateClassDefinition() helper on the base class that's not part of the interface. That way the code is re-usable but it's not saying one thing on one method and doing another thing in the other. If there's some reason why that's not acceptable, then this needs a lot more documentation.

I'd prefer to solve this through documentation if possible. I think the docs I added in leu of your first quoteblock might be sufficient, but I'd happily expand it. I'd like to keep a working drupal-external component if possible, and simple instanceof validation makes sense there for the moment in my mind. If you really feel contrary to that then let's discuss it on irc, but as I said, I'd really like to solve this wtf with more docs if necessary.

Eclipse

EclipseGc’s picture

Status:Needs work» Needs review
effulgentsia’s picture

Are there specific issues open that are blocked on this?

#1743686: Condition Plugin System

EclipseGc’s picture

StatusFileSize
new7.83 KB
new29.59 KB
PASSED: [[SimpleTest]]: [MySQL] 49,420 pass(es).
[ View ]

fago suggested I just move to array('class' => $class) instead of dealing with strings as classes separately and such. This made good sense, so I went ahead an did it. This unifies context definitions as an array, you still subclass to add additional handling, but it should be clearer now, and has type-hinting. I also found that my comments on the context tests still contained the comments from the derivative class that I copied. So I fixed that. I'm hoping this overcomes most of catch's previous issues.

Eclipse

effulgentsia’s picture

Status:Needs review» Reviewed & tested by the community

This was RTBC before, and I think catch's feedback was well addressed, so back to RTBC.

I'm concerned with the Drupal/Core/Plugin/Context/Context class's TypedData integration. validate() sometimes wraps the object in a TypedData object, sometimes not. And getContextValue() sometimes unwraps it, sometimes not. And these times are not symmetrical, so there's lots of ways to end up with a getContextValue() that returns something unintentionally wrapped or unwrapped differently than what was passed to setContextValue(). Just as one example, if you currently change one of the tests to define the node context as array('type' => 'entity', 'constraints' => array('EntityType' => 'node')) instead of array('class' => 'Drupal\node\Plugin\Core\Entity\Node'), it will break, because getContextValue() will return an entity wrapper around $node rather than $node itself. That specific example is being handled in #1868004: Improve the TypedData API usage of EntityNG, but regardless of that issue, the asymmetry of TypedData integration in getContextValue() and validate() will cause problems.

However, I think we can hash that out in a follow up.

EclipseGc’s picture

appreciate the RTBC, thinking about this further, I think once #1868004: Improve the TypedData API usage of EntityNG is in, since we'll be passing a TypedData object when we pass an entity, we will need to check to see if a typed data object and validate differently. They still won't be symmetrical code wise, but should be symmetrical in their output. (at least, that's the theory).

Still we're stuck waiting on the other issue before we can even address it, and I think we can code around that easily enough until the issue is squared away.

Eclipse

Dries’s picture

I read all the comments and looked at the patch. The code itself looks good, however, I don't see a (link to a) discussion of why we need an abstracted context system, vs having explicit setter/getters in Plugins. Having explicit setters/getters (e.g. $userBlock->setUser($user)) would be much better, if possible. So while the code is ok, I'd like to better understand why we have this architecture.

Having explicit setters/getters may translate into a bit more work for plugin developers, but it provides a much better developer experience / API for the developers who actually have to use the plugins in their code. It seems to me like we want to optimize for the latter instead of the former.

I understand both can co-exist and that having this abstract context system does not rule out explicit setters/getters, but I'm afraid it sets the wrong best-practice for developer.

Dries’s picture

Here is what I mean (pseudo-code):

class MyUserBlock extends Plugin {

  public function setUser(User $user) {
    $this->user = $user
  }

  public function getUser() {
    return $this->user;
  }

  ...
}

Seems simple enough, and makes it easier for people to understand what is going on in MyUserBlock.

Dries’s picture

I talked to Kris a bit and it sounds like the 90% use case is for these context to be machine driven, meaning they would be set programmatically. I'm not 100% sure that justifies a more abstract API, but it could. I'm still not clear why we haven't considered explicit setters/getters. Seems worthy a conversation before we add another abstract system to core.

Crell’s picture

The reason to not use a specific setter is scale. Sure you could have a setUserObject() method, and a setNodeObject() method, but there's an arbitrary number of possible possible things to be set, and they need to be settable programmatically. That means the dynamic code that is setting up the plugin needs to know about a potentially arbitrary number of purpose-built methods. That doesn't scale. With a common setter, it can just call setContext() within a loop and be done with it.

Dries’s picture

Crell: Could you be more specific about an "arbitrary number of possible possible things". For example, do you have a good example of that that would help me understand this? I think that would help me understand.

effulgentsia’s picture

I'll take a stab at one example, though Crell or others might have other ones.

Suppose you're building a social networking site, and you have a block that takes 2 contexts called 'user1' and 'user2', each of type 'user'. The functionality of the block is to have a button that when you click it, sends a message to each one, introducing them to the other, similar to how you can introduce friends on Facebook.

There's multiple ways the site builder might want to configure which users are the contexts. One possibility is that when viewing a user profile page, 'user1' should be the user whose profile is being viewed and 'user2' should be the user who last commented on a post that user1 authored.

If this block has methods setUser1() and setUser2(), how would a generic block context configuration UI know that? What this patch allows is for each block to identify all the names and types of its contexts, so that a context configuration UI can provide the correct interface to the administrator to configure those contexts, and then at runtime, set the correct values.

EclipseGc’s picture

+1000000000 to what effulgentsia just described. Really you can name a "context" locally on the plugin anything you like. If you were displaying multiple nodes (say Artist and Album) you might name the contexts as such. They're both still nodes, but a simple setNodeContext() isn't going to suffice. You'd need setAlbum/ArtistContext() methods, and we'd have to do some sort of magic getter/setter naming and... yeah it'd be ugly. With the api I've proposed here it's simply $plugin->setContextValue('album', $album_node)->setContextValue('artist', $artist_node); and we're done. Easy enough to toss into a foreach of some sort because the Plugin definitions are going to map exactly to that first parameter.

Hopefully that explains the level of abstraction some.

Eclipse

fago’s picture

I talked to Kris a bit and it sounds like the 90% use case is for these context to be machine driven, meaning they would be set programmatically.

Yes, but we need to be able to pre-configure context values via UI (or generally config) - and for that to work in a general fashion we need the metadata as noted above. For proper pre-configuration of context values / parameters via an UI or configuration to work out we actually need to have
- an API to get/set values
- an API to validate values independently of form submissions
- metadata that is expressible enough to allow for input-form-generation to pre-input a value and/or to pre-process values

I do not say we have to solve all those issues in core, but at least it has to be feasible to build upon the core API in contrib. I can say that this resembles Rules' architecture around action/condition parameters (but based on a better API), so we know it's feasible that way and can be the base for Rules' parameter configuration system. That way this will bring the two parallel worlds of parameter/context configuration of Drupal 7 together in a single API both can leverage. This will not only safe us tons of duplicated (and rather complex) code, it will lead to a better solution but also enables both worlds to integrate with each other in ways we do not even think of today :) (well I'd have some ideas)

By basing our context definitions on typed data definitions we get proper validation for free, such that you can specify your validation constraints just part of the context/data definition and it will be picked up natively. Thus, this gives us all three points: APIs to get/set values, for validation and metadata.

catch’s picture

Assigned:EclipseGc» Dries

I think the abstraction makes sense here as well, but giving Dries another chance to come back on this.

Dries’s picture

Status:Reviewed & tested by the community» Fixed

That example helpful, and made me understand the design. Committed to 8.x. Thanks all.

cosmicdreams’s picture

Whoohooo! great work everybody!

cosmicdreams’s picture

Now that this is in, do we have specific areas within core where we plan to use this?

a_c_m’s picture

*fluff comment warning / nothing to see here* - First time i've followed a thread like this on a core patch/feature. Wanted to just say, hats off to you all. Great process, great teamwork, impressive result.

fago’s picture

Great! This is being used in the latest patch from #1743686: Condition Plugin System.

andypost’s picture

I think this require a change notice!

xjm’s picture

Title:Contextual Plugins and supporting code» Change notice: Contextual Plugins and supporting code
Category:feature» task
Priority:Major» Critical
Status:Fixed» Active
Issue tags:+Needs change record
webchick’s picture

Assigned:Dries» Unassigned

Don't think Dries needs to be on this anymore.

Since we're currently at about double the number of critical tasks in the queue, it'd be great to get a change notice for this.

phenaproxima’s picture

I'm having a go at a change notice for this. (Disclaimer: it's my first time.)

phenaproxima’s picture

Assigned:Unassigned» phenaproxima

Sorry, forgot to assign to myself.

amanire’s picture

Assigned:phenaproxima» Unassigned

I'm reviewing the history now and taking a crack at the change notice.

amanire’s picture

Woops, should've refreshed my browser. Sorry to step on your toes there, phenaproxima.

phenaproxima’s picture

Assigned:Unassigned» phenaproxima
Issue tags:+SprintWeekend2013

Adding the #SprintWeekend tag.

@amanire - no worries :) I wouldn't mind any help I can get!

xjm’s picture

Assigned:phenaproxima» Unassigned
phenaproxima’s picture

Status:Needs review» Active

Proposed change notice:

Impacts
Module developers writing plugins.

Summary
None of this stuff exists in D7 core. Several new classes and interfaces were added in this change to implement context-awareness for plugins:

\Drupal\Component\Plugin\Context\ContextInterface
\Drupal\Component\Plugin\Context\Context
\Drupal\Component\Plugin\ContextAwarePluginInterface
\Drupal\Component\Plugin\ContextAwarePluginBase
\Drupal\Core\Plugin\Context\Context
\Drupal\Core\Plugin\ContextAwarePluginBase

API Changes
Plugins that need to be context-aware (i.e., requiring a node or user or something similar in order to function) should either extend an implementation of ContextAwarePluginBase, or implement ContextAwarePluginInterface themselves. They also need to mention any contexts they use in their plugin definition. Here's an example (the only one in core, at the time of this writing) from core/modules/node/lib/Drupal/node/Plugin/Core/Condition/NodeType.php:

/**
* Provides a 'Node Type' condition.
*
* @Plugin(
*   id = "node_type",
*   label = @Translation("Node Bundle"),
*   module = "node",
*   context = {
*     "node" = {
*       "type" = "entity",
*       "constraints" = {
*         "EntityType" = "node"
*       }
*     }
*   }
* )
*/

The contexts themselves are wrapped by an implementation of ContextInterface, which defines basic get/set/validate methods. ContextInterface's constructor expects a context definition array, which specifies the kind of data to be wrapped using the Typed Data API's syntax.

As you can see, there are two versions of Context and two versions of ContextAwarePluginBase. The only difference between them is that the core version of Context supports the Typed Data API, and the core version of ContextAwarePluginBase uses the core version of Context.

Notes
When implementing context-aware plugins, the best practice is to build a base class for your plugin type that extends the core ContextAwarePluginBase.

xjm’s picture

Status:Active» Needs review

Marking for review. Hopefully @EclipseGc can give it a look.

EclipseGc’s picture

The only important points here are that it's advisable to build a base class for your plugin type that extends the Core ContextAwarePluginBase and a note that syntax for defining contexts that are Typed Data is actually TypedData's own syntax, so more information can be gleaned by looking there.

Otherwise this looks pretty good.

Eclipse

phenaproxima’s picture

Updated to reflect your suggestions - is this good to go? If so, I'll add the "official" change record.

EclipseGc’s picture

Status:Active» Needs review

Close enough for jazz. :-D

phenaproxima’s picture

Change notice posted at http://drupal.org/node/1938688

fago’s picture

The change notice looks good to me. Just one thing:

As you can see, there are two versions of Context and two versions of ContextAwarePluginBase. The only difference between them is that the core version of Context supports the Typed Data API, and the core version of ContextAwarePluginBase uses the core version of Context.

This should clarify the difference to the non-core version, i.e. the Component version does not rely on typed data.

phenaproxima’s picture

Revised the change notice to include that point.

podarok’s picture

Title:Change notice: Contextual Plugins and supporting code» Contextual Plugins and supporting code
Category:task» feature
Priority:Critical» Major
Status:Needs review» Fixed
Issue tags:-Needs change record

#92 looks good for me
thanks!

xjm’s picture

Issue tags:+Needs change record

Thanks @phenaproxima et al!

Status:Fixed» Closed (fixed)

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

jibran’s picture

xjm’s picture

Issue tags:-Needs change record

Another one to blame on d.o.