ConditionPluginBase::buildConfiguration() depends on a form state item set by the parent form. This makes the form less portable, and although it may not be a violation of PluginFormInterface, it means that the plugins' forms are not as easy to embed as PluginFormInterface suggests.
Let ConditionPluginBase retrieve available contexts through the context.repository service itself if no contexts are stored in the form state. This also prevents problems when #2537732: PluginFormInterface must have access to the complete $form_state (introduce SubFormState for embedded forms) is committed, because it would prevent plugin forms from accessing the global form state, and thus the globally set contexts.
The only current usage of this global form state key is in BlockForm::form(), and that is equivalent to the proposed solution.
Allow the context repository to be optionally injected to ConditionBase plugins through the constructor, and add a protected ::getContextRepository() method that uses service location in case the context repository is not injected. This causes a minor break of unit tests of code that calls ConditionPluginBase::buildConfiguration(), because the context repository is not mocked in those tests yet.
None.
None.
| Comment | File | Size | Author |
|---|---|---|---|
| #15 | drupal_2647464_15.patch | 6.89 KB | xano |
Comments
Comment #2
xanoComment #3
xanoThe original code that accessed the form state was a weird kind of integration between
BlockFormandConditionPluginBase. I'm not sure this is covered by any tests, as #2537732: PluginFormInterface must have access to the complete $form_state (introduce SubFormState for embedded forms) passes all tests, but the integration between these two classes should be broken. I'd love to be able to remove it completely, as this integration is very obscure, and we can alterConditionPluginBasewithout breaking anything as long as no other code uses this form state value directly. If we can't, do we have conventions for testing such deprecated integrations?Comment #4
xanoComment #7
xanoComment #8
xanoBlockForm::submitForm()sets context mapping values on context-aware condition plugins, but these values come from the condition plugin forms, which means the plugins must submit these values themselves, and they're none ofBlockForm's business.Comment #9
xanoI'm not enirely familiar with the context system, but it seems we'll want contexts to be set on conditions from outside through
::setContext()as well. This patch allows condition configuration forms to use contexts from three sources:BlockForm, although we may want to remove that, as the retrieval of globally available conditions from within condition plugins replaces this integration.::setContext().Comment #11
xanoThe test fails, because the
Languagecondition's context form is suddenly visible. What is the reason users are not supposed to configure this context?Comment #12
dsnopekI'm not entirely familiar with this issue, but... :-) Plugins really shouldn't be directly accessing the
ContextRepository. They should be dealing only with contexts passed to them via::setContext(). Getting the contexts out of theContextRepositoryand passing them to the plugin is the job of the thing using the plugin, and should never be done by a plugin itself.This is important because the context repository is looking at stuff from the current request. If, for example, you're editing a page in Page Manager, you want to be able to set the contexts to the values that would be used on the page you are editing, not the page with the admin form on it.
That said, I'm not really familiar with this issue, so I might be misunderstanding something. Tim Plunkett and EclipseGC know the block and context systems much better than me, so input from them would be good!
Comment #13
tim.plunkettI'm sorry but this is severely wrong. Each plugin should NOT have the context repository. What about passing them in as a parameter?
Or just suck it up and won't fix.
Comment #14
xanoThat's what @EclipseGc and I settled on when discussing this on IRC.
That's not very productive. We should fix it either in D8, or postpone to D9. Let's try the former now we have an unexplored possible solution.
Comment #15
xanoLet's try this. No interdiff, because it's a completely different approach.
Comment #17
dsnopekAs far as contexts, #15 looks way better!
However, this adds an extra argument to a method on an existing interface, and I'm not sure how the core committers will see that as far as BC. I think this would make any class that implements
ConditionInterfaceorBlockPluginInterfacewithout extending the base classes cause a fatal error because it's missing the new argument? :-/Comment #18
xanoIt does not add any extra methods, as both interfaces already extend
PluginFormInterface. The patch merely adds a third, optional parameter to an existing method, and therefore is fully backwards-compatible.Comment #19
dsnopek@Xano: Ah, ok, sorry! I thought adding an additional argument to a method on an interface (even an optional argument) would need to be implemented by implementors of the interface. Sorry for the noise!
Comment #20
berdir@dsnopek is right. An optional method on an interface is *required* for implementing classes to be specified, a method definition must match an interface exactly.
Adding optional arguments only works for classes that are extending an interface, not the other way round.
I don't really understand how this arguments makes it really better, it's still something special that you have to know about? Just document that the specified key has to be provided in $form_state, problem solved?
Comment #21
xanoComment #22
eclipsegc commentedWell ok, so just answering the points here:
1.) Agreed it's not an API. But we shipped with it, so it might be our best option for the time being
2.) Which is exactly why berdir is suggesting we document it
3.) The available contexts are just as global as the form state which is exactly why I put it there. If every single condition plugin got this same group of gathered contexts, well that's ok because it's exactly what I intended. The plugin has no notion of its SPECIFIC configured context at this point, we are in fact getting all of them so that we can go through exactly that process.
While I'd love to clean this up in some way, there's a reason that it went down like you see it and it's essentially all of the same things you're struggling against right now.
Eclipse
Comment #34
andypostThis is the only issue left to close parent meta
Comment #36
andypost