ContextInterface needs documentation

Uh... yah...

https://api.drupal.org/api/drupal/core!lib!Drupal!Core!Plugin!Context!Co...

is the interface in question. It extends

https://api.drupal.org/api/drupal/core!lib!Drupal!Component!Plugin!Conte...

(using an alias of ComponentContextInterface for the extends)

That interface's description is:

A generic context interface for wrapping data a plugin needs to operate.

It looks like the difference between the two is that the Core interface adds support for TypedData.

So how about making the description be:

Interface for plugin context data, including methods using typed data.

If that works... good novice project to make the patch?

Sounds good @jhodgdon

That I have no idea about...

Solved issue with the text proposed by jhodgdon.
"Interface for plugin context data, including methods using typed data."

Thanks for the patch! The problem is that we need to have more information added to both classes, to explain what "plugin context data" actually means.

+++ b/core/lib/Drupal/Component/Plugin/Context/ContextInterface.php
@@ -8,7 +8,30 @@
+ * Contexts are very similar to dependency injection but they have a human
+ * readable name and other features usable on the UI. Contexts must have a
+ * definition and might have a value.

I disagreed with this on IRC but was apparently ignored. I think comparing the Context API to dependency injection is not helpful at best, misleading at worst.

OK...

So first off, I think we need to add documentation explaining the generic concept of plugin context to the *component* interface, presumably without referencing concepts from Drupal. Because right now, there's no explanation at all on the component interface of what "plugin context" actually means.

Second, I think the first line patch from above that clarifies the difference between the core and component interface should be applied to the component interface. And then that interface can reference its base for the explanation, something like "See \Drupal\Core\Plugin\ComponentInterface for a description of plugin context." Or it could provide a Drupal-specific description.

Third... I have some suggestions for these descriptions, and I don't think describing it as similar to either dependency injection or setter injection is necessarily very useful.

How about this for the Component ContextInterface doc block:

Provides an interface that wraps the data a plugin needs to operate.

Plugins often change their behavior based on various data, such as session variables, the request URL, other request properties, or system state variables. Some of this data may be passed directly into the plugin class constructor method; other data is considered "external", and this is known as the "plugin context". A plugin that uses context data is called "context-aware", and should implement \Drupal\Component\Plugin\ContextAwarePluginInterface.

Context is separated out into individual objects, specified by this interface. Some types of context have values associated with them; all types of context have definitions, which are specified in classes implementing \Drupal\Component\Plugin\Context\ContextDefinitionInterface (with methods to get/set the context label, description, data type, and validation constraints).

Plugins typically obtain their context data by defining an event that they dispatch during the request, and using event subscribers to gather the needed data into context objects.

And then I would make sure there are @see links between this interface and ContextAwarePluginInterface and ContextDefinitionInterface, or maybe a note on the other two like "See \Drupal\Component\Plugin\Context\ContextInterface for an explanation of plugin context."

And then I think the Core interface can just reference the Component interface in the same way? I'm not sure after writing this that it needs a Drupal-specific explanation or not, but if it does, how about something like this for the Core ContextInterface doc block:

Provides a Drupal-specific interface that wraps the data a plugin needs to operate.

See \Drupal\Component\Plugin\Context\ContextInterface for an explanation of plugin context in general. This Drupal-specific interface, along with \Drupal\Core\Plugin\Context\ContextDefinitionInterface, override the non-Drupal generic interfaces in order to provide typed-data methods.

Usage example: Drupal block plugins use context for access and visibility conditions. \Drupal\Core\Block\BlockBase::access() calls \Drupal\Core\Block\BlockBase::getConditionContexts(), which dispatches a block-specific event asking for context. Event subscribers in the Block module (and potentially other modules that supply block condition context) subscribe to this event, and set up context objects containing the current language, the active node, the current user account, and other data. The access() method then evaluates the block visibility conditions against the context data, to determine whether the block should be shown.

So... thoughts?

Plugins often change their behavior based on various data, such as session variables, the request URL, other request properties, or system state variables. Some of this data may be passed directly into the plugin class constructor method; other data is considered "external", and this is known as the "plugin context".

I'd not say it's about plugins *changing* their behaviour, but it's about plugins working with contextual data.

Plugins typically obtain their context data by defining an event that they dispatch during the request, and using event subscribers to gather the needed data into context objects.

I disagree - it's very different how plugins get their context. But the point is that plugins do not have to *obtain* their context, the caller has to provide the plugin with necessary context, i.e. set it on it. Where the caller takes the context from is his responsibility.

I think comparing the Context API to dependency injection is not helpful at best, misleading at worst.

Yes, totally. Context should not be used for dependency injection at all, it's about contextual data - but not injected logic or services.

OK, how about this? I didn't bother with an interdiff; basically everything is different from the previous patches.

This patch still applies. Review please?

1. +++ b/core/lib/Drupal/Component/Plugin/Context/ContextInterface.php
   @@ -8,7 +8,27 @@
   + * interface. Some types of context have values associated with them; all types
   

   This isn't really accurate. Contexts, as a tool, represent a definition. The same context object can have values set on it. Empty contexts (i.e. definition only) are useful for administrative and validation purposes. Values are used during execution.

2. +++ b/core/lib/Drupal/Core/Plugin/Context/ContextInterface.php
   @@ -11,7 +11,22 @@
   + * Usage example: Drupal block plugins use context for access and visibility
   

   Just visibility. Access isn't actually defined by conditions, but visibility is calculated at the same time.

Thanks for the review! But ... Um... I don't get the difference between:

 * Context is separated out into individual objects, specified by this
 * interface. Some types of context have values associated with them; all types
 * of context have definitions, which are specified in classes implementing
 * \Drupal\Component\Plugin\Context\ContextDefinitionInterface (with methods to
 * get/set the context label, description, data type, and validation
 * constraints).

and your comment in #17 - point 1. It seems like you are saying that all contexts have a definition, and some have values. That is what I think the paragraph above says? What exactly needs to be changed?

Point #2 - OK will fix that when I figure out what needs to be fixed for point #1.

Ok, it's just it's not as arbitrary as the docs sound here. Contexts w/o a value are useful administratively. Contexts with a value are for run time. As the docs read now it's kind of "well, uh, why don't they all have values?"

That's all I'm getting at.

Eclipse

Ah, OK. How about this?

1. +++ b/core/lib/Drupal/Component/Plugin/Context/ContextInterface.php
   @@ -8,7 +8,29 @@
   + * constraints). In addition, some types of context have data values and/or
   

   "In addition, during plugin execution, required contexts must have data values."

2. +++ b/core/lib/Drupal/Component/Plugin/Context/ContextInterface.php
   @@ -8,7 +8,29 @@
   + * constraints. The definition, values, and constraints can be accessed via
   

   The mention of constraints should really happen up with the Definition since that's actually part of the definition of this context.

3. +++ b/core/lib/Drupal/Component/Plugin/Context/ContextInterface.php
   @@ -8,7 +8,29 @@
   + * typical method for doing this is for a plugin to define an event that it
   

   Plugin doesn't define the event. A module might define an event, but plugins definitely don't

4. +++ b/core/lib/Drupal/Component/Plugin/Context/ContextInterface.php
   @@ -8,7 +8,29 @@
   + * subscribers to gather the needed data into context objects.
   

   "provide" not "gather"

5. +++ b/core/lib/Drupal/Core/Plugin/Context/ContextInterface.php
   @@ -11,7 +11,22 @@
   + * Usage example: Drupal block plugins use context for visibility conditions.
   + * \Drupal\Core\Block\BlockBase::access() calls
   + * \Drupal\Core\Block\BlockBase::getConditionContexts(), which dispatches a
   + * block-specific event asking for context. Event subscribers in the Block
   + * module (and potentially other modules that supply block condition context)
   + * subscribe to this event, and set up context objects containing the current
   + * language, the active node, the current user account, and other data. The
   + * access() method then evaluates the block visibility conditions against the
   + * context data, to determine whether the block should be shown.
   

   This whole description of how to do this is about the worst solution we could implement. Yes it's how core does it right now, but it's not a paradigm people should probably emulate. :-S

I know there are a lot of criticisms in this review, let me take a pass at this and see if I can expand this documentation in a sensible way.

Eclipse

That would be great! I obviously don't know much about this stuff; was just trying to make sense of it and clean up the language for clarity. Meanwhile, status update...

Yeah, well you shouldn't have to guess what is going to make me happy from a description perspective. Let me just write it. :-)

Eclipse

:)

How's this?

Eclipse

Thanks! I read through this patch... some things I think are still confusing and/or wrong:

a) I thought someone in an earlier comment said that all context has definitions but not all has data?

+ * constraints). Additionally, during plugin execution, required contexts must
+ * have data values. The definition, values, and constraints can be accessed

b) This wording is ... awkward at best and I do not know what it means:

+ * Context-aware plugins can specify an array of context definitions keyed by
+ * context name at the plugin definition under the "context" key.  See
+ * \Drupal\Component\Plugin\Context\ContextInterface for an explanation of
+ * plugin context.

"context name **at** the plugin definition"? Hm... that came from an earlier patch, possibly mine. :) Still, needs fixing.

c)

+ * Users of plugins must provide the necessary context data to plugins. A
+ * typical method for doing this is to gather available context data before
+ * instantiating plugins and then leverage
+ * \Drupal\Core\Plugin\Context\ContextHandlerInterface::applyContextMapping()
+ * in order to map context data into the configured plugins properly. This
+ * could be done through manual inspection of the current route, through event
+ * subscribers, configuration objects, or any combination there-of.

In place of "leverage", how about just "use"?

Also "thereof" does not have a - in the middle.

I also don't understand how mapping context data has anything to do with inspection of routes etc... this paragraph seems to be a mix of telling what the user of the plugin has to do to set up the proper context data, and what the plugin does to locate it?

Yeah, so that's why, when I did an initial pass on this stuff with chx, I didn't mention any of this, cause it's sort of impossible to find a good spot for it. It should probably exist as external documentation. How about I just explain some stuff here and we see if there's not a better way/place to put things.

What is Context?

Context is an object, or array of objects that are required for a plugin to perform its primary operation. In the case of blocks this could be checking access & building the render array, in the case of conditions this would be the object(s) required in order to perform our evaluate() method. Any number of other plugin types could implement a similar need through the context system, Actions, for example, could require context in order to perform their action on the object.

Typically context is a dynamic thing that is injected during run time shortly before the plugin performs its "primary operation".

Example of Context:

If we wanted to check to node type of a node, we'd need a node in order to check the node type. (fwhew!) So the NodeType condition takes a node context. If we want to know if a user has a specific role, we must have a user object to check, thus the UserRole condition, requires a user context. (and so on and so on)

In the case of blocks, this is similar but could get even more specific. If you had a block that renders particular field instances, then you must have a node of the proper bundle in order to render that field. So if you want to render a field that only appears on articles, the required context of the block could be article nodes. This is a "constraint". Simply saying "node" is not enough, you must in fact require an article. This can be done dynamically via plugin derivatives.

WTF Is a ContextDefinition?

It's how we define what context is required. There's an annotation class that does this for us called (strangely enough) ContextDefinition and it is an adapter around another ContextDefinition class that maps very closely to the DataDefinition classes typed data uses. In this way, Drupal's plugin context and typed_data are very very similar, and allow for the same sort of notation when defining data types. These definitions then can be used to validate that the objects passed as context are indeed what the Plugin asked for.

Ok, great, WTF is a ContextInterface object then?

The ContextInterface object is an abstraction that simultaneously wraps ContextDefinitions and whatever the context object is. This gives us methods we can use to verify we got what we asked for (since we can't type hint) and various methods we can use to introspect the definition and or get the actual context object when it's present. Context Aware Plugins type hint for these objects and can leverage the definitions they carry with them to confirm they are looking at contexts that should be usable to them. The ContextHandler has a number of methods on it that can be used for these sorts of context matching purposes.

Ok, so how do we get contexts to give to the plugins?

Great question! Contrary to what other systems do, Plugins don't pretend to know what they need, they just document that they need objects that conform to X, whatever X may be (some typed_data object typically). To that end, we have to satify contextual requirements of plugins in some OTHER way. We do this via two mechanisms. The first asks Drupal what sort of contexts WILL be available. These are returned as ContextInterface objects that just have a ContextDefinition within them. They CAN have the actual object if it's available (like current user or language) but they don't have to have the actual object since it's completely unnecessary. These contexts are then used to find what plugins are eligible to operate (because contexts they can use are available, or they don't need contexts to function) and then plugins will be configured to utilize one of these contexts. This is a "mapping" process that is built and stored in the plugin's configuration.

The second mechanism collects contexts during actual operation. Plugins are then instantiated and the available contexts are mapped into the plugins, and the mapping we previously saved is used in order to fulfill the plugin's required context(s). At this point, the plugin's "primary operation" should now operate properly.

Ok, you told me how we get contexts into plugins, but how do we get contexts

As mentioned in the docs that we've batted back and forth on this issue, Drupal core leverages an event subscriber/dispatcher methodology. This works reasonably well for Drupal's block system in its current configuration, but is probably only one facet of a properly functioning context gathering utility. As part of the D8 development cycle, a lot of discussion and debate went into getting routes that know what sort of objects exist as part of their route parameters. This is great because its in a notation that Plugin Contexts can totally leverage meaning we can know there's a node/user/term/whatever object available simply by asking the current route. This isn't super useful to Drupal core yet, but for a solution like page_manager this is the equivalent of the keys to the kingdom. Page manager has historically supported configuring additional contexts. This could take the shape of asking for an arbitrary context (give me node 5) or asking for a context derived from an existing context (I want the user who is the author of the current node). And then systems like rules make contexts available arbitrarily by saying "this function we want to act upon had an node object passed to it". In this way, they can get plugins that can act on nodes and they have a node object (provided by the function) in order to satisfy their plugin's contextual requirements.

Conclusion

I hope this book made some sense of the topic. I'm super open to figuring out how to distill this into documentation that's worthwhile at the class/interface level. Let me know what you think our course of action should be.

Eclipse

Well, sounds like we need a new patch. I will not have time to work on this until Tuesday next week at the earliest, but if someone else wants to make a new patch, that would be great. In the meantime, we could also add this information to somewhere under drupal.org/developing/api (in the Drupal 8 section)?

Tagging

So should #27 be a big dockblock @ingroup plugin on the ContextInterface class?

Should someone read over the Examples plugin_example and see if it adequately demonstrates this concept?

Ok, it's been a while, and I've had to explain this a lot more, and I'm hoping I'm a little better (and less wordy) at it.

So let's start new, and try this.

Eclipse

sorry, status.

Thanks for reviving this issue!

I took a fresh look at this patch... I think it can be improved:

1. +++ b/core/lib/Drupal/Component/Plugin/Context/ContextDefinitionInterface.php
   @@ -8,7 +8,9 @@
   + * Interface used to define value objects found in ContextInterface.
   

   Hm. So when I look at ContextInterface, I find that objects implementing ContextDefinitionInterface are returned from the getContextDefinition() method. Not the *value() methods.

   So I find this one-line description of the definition interface kind of perplexing.

   It seems like ContextInterface has concepts of Definition, Value, and Constraints. And this ContextDefinitionInterface is related to the Definition concept, not the Value concept, right?

   So maybe change this one-liner to say it's for the *definition* objects?

2. +++ b/core/lib/Drupal/Core/Plugin/Context/ContextDefinitionInterface.php
   @@ -10,7 +10,9 @@
   + * Core implementation to add support for TypedData DataDefinitions.
   

   As a stand-alone one-line description, this one leaves me a bit unsatisfied, because it doesn't tell me what the interface does at all, only why there is an override of the base Component space interface in the Core space.

   Can we change it so that the one-line description of the interface still says what the interface is actually for?

3. +++ b/core/lib/Drupal/Core/Plugin/Context/ContextDefinitionInterface.php
   @@ -10,7 +10,9 @@
   + * @see \Drupal\Component\Plugin\Context\ContextDefinitionInterface
   

   I think we should also have an @see to the Core ContextInterface here?

4. +++ b/core/lib/Drupal/Core/Plugin/Context/ContextInterface.php
   @@ -11,7 +11,9 @@
   + * Core implementation to support cacheability and updated return docs.
   

   Again I think this one-liner should still say what the interface is for, and it doesn't.

5. +++ b/core/lib/Drupal/Core/Plugin/Context/ContextInterface.php
   @@ -11,7 +11,9 @@
   + * @see \Drupal\Component\Plugin\Context\ContextInterface
   

   Also add an @see to the core ContextDefinitionInterface here?

EclilpseGc and I walked through this together based on the last review.

Change in status

I guess this needs a version bump?

Eclipse

One thing which would be really nice IMHO is some kind of overview documentation which describes how the various pieces work together.

So I agree with @dawehner that what would be really nice here is a core.api.php section about the context system as a whole. Spun off #2867836: Create a core.api.php section about the Context system for that, because the actual problem that kicked off this issue is that the existing "documentation" is quite silly, simply restating the interface name in text, so we should probably resolve this first.

The docs that are there seem accurate to me, and @tim.plunkett approved of them as well.

Marking RTBC. I'll commit later once 8.3.x is out of code freeze.

Committed to both 8.4.x and 8.3.x. Agreed with the need for a more extensive doc section. The input from @EclipseGC above could be useful for that. See you in #2867836: Create a core.api.php section about the Context system? :)