Implement flood protection on payment method forms

+++ b/modules/payment/src/PluginForm/PaymentMethodAddForm.php
index c6a357b9..a17fe0d8 100644
--- a/modules/payment/src/PluginForm/PaymentMethodFormBase.php

--- a/modules/payment/src/PluginForm/PaymentMethodFormBase.php
+++ b/modules/payment/src/PluginForm/PaymentMethodFormBase.php

Makes sense to do base. I was thinking add only. But someone could pull this off if logged in and trying to add cards to their account.

Question on review: Is this sufficient to capture flood on both the one-off payment method add page AND adding a payment method during checkout? I think so, since this is a form that goes inline of the checkout pane?

Yeah this should capture all the things.

1. +++ b/modules/payment/src/Plugin/Commerce/CheckoutPane/PaymentProcess.php
   @@ -186,13 +186,13 @@ class PaymentProcess extends CheckoutPaneBase {
   -        $message = $this->t('We encountered an error processing your payment method. Please verify your details and try again.');
   +        $message = $this->t('We encountered an error processing your payment. Please verify your details and try again.');
   ...
   -        $message = $this->t('We encountered an unexpected error processing your payment method. Please try again later.');
   +        $message = $this->t('We encountered an unexpected error processing your payment. Please try again later.');
   

   Any specific reason for changing the messaging here? Doing so in this patch would also break translations.

2. +++ b/modules/payment/src/PluginForm/PaymentMethodFormBase.php
   @@ -51,12 +66,15 @@ class PaymentMethodFormBase extends PaymentGatewayFormBase implements ContainerI
   +  public function __construct(CurrentStoreInterface $current_store, EntityTypeManagerInterface $entity_type_manager, InlineFormManager $inline_form_manager, LoggerInterface $logger, FloodInterface $flood) {
   ...
   +    $this->flood = $flood;
   
   @@ -67,7 +85,8 @@ class PaymentMethodFormBase extends PaymentGatewayFormBase implements ContainerI
   +      $container->get('logger.channel.commerce_payment'),
   +      $container->get('flood')
   

   I'm going to ty and check how many things extend this class in payment gateways. I remember the last time the constructor changed it was a small pain.

   However I can't recall if we consider these constructors as BC.

3. +++ b/modules/payment/src/PluginForm/PaymentMethodFormBase.php
   @@ -111,7 +130,16 @@ class PaymentMethodFormBase extends PaymentGatewayFormBase implements ContainerI
   +    if (!$this->flood->isAllowed($this->floodKey, 9, 60)) {
   

   Can we document 9 and 60.

4. +++ b/modules/payment/tests/src/Functional/PaymentMethodTest.php
   @@ -225,12 +216,50 @@ class PaymentMethodTest extends CommerceBrowserTestBase {
   +      'payment_method[payment_details][expiration][year]' => date('Y') + 1,
   

   smort.

Instead of adding an argument to the constructor, can we use a setFlood method, and the create method does something like

$instance = new  static(...);
$instance->setFlood($container->...);
return $instance;

Preserves BC and headache in payment gateways

Just to confirm, this applies both to standalone payment method creation and embedded payment method creation, as on the checkout form, right? (If so, might want to retitle the issue to make that clear.)

Also, I think we may need to make the flood parameters configurable - 9 attempts in 60 seconds may be a reasonable default, but I can imagine different payment vendors requiring different settings.

So this passes, just needs the nits.

Per #37: this applies anywhere the payment method form is embedded – checkout, user form, any custom form. It's also actually at the base. So anytime a payment method inline form is embedded for create or update it will be utilized.

For a setting.. should it just be Settings::get() or some form UI somewhere? The flood control isn't per payment gateway, it's the system overall. I'm not for a setting because it means building a form for one value and deciding where that form has to live, etc, until there is a demand to expose the setting.

@bradjones1: re: constructor BC I still would suggest implementing the approach from Matt instead.

This approach is less intrusive and doesn't force all the gateways to tag a new version to get rid of the deprecation notice...