Add a maximum submission limit for forms

Here we go, however I'm not too sure about :

Illegal string offset '#disabled'

For :

$form_action['#disabled'] = 'disabled';

Also, should a second verification be done after the form has been submitted, in case the form is submitted by other means than pressing the submit button?

Fixed the failing test.

1. +++ b/contact_storage.module
   @@ -77,6 +84,27 @@ function contact_storage_form_contact_message_form_alter(&$form, &$form_state, $
   +
   +  // Verify if the maximum submission limit has been reached.
   +  $query = \Drupal::entityQuery('contact_message')
   +    ->condition('contact_form', $contact_form->id());
   +  $submissions = count($query->execute());
   +  $maximum_submission = $contact_form->getThirdPartySetting('contact_storage', 'maximum_submission', 0);
   +
   +  if (($submissions >= $maximum_submission) && ($maximum_submission !== 0)) {
   

   You should first check if the max submission value is 0. There's no point in doing a query when there is no limit.

   Also, this is now the total amount.

   I think what's more likely desired here is to have a limit that's per user. So you need to check limit on the user id. The description in the form should also mention that.

   Where it gets interesting is anonymous submissions. We don't want to limit it for all uid 0 together I think. So we maybe need to store the IP of the submission?

2. +++ b/contact_storage.module
   @@ -77,6 +84,27 @@ function contact_storage_form_contact_message_form_alter(&$form, &$form_state, $
   +    // Sets the error message.
   +    $form['maximum_submission_error'] = array(
   +      '#type' => 'container',
   +      '#markup' => t('This form has reached the maximum submission limit.'),
   +      '#attributes' => array(
   +        'class' => array('messages', 'messages--error'),
   +      ),
   +      '#weight' => -100,
   +    );
   

   Can you post a screenshot of how this looks?

Can you post a screenshot of how this looks?

Here's the requested screenshot. Will await community input, as discussed, regarding whether we want the form submission limit to be per user or for the total number of submissions or both.

Yes, that's not the screenshot that I meant :)

We could also hide the buttons completely in that case? Validation constraint doesn't hurt as well.

@larowlan: What do you think about per user or not? I think usually you want to limit the submissions per user. Total limit also has use cases, but I also don't want to build a full-blown event registration system here ;)

(12:04:50 AM) berdir: larowlan: hi. Just wanted to ask if you can provide some feedback on https://www.drupal.org/node/2724507#comment-11217617, then we can continue to work on that tomorrow
(12:04:51 AM) Druplicon: https://www.drupal.org/node/2724507 => Add a maximum submission limit for forms #2724507: Add a maximum submission limit for forms => 13 comments, 1 IRC mention
(12:05:42 AM) larowlan: berdir: sure will do
(12:05:50 AM) larowlan: berdir: I agree with you on the max per user
(12:06:29 AM) larowlan: berdir: is that all the feedback you need?
(12:06:54 AM) berdir: larowlan: ok. as we noticed while discussing this, we don't store the uid yet in a message. and I suppose we want to limit it per IP for anonymous users. so we need two new base fields to store that information?
(12:07:10 AM) larowlan: berdir: sounds good

Here's the requested screenshot. Will await community input, as discussed, regarding whether we want the form submission limit to be per user or for the total number of submissions or both.

So yeah for my usecase it would be a global limit, but I agree the more common usecase is per user.

Here's my attempt at this. I added a constraint on the message field of the contact_message entity type. The validate method of the constraint validation class is duplicated from the hook_form_FORM_ID_alter function. Validating access to the form by validating an entity field's input sounds wrong and this code duplication looks absolutely wrong, but I'm not exactly sure how to do this differently. Also, I was unable to write a test to verify that anonymous user has a submission limit, I guess it might have to do with (in ContactStorageTest) :

user_role_grant_permissions(DRUPAL_ANONYMOUS_RID, array('access site-wide contact form'));

The patch passes manual testing, though.

+++ b/contact_storage.module
@@ -77,6 +84,45 @@ function contact_storage_form_contact_message_form_alter(&$form, &$form_state, $
+    $uid = \Drupal::currentUser()->id();
...
+    if ($uid === 0) {
...
+        ->condition('uid', 0);
...
+        ->condition('uid', $uid);

Please use

$account = \Drupal::currentUser();
...
$account->isAnonymous()
$account->id()

1. +++ b/config/schema/contact_storage.schema.yml
   @@ -11,3 +11,6 @@ contact.form.*.third_party.contact_storage:
          label: 'Show preview button'
   +    maximum_submissions:
   +      type: integer
   +      label: 'Maximum submission limit'
   diff --git a/contact_storage.module b/contact_storage.module
   
   diff --git a/contact_storage.module b/contact_storage.module
   index 368f6f0..7686424 100644
   

   Lets rename this to maxium_submissions_user.

2. +++ b/contact_storage.module
   @@ -77,6 +84,45 @@ function contact_storage_form_contact_message_form_alter(&$form, &$form_state, $
   +      // Remove the submit and preview buttons.
   +      unset($form['actions']['submit']);
   +      unset($form['actions']['preview']);
   

   Did you try setting #access to FALSE?

   unset() can result in notices, if other code tries to access those arrays and they're completely gone then.

3. +++ b/contact_storage.module
   @@ -89,6 +135,11 @@ function contact_storage_contact_message_redirect_submit(&$form, &$form_state) {
   +
   +  // Saves the ID and IP address of the user who submitted the form.
   +  $contact_message->get('uid')->setValue(\Drupal::currentUser()->id());
   +  $contact_message->get('ip_address')->setValue(\Drupal::request()->getClientIp());
   +  $contact_message->save();
   

   This requires a re-save and the data is also not available for mails.

   There are two alternatives:

   a) An entity builder callback, that is called when the entity is built from the form values. It's still tied to the form, however.

   b) A default value callback on the field definitions, then the value is already there when we create the entity.

4. +++ b/contact_storage.module
   @@ -114,6 +165,15 @@ function contact_storage_entity_base_field_info(EntityTypeInterface $entity_type
   +    $fields['uid'] = BaseFieldDefinition::create('integer')
   +      ->setLabel(t('User ID'))
   +      ->setDescription(t('The user ID.'))
   +      ->setSetting('unsigned', TRUE);
   

   Use type entity_reference with a target_type => user setting.

5. +++ b/src/Plugin/Validation/Constraint/ConstactStorageMaximumSubmissionsConstraintValidator.php
   @@ -0,0 +1,48 @@
   +    // Verify if the maximum submission limit has been reached, for this user.
   +    $contact_form = $entity->getParent()->get('contact_form')->referencedEntities()[0];
   +    $maximum_submissions = $contact_form->getThirdPartySetting('contact_storage', 'maximum_submissions', 0);
   +    if ($maximum_submissions !== 0) {
   +      // There is a submission limit (0 = unlimited).
   +      $uid = \Drupal::currentUser()->id();
   +
   +      if ($uid === 0) {
   +        // Anonymous user, limit per submission with the same IP address.
   +        $ip_address = \Drupal::request()->getClientIp();
   +        $query = \Drupal::entityQuery('contact_message')
   +          ->condition('contact_form', $contact_form->id())
   +          ->condition('ip_address', $ip_address)
   +          ->condition('uid', 0);
   

   Can we somehow avoid to duplicate this code? Move it somewhere both places can call it and get the current submissions back?

   There have been discussions about a repository service with such methods.

All those changes should have done.

1. +++ b/contact_storage.module
   @@ -114,10 +169,41 @@ function contact_storage_entity_base_field_info(EntityTypeInterface $entity_type
          ->setReadOnly(TRUE);
    
   +    $fields['uid'] = BaseFieldDefinition::create('entity_reference')
   +      ->setLabel(t('User ID'))
   +      ->setDescription(t('The user ID.'))
   +      ->setSetting('target_type', 'contact_form')
   +      ->setDefaultValueCallback('contact_storage_contact_message_default_uid');
   +
   +    $fields['ip_address'] = BaseFieldDefinition::create('string')
   +      ->setLabel(t('IP address'))
   +      ->setDescription(t('The IP address of the submitter.'))
   +      ->setDefaultValueCallback('contact_storage_contact_message_default_ip_address');
   +
        return $fields;
   

   we need an update function to add those two fields.

2. +++ b/contact_storage.module
   @@ -114,10 +169,41 @@ function contact_storage_entity_base_field_info(EntityTypeInterface $entity_type
   +/**
   + * Returns the user id, which is stored in the contact_message entity.
   

   lets document on those two functions that they are used as default value callbacks. they seem weird if you don't know that.

3. +++ b/contact_storage.module
   @@ -114,10 +169,41 @@ function contact_storage_entity_base_field_info(EntityTypeInterface $entity_type
   + * @return int
   + *   The user ID.
   + *
   + */
   

   no empty line after return.

Here we go. I moved the test in its own function and therefor had to do a tiny bit of refactoring on the test class to extract common behavior in a setUp() function. This will be green after the modification suggested in #2780763: Add missing parameter in ContactFormCloneForm::__construct() is in.

Looks good to me.

I think we have at least 3 issues that refactor that test in a similar way. going to be a bit painful to re-roll the others, we might want to focus on getting one in and then finish the others.