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
Comment | File | Size | Author |
---|---|---|---|
#59 | context.patch | 29.59 KB | EclipseGc |
#59 | context-interdiff-do-not-test.patch | 7.83 KB | EclipseGc |
#56 | context.patch | 31.04 KB | EclipseGc |
#56 | context-interdiff-do-not-test.patch | 3.66 KB | EclipseGc |
#50 | context.patch | 28.92 KB | EclipseGc |
Comments
Comment #1
Crell CreditAttribution: Crell commentedWhat 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.
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.
This seems like it should be a PluginContextAwareInterface, so that the base class is just some default utilities.
Technical foul for calling a floating function from within the class. 5 yard penalty, still first down.
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. :-)
Comment #2
tim.plunkettI 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.
Comment #3
EclipseGc CreditAttribution: EclipseGc commentedIt'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.
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.
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.
Because one is a component because I don't want to neglect that, and the other is for Drupal core.
Eclipse
Comment #4
EclipseGc CreditAttribution: EclipseGc commentedAdded 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
Comment #5
Crell CreditAttribution: Crell commentedI see nothing here that is creating a new context or typed data object. Is that coming later, then?
Comment #6
webchickassume that was a cross-post...
Comment #7
EclipseGc CreditAttribution: EclipseGc commentedCrell
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
Comment #8
EclipseGc CreditAttribution: EclipseGc commentedAs a practical example (which the tests actually do this)
if you had a plugin of some sort that had a context notation on it:
Then you could go about instantiating that plugin thus:
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:
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
Comment #9
EclipseGc CreditAttribution: EclipseGc commentedOk, 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
Comment #11
EclipseGc CreditAttribution: EclipseGc commentedUpgrade path test failure, not mine :-)
Comment #13
EclipseGc CreditAttribution: EclipseGc commentedAdded 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
Comment #14
EclipseGc CreditAttribution: EclipseGc commentedComment #15
Crell CreditAttribution: Crell commentedWith 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:
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
Should this really be an exception rather than just an empty array?
Comment #16
EclipseGc CreditAttribution: EclipseGc commentedThat 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:
Comment #17
EclipseGc CreditAttribution: EclipseGc commentedOk, 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
Comment #19
EclipseGc CreditAttribution: EclipseGc commentedThis should apply to head. It won't pass until TypedData validation is in though.
Eclipse
Comment #20
EclipseGc CreditAttribution: EclipseGc commentedComment #21
EclipseGc CreditAttribution: EclipseGc commentedwtf, put conditions instead of context.
Comment #23
EclipseGc CreditAttribution: EclipseGc commentedFTR that one exception was expected, and is continued to be expected until #1845546: Implement validation for the TypedData API lands.
Eclipse
Comment #24
effulgentsia CreditAttribution: effulgentsia commentedComment #25
EclipseGc CreditAttribution: EclipseGc commented#21: context.patch queued for re-testing.
Requeueing to prove that this passes test now.
Eclipse
Comment #27
EclipseGc CreditAttribution: EclipseGc commentedLooks like a random failure to me :-D (if I'm allowed to be happy about those).
Eclipse
Comment #28
EclipseGc CreditAttribution: EclipseGc commentedI think this handles most of the feedback.
Comment #29
EclipseGc CreditAttribution: EclipseGc commentedI'll fix the missing newlines in the next patch, they're already fixed locally.
Eclipse
Comment #30
Crell CreditAttribution: Crell commentedPedant: 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. :-)
Grammar hiccup. I think you mean "some value, generally a class matching..."
Typically the name of a class? The comment is not clear on what I should be doing here.
"ContextuallyAware" sounds awkward. Why not just "ContextAwarePluginInterface"? (Vis, plugin that is aware of context.)
The two plurals here is awkward. I think ContextWrappers is sufficient.
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().
Comment #31
EclipseGc CreditAttribution: EclipseGc commentedOK, taking care of Crell's issues with #30
Eclipse
Comment #32
fagogeneric context? That's very generic - can we be more specific on what I need this for?
How do I know whether validation fails?
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?
No, it isn't.
Comment #33
EclipseGc CreditAttribution: EclipseGc commentedIn 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.
and
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
Comment #34
fagoYep, this should really go in the interface docs though.
Yep, makes sense - sometimes we want to unwrap, sometimes not.
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.
Comment #35
fagoReviewed documention style
Misses a lead \ - multiple times.
Should not exceed 80chars / 1 line - multiple times.
Contains + leading \ - multiple times.
This sounds complex to me - is this the general way we document that? I think I've used "Gets ..." + "Sets ..." until now.
There should be no new line between two @params.
leading \.
\. Other @see notations also - multiple times.
Comment #36
EclipseGc CreditAttribution: EclipseGc commentedOk, I think I addressed all of fago's issues.
Eclipse
Comment #37
fagook, reviewed it again:
All specified classes miss the leading \.
It says get context value, so what you get is not $context. Maybe $value or $context_value;
The class is already the context, so what is that?
again, the object is already the context. setContextValue() maybe?
should be fully qualified as the others
copy paste mistake?
Could use the better docs from the class.
array|null ?
Gets the defined contexts.?
-wrapper. Also, why not just "Gets the defined context"?
-wrapper + the returned interface needs to be documentated
The context value needs not be an object, not?
Contains + \
leading \
shuold not be two lines
- wrapper
/s/"context wrapper"/"context"
It's just for tests, but I guess it should capitilize as "User name" - others also.
The class is already the context, so what is that?
again, the object is already the context. setContextValue() maybe?
should be fully qualified as the others
copy paste mistake?
Could use the better docs from the class.
array|null ?
Gets the defined contexts.?
-wrapper. Also, why not just "Gets the defined context"?
-wrapper + the returned interface needs to be documentated
The context value needs not be an object, not?
Contains + \
leading \
shuold not be two lines
- wrapper
/s/"context wrapper"/"context"
It's just for tests, but I guess it should capitilize as "User name" - others also.
All specified classes miss the leading \.
It says get context value, so what you get is not $context. Maybe $value or $context_value;
Comment #38
EclipseGc CreditAttribution: EclipseGc commentedAll 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.
These are generally named whatever I expect to come across here ($node, $user, etc) so I'm going with $string_context.
Fair enough, changed to $contextValue and updated code appropriately.
Considering the previous change, yeah ok.
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.
?? we don't generally put leading \ on namespace. No where I've seen at least.
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
Comment #39
EclipseGc CreditAttribution: EclipseGc commentedComment #40
fagook, I had another look at it but there where still quite some glitches - so I went ahead and fixed them. Updated patch attached.
Comment #41
fagoof course, sry for that.
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?
Comment #42
fagook, 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.
Comment #43
EclipseGc CreditAttribution: EclipseGc commentedThis 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
Comment #44
fagoGreat - I think it's RTBC also.
Comment #45
fubhy CreditAttribution: fubhy commentedYou are going to hate me for this review then. Drive-By-Shooting incoming.
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.
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.
*for* wrapping data a plugin needs to operate.
Maybe better "Base class for plugins that are context aware?"
It's the return statement, oh and it's an array, alright! No need to repeat yourself!
@return array "The set context objects"
Same here. A plain "The context object" is enough.
Same here...
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."
Missing whitespace after opening php statement.
Should be "Overrides" and not "Override for".
Missing new line after opening php statements.
Comment #46
fubhy CreditAttribution: fubhy commentedOh, but looks nice conceptually. So once those nitpicks are fixed +1 RTBC :)
Comment #47
EclipseGc CreditAttribution: EclipseGc commentedok, fixed fubhy's issues.
Eclipse
Comment #48
EclipseGc CreditAttribution: EclipseGc commentedComment #49
fubhy CreditAttribution: fubhy commentedThis is still a bit weird.
A matter of personal flavor I guess. RTBC anyways if this comes back green.
Comment #50
EclipseGc CreditAttribution: EclipseGc commentedok, 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
Comment #51
fubhy CreditAttribution: fubhy commentedah yeah, good catch... still RTBC
Comment #52
fagoImprovements 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.
Comment #53
Crell CreditAttribution: Crell commentedI 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. :-)
Comment #54
fagoad #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
Comment #55
catchThis 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:
$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.
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.
extending exception already means implemention ExceptionInterface, not necessary to state that explicitly.
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.
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.
Comment #56
EclipseGc CreditAttribution: EclipseGc commentedI 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.
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.
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.
Added additional docs to hopefully make this less of a WTF.
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
Comment #57
EclipseGc CreditAttribution: EclipseGc commentedComment #58
effulgentsia CreditAttribution: effulgentsia commented#1743686: Condition Plugin System
Comment #59
EclipseGc CreditAttribution: EclipseGc commentedfago 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
Comment #60
effulgentsia CreditAttribution: effulgentsia commentedThis 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 ofarray('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.
Comment #61
EclipseGc CreditAttribution: EclipseGc commentedappreciate 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
Comment #62
Dries CreditAttribution: Dries commentedI 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.
Comment #63
Dries CreditAttribution: Dries commentedHere is what I mean (pseudo-code):
Seems simple enough, and makes it easier for people to understand what is going on in MyUserBlock.
Comment #64
Dries CreditAttribution: Dries commentedI 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.
Comment #65
Crell CreditAttribution: Crell commentedThe 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.
Comment #66
Dries CreditAttribution: Dries commentedCrell: 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.
Comment #67
effulgentsia CreditAttribution: effulgentsia commentedI'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.
Comment #68
EclipseGc CreditAttribution: EclipseGc commented+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
Comment #69
fagoYes, 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.
Comment #70
catchI think the abstraction makes sense here as well, but giving Dries another chance to come back on this.
Comment #71
Dries CreditAttribution: Dries commentedThat example helpful, and made me understand the design. Committed to 8.x. Thanks all.
Comment #72
cosmicdreams CreditAttribution: cosmicdreams commentedWhoohooo! great work everybody!
Comment #73
cosmicdreams CreditAttribution: cosmicdreams commentedNow that this is in, do we have specific areas within core where we plan to use this?
Comment #74
a_c_m CreditAttribution: a_c_m commented*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.
Comment #75
fagoGreat! This is being used in the latest patch from #1743686: Condition Plugin System.
Comment #76
andypostI think this require a change notice!
Comment #77
xjmComment #78
webchickDon'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.
Comment #79
phenaproximaI'm having a go at a change notice for this. (Disclaimer: it's my first time.)
Comment #80
phenaproximaSorry, forgot to assign to myself.
Comment #81
amanire CreditAttribution: amanire commentedI'm reviewing the history now and taking a crack at the change notice.
Comment #82
amanire CreditAttribution: amanire commentedWoops, should've refreshed my browser. Sorry to step on your toes there, phenaproxima.
Comment #83
phenaproximaAdding the #SprintWeekend tag.
@amanire - no worries :) I wouldn't mind any help I can get!
Comment #84
xjmComment #85
phenaproximaProposed 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:
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 implementContextAwarePluginInterface
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: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 ofContextAwarePluginBase
. The only difference between them is that the core version ofContext
supports the Typed Data API, and the core version ofContextAwarePluginBase
uses the core version ofContext
.Notes
When implementing context-aware plugins, the best practice is to build a base class for your plugin type that extends the core
ContextAwarePluginBase
.Comment #86
xjmMarking for review. Hopefully @EclipseGc can give it a look.
Comment #87
EclipseGc CreditAttribution: EclipseGc commentedThe 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
Comment #88
phenaproximaUpdated to reflect your suggestions - is this good to go? If so, I'll add the "official" change record.
Comment #89
EclipseGc CreditAttribution: EclipseGc commentedClose enough for jazz. :-D
Comment #90
phenaproximaChange notice posted at http://drupal.org/node/1938688
Comment #91
fagoThe change notice looks good to me. Just one thing:
This should clarify the difference to the non-core version, i.e. the Component version does not rely on typed data.
Comment #92
phenaproximaRevised the change notice to include that point.
Comment #93
podarok#92 looks good for me
thanks!
Comment #94
xjmThanks @phenaproxima et al!
Comment #96
jibranAdded #2071553: Create route and params condition.
Comment #97
xjmAnother one to blame on d.o.