Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.In the commerce_plugin_select form element, I think there's an issue with the way it's creating the plugin instance in both the validatePlugin and submitPlugin methods.
It's currently doing this:
$plugin = $plugin_manager->createInstance($target_plugin_id, $values['target_plugin_configuration']);
Which has the effect of creating the plugin using the configuration being submitted in the form, before the plugin has run its submit handler, and in the case of the validatePlugin function, before the form submission has even been validated.
This means that the plugin's submit handler itself doesn't have easy access to its original configuration. It also means doing something like leaving a configuration value set that's not a part of the plugin form isn't possible, because that value will be blank when inside the plugin's submit callback since the plugin was created with configuration straight from the form.
I think that the plugin should be created with its original configuration (as it was before the form was submitted), and it should be up to the plugin's submit handler to assign the new configuration values as appropriate.
| Comment | File | Size | Author |
|---|---|---|---|
| #13 | pluginselect_element-2859423-13.patch | 33.09 KB | mglaman |
| #7 | pluginselect_element-2859423-7.patch | 6.81 KB | mglaman |
Comments
Comment #2
bojanz CreditAttribution: bojanz at Centarro commentedGood catch! You're right.
Comment #3
mikelutzHere's a stab at fixing this. I've adjusted the submit and validation handlers, as well as in the build form. Another issue with the way this was done, was that if you were editing an existing entity with a pluginSelect, and you switched away from the current plugin and then back again, all of your configuration would be erased. With this patch, it will restore the current saved plugin configuration if you switch back to that plugin, but it won't keep track of plugin settings on the add form, or on the edit form for plugins that aren't currently saved. That might be something to add in the future, but I consider that a new feature more than a bug fix.
I've also set it to build plugins from default configurations if they don't exist rather than using unvalidated form_state values. There doesn't seem to be any reason to do so, it just ends up.
This patch also brings out a bug in the promotions module, which wasn't properly saving its values (see attached issue) I've uploaded a patch for that as well.
Comment #4
mglamanFixing status, we have a patch
Comment #5
mikelutzI rerolled this with minor coding style fixes.
Comment #6
mglamanHere's a simpler approach. We do not need to populate any config since we're just validating. Unless something is different in shipping.
Comment #7
mglamanHere's the last patch plus mikeNCM's fixes in #2860264: OrderTotalPrice/PromotionConditionBase should explicitly save configuration items to take care of it once and for all.
Comment #8
mglamanAlso: testing this along side work for #2842924: Add a product condition and #2862945: Prevent a promotion from applying if other promotions present. Conditions are saving properly, now, along with their weight and negation.
Comment #9
mglamanHere is a PR for review: https://github.com/drupalcommerce/commerce/pull/690. I didn't squash or rebase to show each commit and the three patches. Tests are green.
I'd like to run this past someone using shipping.
Comment #10
bmcclure CreditAttribution: bmcclure as a volunteer and at Top Floor commentedDoes not explicitly populating the configuration mean the plugin is created with its default (e.g. blank) values in the submit handler, or does it still get its existing stored config loaded into it?
Comment #11
mikelutzNo, with the PR, the plugin does not get its configuration loaded when the submit function is called, which is frustrating on our end because we need to know the original configuration to validate and submit. Our specific use case at the moment is api credentials that we do not want to send to the browser every time the settings form is viewed for security reasons. We can't even use form type #value, because when the form gets rebuilt on submit before going to the submit functions, the build form is called with the wrong configuration and our #value disappears right before hitting our submit function. Our current workaround is to basically run my original patch inside our build, validate, and submit functions in the plugin. We load the original entity, and create another instance of the plugin with the correct configuration which we extract and load into our current instance so that we can access it.
It's fine when adding a new shipping method, but for editing one, we really should have access to the correct configuration in the validate and submit handlers, and from a UX perspective, it seems wrong to lose all the configuration values when switching from one plugin to another.
Comment #12
mglamanBojan discussed and I'll be updating the PR to stored the original config in the element, so we can rebuild the plugin in its pristine state. I'll follow up in IRC
Comment #13
mglamanAfter TONS of debugging and help from mikeNCM we have something passing tests!
Comment #14
mglamanWas merged on GitHub, waiting for TravisCI before pushing here.
Comment #16
mglamanComment #17
mglamanThanks so much to mikeNCM for bouncing back and forth with me on this and helping reach this solution!