- Issue created by @rszrama
- ๐จ๐ดColombia i'mbatman
imbatman โ made their first commit to this issueโs fork.
- Merge request !482Issue #3538209: Introduce redesigned merchant-facing payment form with dynamic route override. โ (Open) created by i'mbatman
- ๐บ๐ธUnited States rszrama
Quick fix needed: the logic of the
use_two_step_add_payment_form
setting is backwards. If it's set toTRUE
, then we should use the old style two step form, not if it's set toFALSE
.From an update standpoint, I think it's fine for the new version of the form to be the default. Howver, it is fair to question whether or not that breaks backend interfaces or test suites for existing sites. We'll certainly document the change in the release notes, but let's also add an update hook that doesn't actually change anything but displays the following message to users who update:
Your payment add form has been updated to a new single page format. If you depend on the old two step method, you will need to set a variable in settings.php to preserve it. See the release notes for Commerce Core x.y.z for more information.
Otherwise this looks and functions great! Still ways we can improve the styling of the form (e.g., fixing order total summaries, limiting the form width, etc.), but let's consider that out of scope for this ticket.
- ๐ฎ๐ฑIsrael jsacksick
@imbatman: Thank you for this.
I haven't tested this manually, but we have to fix the broken tests. I think we need to keep the tests coverage for the existing form, since we're retaining it, while also introducing tests for the new form.
Additionally, couple of possible improvements:
- We can leverage the AjaxFormTrait instead of redefining an ajax callback etc (We need to see if this works ofc).
- From the event subscriber, let's import the class so we can do:
PaymentForm::class
(this prevents mistakes). - I'm suggesting we rename PaymentForm (which is too generic) to indicate that this form is for adding a payment for an order. So perhaps
OrderPaymentAddForm
or justOrderPaymentForm
? - We can probably remove the buildPaymentOptionLabels() helper method in favor of EntityHelper::extractLabels()
Overall, what's actually really good about the new approach is that the logic was broken down into separate methods, which allows a child implementation to override specific parts of the code.