Fix MemoryBackend::clearByPrefix with multiple items, and improve coverage and clarity

The Needs Review Queue Bot → tested this issue. It either no longer applies to Drupal core, or fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".

Apart from a re-roll or rebase, this issue may need more work to address feedback in the issue or MR comments. To progress an issue, incorporate this feedback as part of the process of updating the issue. This helps other contributors to know what is outstanding.

Consult the Drupal Contributor Guide → to find step-by-step guides for working with issues.

I've rebased this onto a new MR against 10.1.x

The problem is now that 📌 Improve test coverage of the flood memory backend test and convert it to a unit test Fixed also added unit tests for the MemoryBackend and it's hard for me to figure out what needs to be ported from the new unit tests this issue was adding so I've removed them all for now

@dpi are you able to reconcile the tests?

Thanks for all the work in here!

Given that this issue is no longer about solving a major bug that potentially leads to data loss, but is now more of a cleanup task to add better test coverage, it needs:

1. A new, more accurate title.
2. A real issue summary, explaining what we're now trying to accomplish here.
3. Probably needs a change record for the constructor changes to core/lib/Drupal/Core/Flood/MemoryBackend.php
4. Probably needs a deprecation test (?)
5. I opened a couple of minor MR comments, too.

Isn't this now a normal task, not a major bug? If I'm wrong, please move it back, but re-classifying for now. But since I found it as a bug, tagging to be smashed. ;)

Thanks again,
-Derek

In progress.

New summary and title

Re #38

no longer about solving a major bug [...] now more of a cleanup task

Isn't this now a normal task, not a major bug?

I encountered a bug during creation of unit tests for the recently added clearByPrefix, so reclassifying back to bug. Noting that expanding and clarifying coverage is the primary motivation for this issue

A new, more accurate title.

Done.

A real issue summary, explaining what we're now trying to accomplish here.

Done.

Probably needs a change record for the constructor changes

There's already a CR, and linked from code via trigger_error deprecation.

Probably needs a deprecation test (?)

Not sure about that.

I opened a couple of minor MR comments, too.

Resolved updating D9->D10 references and grammar issue.

---

Tags

• Removing Needs tests, this is about test improvement.
• Removing Needs change record, already has CR
• Removing Needs issue summary update, updated per previous comment
• Removing Needs title update, updated per previous comment

Summarising new and existing code changes, especially since 📌 Improve test coverage of the flood memory backend test and convert it to a unit test Fixed and ✨ Add a clearByPrefix() method to the flood API Fixed :

• Make MemoryBackend tests clearer to read and improve coverage, inject datetime for overridability and testability (no longer need sleep)
• The English of comments and unit test method names committed with 📌 Improve test coverage of the flood memory backend test and convert it to a unit test Fixed is a little whack, so parts have been modified.
• Fixed a couple of ints passed passed to strings, and strings passed to int with $threshold and $identifier
• Improved code with [PHPStan] array shapes
• testGarbageCollection replaces testNotExpiring
• Note on change introduced for testNotExpiring:the changes in this PR where this comment is found "Progress time before window, event still exists after garbage collection."
  • It is fundamentally testing that garbage collection will not clear events as current time has not exceeded the expiry.
  • The existing check in testNotExpiring with comment ""Run cron", which clears the flood data and verify event is not allowed." is equivalent to the changes in this PR where this comment is found "Progress time before window, event still exists after garbage collection."
• Added unit test coverage for the new prefix clear functionality added in Jan 2023 in ✨ Add a clearByPrefix() method to the flood API Fixed . Presently only Kernel tests, which 📌 Improve test coverage of the flood memory backend test and convert it to a unit test Fixed already resolved to move Kernel to Unit.
  • This also uncovered a bug with \Drupal\Core\Flood\MemoryBackend::clearByPrefix only clearing the first event.

---

Back to NR

This is flood rather than cache, since we don't have a component for flood, moving to 'base'.

Took a look (correct me if I'm wrong) but there are 2 open threads on the MR that appear to request some tweaks.

Update of 2910000-mr-1451-d93 → patch so it can be applied for Drupal 9.5.x in the meantime.