Add validation constraints to contact.settings

That looks pretty good to me. No remarks; as far as I'm concerned that's RTBC. I suspect the test failure is a random failure, but I don't have permission (seemingly) to re-run the failed job...

So if you can re-run that job, and it passes, consider this me RTBCing.

The tests are passing now.

The descriptions/labels of the config are very vague, but that's a pre-existing issue.

Unfortunately that means that the validation constraints here turn out to be kinda nonsensical. Defining the minimum for user.flood:ip_window to be 1 is absurd:

   * @param int $window
   *   (optional) Number of seconds in the time window for this event (default is 3600
   *   seconds, or 1 hour).

So we're saying that the minimum window is 1 second. Why does that make sense?

The purpose of these config validation issues is not to blindly add validation constraints, but to first understand the configuration and how it is interpreted by the associated code/APIs, and then define sensible validation constraints.

Unfortunately that was a step in the wrong direction … so I explained how you can start moving this in the right direction: https://git.drupalcode.org/project/drupal/-/merge_requests/7469/diffs#no... 😊

Think your last comment does seem reasonable, guessing we could work with that.

Handled all the comments and rebased. Think this is ok.

I think this looks good, I don't think we need specific test coverage for these values.

I wonder if we should ask for a user module maintainer for a review? Will ping kristiaan.

I pinged kristiaan in slack, and in the meanwhile I figured there may be something else that we could do.
Instead of doing a min check here, we just need to know that the value is > 0, because smaller is not an option.

So I think isPositive might be a simpler way to validate this instead of being this specific.
However, waiting to put this back to needs work based on feedback.

Gave some preliminary feedback on Slack, if it's not enough then we can quickly have a look in person in Leuven a week and a half from now. My only concern is that a too loose validation would allow config to contain values that the code does not handle well (or at all). So as long as that concern is taken care of, I'm fine with this.

// isAllowed - uses ip_window
      $number = $this->connection->select(static::TABLE_NAME, 'f')
        ->condition('event', $name)
        ->condition('identifier', $identifier)
        ->condition('timestamp', $this->time->getRequestTime() - $window, '>')

The ip window is used in isAllowed and register on the Flood backends, this is could be anything as long as it is positive, I would suggest this being greater than 10 to allow for time drift between different backends if needed, but this isn't techinically required. I also can't seem to find a form where this is configurable.

User window also doesn't have any actual minimum, but it should be greater than the value in ip_window. I think we should update the constraints based on what is actually happening in code.

I dont see why we should limit how we set this. Above zero is valid, and if you like you can configure the value as you wish. The defaults are pretty high for ip window, and lower for user window.

/ Do not allow any login from the current user's IP if the limit has been
      // reached. Default is 50 failed attempts allowed in one hour. This is
      // independent of the per-user limit to catch attempts from one IP to log
      // in to many different user accounts.  We have a reasonably high limit
      // since there may be only one apparent IP for all users at an
      // institution.
      if (!$this->userFloodControl->isAllowed('user.failed_login_ip', $flood_config->get('ip_limit'), $flood_config->get('ip_window'))) {
        $form_state->set('flood_control_triggered', 'ip');
        return;
      }

Still, you should be able to configure this as you wish as long as it is a positive integer. I don't see places in the code where this would break, it would only really allow for settings that might not make sense in all instances.

I agree, so let's change the config schema to use minimum values of 0?

Not sure if 0 is a good idea. It might be possible, but could also end up devision by zero. Also don't see the value in adding 0 as an option. You sure you want 0? 1+ seems safer and make more sense.

Double checked, there is no code dividing, so guess, 0+ is fine then.

There seem to be a lot of unrelated changes in the MR. But the latest commit looks great.

I rebased