Add a clearByPrefix() method to the flood API

FWIW, this might not be easy to implement for alternative backends. Or at least not efficient. Redis for example.

1. +++ b/core/lib/Drupal/Core/Flood/FloodInterface.php
   @@ -36,6 +36,16 @@ public function register($name, $window = 3600, $identifier = NULL);
   +  public function clearByPrefix($name, $prefix);
   

   Can we add typehints: Something like: clearByPrefix(string $name, string $prefix): void;

2. The new method cannot be added to the FloodInterface, because when somebody it overriding the service and they did not add the new method, they will have a BC break.
3. This new method will need testing.

A new patch which includes tests and suggestions from @daffie.

I also made a change to MemoryBackend::clearByPrefix: The second parameter passed to explode() was an array, not a string. It's the key of the array we are interested in exploding I think.

Lets see if a patch with only the added tests fails.

Back to needs review.
I will do a review later.

@eddie_c: Thanks for writing the tests.

FWIW, this might not be easy to implement for alternative backends. Or at least not efficient. Redis for example.

This is the same problem we had with cache backends, which eventually led to cache tags.

The use case is #2881572: User login flood lock doesn't clear when reset password as admin where we want to remove flood entries for a user, but the flood entries include their IP address which we don't know.

Yeah, and I assume currently we'd be forced to implement similar workarounds like we had to in the memcache module in D7, store prefix deletions and then check them on read.

The patch look good to me. Just a couple of minor remarks:

1. +++ b/core/lib/Drupal/Core/Flood/DatabaseBackend.php
   @@ -107,6 +107,26 @@ public function clear($name, $identifier = NULL) {
   +        ->condition('identifier', $prefix . '-%', 'LIKE')
   

   Can we change '-%' to '%' or is there a very good reason why the dash is added?

2. +++ b/core/lib/Drupal/Core/Flood/MemoryBackend.php
   @@ -54,6 +54,23 @@ public function clear($name, $identifier = NULL) {
   +      $identifier_prefix = explode("-", key($identifier));
   

   This looks wrong to me. Should we not do the explode() first and then get the first element with reset()?

3. +++ b/core/modules/system/tests/src/Kernel/System/FloodTest.php
   @@ -103,4 +103,46 @@ public function testDatabaseBackend() {
   +  public function testMemoryBackendClearByPrefix() {
   ...
   +    $flood = new MemoryBackend($request_stack);
   ...
   +  public function testDatabaseBackendClearByPrefix() {
   ...
   +    $flood = new DatabaseBackend($connection, $request_stack);
   

   The 2 new test method are almost the same. The only difference I can see is MemoryBackend vs. DatabaseBackend. Can we change it to a single test method with a dataProvider method. See: https://phpunit.de/manual/3.7/en/writing-tests-for-phpunit.html#writing-... for more info or do a code base search for "dataProvider".

Checking the implementations again. the thing with - is coming from this:

```
MariaDB [d8_import]> select * from flood;
+-----+------------------------+-------------+------------+------------+
| fid | event | identifier | timestamp | expiration |
+-----+------------------------+-------------+------------+------------+
| 3 | user.failed_login_ip | 127.0.0.1 | 1628326866 | 1628330466 |
| 4 | user.failed_login_ip | 127.0.0.1 | 1628326873 | 1628330473 |
| 5 | user.failed_login_user | 1-127.0.0.1 | 1628326873 | 1628348473 |
+-----+------------------------+-------------+------------+------------+
```

The user/ip combination uses - as separator. However, right now, that is an implementation detail, the flood API does, so far, not care at all what's in the identifier.

I like limiting this API somewhat. It makes more more feasible to implement in alternative backends that have no wildcard support. A bit like the memory backend, although we can expect that the number of entries there is very limited and it should IMHO be a lot easier to implement by just checking each array key.

However, in that case, this needs to be more clearly documented. We should document on the existing methods, or somewhere central that the identifier may consist of two sections split by a -. And that flood backends can optionally (at least for BC) support clearing by the first section. We should also put the new method on an optional interface I guess so that implementations can check whether that is supported.

#16 should help alternative backends a lot, also agreed we should add a new optional interface for this.

This patch reduces the duplication in tests. Still need to look at #15 point 2 and @Berdir's suggestion of adding an optional interface.

@eddie_c: Could you also post an interdiff file. It makes review your changes a lot easier.

@daffie Here's an updated patch with interdiff to the patch in #7.

The patch includes:
- The optional interface for the clearByPrefix method
- A correction which addresses your point in #15.2

Still to add: documentation

Removing unused use statement

...And correcting misspelling in comment

Thanks for the docs @paulocs. I wonder if they could be made a little bit clearer. Something like this would expand on what you wrote:

   * @param string $identifier
   *   (optional) Unique identifier of the current user. Defaults to the current
   *   user's IP address. The identifier can be given an additional prefix
   *   separated by "-". Flood backends may then optionally implement the
   *   PrefixFloodInterface which allows all flood events that share the same
   *   prefix to be cleared simultaneously.

Created a MR against 10.x branch and updated the comment description as per #26.

Rebased but didn't touch anything else, which (probably?) leaves me eligible to RTBC.

- Code changes make sense to me.
- All remarks made seem to be addressed.
- Green testbot

RTBC for me.

This needs a change record for the new interface.

Draft CR added.

Looks good now, hopefully the '-' prefix limitation will help Redis et al implement support for this without too much pain.

Committed be7d65e and pushed to 10.1.x. Thanks!

Seems we forgot to publish CR. Published it.