The image effect form validation passes the control to the effect plugin to validate the form values. This is done by instantiating a new form state that is passd to the plugin form validator. In case of errors the errors are registered to the new virtual form state but are not passed back to the main effect form.
Copy the form validation errors back to the main effect form state.
None.
None.
None.
None.
| Comment | File | Size | Author |
|---|---|---|---|
| #15 | interdiff.txt | 780 bytes | claudiu.cristea |
| #15 | 2645784-15.patch | 2.39 KB | claudiu.cristea |
| #12 | Screen Shot 2016-01-15 at 09.28.47.png | 29.89 KB | mallezie |
Comments
Comment #2
toniteof commentedI am working on it.
Comment #3
toniteof commentedSame problem with background color validation in style rotate effect.
We should pass original $form_state through for validation, to get error message on right place.
Comment #4
gnugetUsing this patch doesn't imply to we will need to change the way to access to the $form_state values for any contrib image filter?
Because we will need to use
$data=$form_state->getValue('data'); $bgcolor = $data['bgcolor'];instead of$form_state->getValue('bgcolor');etc.Comment #5
claudiu.cristea@toniteof thank you but the fix is not exactly that. Two patches, one that is only the test to prove the bug.
Comment #6
claudiu.cristeaIS fix.
Comment #7
claudiu.cristeaSummary...
Comment #9
claudiu.cristeaSetting back to NR. The main patch passed, the test-only failed.
Comment #10
dawehnerNice, this fixes a problem and adds some test coverage
Comment #11
claudiu.cristeaFixing title.
Comment #12
mallezieThanks, this works as expected.
This can also be used in sub contrib image styles as in https://www.drupal.org/project/image_max_size_crop (where i found the bug) and also fixes it for there.
Patch itself looks great! Nothing to notice on it. And thanks for the test! So RTBC + 1
Comment #13
claudiu.cristeaComment #14
tim.plunkettThis is a symptom of #2537732: PluginFormInterface must have access to the complete $form_state (introduce SubFormState for embedded forms), which if fixed properly, would make this obsolete.
If this is committed as a workaround, it should at least reference it in an @todo.
Comment #15
claudiu.cristeaI'm setting this back to RTBC because I only added a @todo comment. It will be up to core committers to commit this as a temporary solution till #2537732: PluginFormInterface must have access to the complete $form_state (introduce SubFormState for embedded forms) lands.
Comment #16
tim.plunkettMy worry is that we're now hardcoding workarounds for 2/40 of the setters that FormState provides. Imagine 38 more of these issues, across 4-6 modules...
Why not instead spend the time to fix it once?
Comment #17
claudiu.cristea@tim.plunkett, I agree. I knew nothing about #2537732: PluginFormInterface must have access to the complete $form_state (introduce SubFormState for embedded forms) when I created this patch.
Comment #18
claudiu.cristeaClosing in favour of #2537732: PluginFormInterface must have access to the complete $form_state (introduce SubFormState for embedded forms),