As far as I can see, we currently only call the configurationFormValidate() and configurationFormSubmit() methods for plugins that actually have a non-empty form.
However, in #2414425-3: Value changes in form validation not saved when server is added Darren Oh argued:
I think a developer who writes a validation method would expect it to run.
I, myself, wouldn't think that. There's nothing to validate and submit, so why would these be called?
Granted, there might be situations where someone wants to put other code there as a mild "hack" – but then they can just use a dummy form with a single value element to ensure the validation method is called. I don't think the situation arises often enough that we have to cater more for it.
On the other hand, after a quick browse, Core doesn't seem to do this anywhere, they always call the method regardless of whether the form is actually non-empty. Which makes our behavior non-standard, which we should of course avoid.
However, they're mitigating this somewhat, at least in most cases, by not including PluginFormInterface in their specific plugin interface – and then check for each plugin whether it implements that interface, assuming then (quite naturally) that the appropriate methods should be called. I think that this is a quite sensible compromise – you don't have to have a form if you just need the validation (or submission) method, but if you don't need any of them those won't be called unnecessary either.
Of course, this would need to be well documented, to ensure that plugin implementers are aware of this behavior – something that Core, as far as I can see, once again ignores.
So, opening the floor, what is everyone's opinion on this?
The decision could then also be implemented in Drupal 7 (as long as we don't go the route via the separate PluginFormInterface) – and there, we also have the possibility to properly document it if the method isn't called in all circumstances (which we can hardly do when we're using a Core interface).
This issue was identified as a Beta Blocker for Drupal 8. We sat down and figured out the value proposition and amount of work (story points) for this issue.
Value and Story points are in the scale of fibonacci. Our minimum is 1, our maximum is 21. The higher, the more value or work a certain issue has.
Value : 2
Story Points: 5
| Comment | File | Size | Author |
|---|---|---|---|
| #12 | 2445251-12--plugin_validation_and_submission_logic.patch | 46.2 KB | drunken monkey |
Comments
Comment #1
drunken monkeyRegarding Drupal 8, I just had to tinker around in that area again. The problem was that during index form validation/submission, the used entity already contained the new values, which broke the logic in the plugins (which already had the new settings when
submitConfigurationForm()was called, thus not being able to recognize whether any values were changed).I was confused, because this seems to have changed in Core, but this behavior doesn't really seem to make sense to me for other plugin systems, either.
So, there's another question in this area: should we use Core's behavior here, or continue with the more complicated workaround I implemented now? (Suggestions for implementing this more cleanly are, of course, welcome in any case.) In the former case, we'd just have to update the
ContentEntitydatasource as well, to manually load the old index to find out whether the settings changed. (Or come up with a completely different system for reacting to such changes.)Comment #2
nick_vhComment #3
drunken monkeyThis only adds tests (and already fixes two issues discovered that way) to ensure we don't break things while re-writing this.
Comment #5
drunken monkeyRegarding how to actually re-write this, I see two options:
@todos inIndexFormto make this consistent for datasources and trackers, too.PluginFormInterfacefrom ourConfigurablePluginInterface.Since no-one else really seems to have an opinion here, I'm gonna go with 2. and implement that. But input is still welcome, we can still go with 1. or some other variant.
Comment #6
drunken monkeyOops, didn't consider that.
Comment #7
drunken monkeyThis might already work. No time to test though, since they're now kicking us out. :(
(Stable release delayed another week!)
Comment #8
drunken monkeyEven if tests pass, still needs visual confirmation of all the plugin forms – especially both with and without Javascript enabled.
Comment #10
drunken monkeyWhy didn't PhpStorm report that as an error?!?
Resulted in a fatal error when run (since there is an interface with the same name in the trait's own namespace).
Comment #12
drunken monkeyThis took incredible long, but it should finally be working.
(Also confirmed manually, both with and without Javascript.)
Comment #14
drunken monkeyA review would've been great, but I wouldn't want to review that mess of a patch either.
So, since the tests also pass and seem to cover this pretty well, I'll just commit this now and we hope for the best/wait for complaints.