Make FloodUnblockManager generic

This is a first stab at it

Guess this would be easier to review as MR?

One of the testing nodes might be having issues.

The MR tests fail on "Previous Major" (Drupal 9) because the "flood" table does not exist in the database, but if the pipeline is run manually, the table is created. It's in core, so it should be installed every time. The current release passes tests triggered both manually and by the MR.

Successful pipeline run is here: https://git.drupalcode.org/issue/flood_control-3437860/-/pipelines/149294

With the goal of ensuring that all functionality should continue to work with this patch in place, there should really be tests included for the filtering changes introduced in 2.3.3 too.

Need to revert those contact module removals. It was moved to core, not killed off. Oops. Will need to figure out why that test was failing instead of removing it.

Leaving on NW to also add tests that are missing for recently added functionality:
- ip whitelist
- using the 3x filters on unblock page
- actually unblocking an ip address

I had one last try at it, but I just can't get the check of values from config to work. That's half the commits in this. At least the MR changes makes it simple to look at.

There is some kind of wild bug/sequencing which has nothing to do with what we're doing here which results in a call to BrowserTestBase::config() or \Drupal::configFactory()->get() returning stale data. When I added code to bypass the config system and check the values directly in the database, and then also get it via config, it works just fine for both. Drop the direct DB call and it then fails. As a result, I changed how those assertions are made from querying the config system, to getting the values from the returned form which has in turn pulled the values from config. Somehow the form can get up to date data, but the test instance cannot. I presume it's caching of some kind in the test instances vs making a request to the site which kinda makes sense except that it works 25 off lines above for the 'user.flood' settings check, and doing a DB query magically makes it work.

I ended up needing to fix way more tests than I thought would be needed. The fixing of tests could be separated out into a new issue. There is another tests related issue posted 📌 Add GitLab CI template Needs review which is to add the GitlabCI file which I did in this fork to allow testing pipelines to run in here. Testing & their fixes could be merged in before this issue is so that only the code related to this issue is in this MR. Or merging this all in will just make tests work too.

Once this or testing is merged, there's still a need for a new issue or two to deal with spelling, phpcs, eslint and phpstan errors/warnings.

Let us keep tests, CI, and static code checking out of this issue and fix all of that in follow up issues.

I will test the patch this week!

I've just testen the MR. Everything seems to be working fine, so for me, it is ready to be committed.

I commit the patch from #7 and I credited everybody that provided patches and participated in the issue ✨ Support external Flood Postponed: needs info

All the additional proposed changes in the merge request https://git.drupalcode.org/project/flood_control/-/merge_requests/45 have not been committed. We need to create separate issues for that.

I created an issue for the phpunit tests: 🐛 Fix phpunit tests Active

@ELC: I created this issue for the phpunit test changes: 🐛 Fix phpunit tests Active

One of the changes that was missing in the patch but that was in the MR was the removal of the old file:

- src/FloodUnblockManager.php

This is just a drive by sorry.

I cannot login to 2 projects after I've updated them to Drupal 10.3.0.
Even after truncating the flood table, I cannot login. Both projects use redis.
That's why I would like the flood_control module to support Redis.
But I am utterly confused by the issues of this module

• Issue #2928007 ✨ Support external Flood Postponed: needs info : this is old and to be ignored or do we need a patch form that for Redis support ?
• This issue (#3437860): Status is "Needs work" but the diff (MR45) cannot be applied to flood_control 2.3.x. So I'm guessing the code is already present in 2.3.x and this issue can be ignored too ?
• So this only leaves #3437875 ✨ Add Redis support Active that is needed to add Redis support ? But that issue has no diff and no patch. Even more, If I compare revisions I see no difference between that issue's issue fork and the 2.3.x branch. I probably suck at comparing revisions so I guess I made a fault there. But I really need Redis support for flood_control module, how Do I do this ?

batigolix → credited alexrayu → .

batigolix → credited andrewbelcher → .

batigolix → credited catch → .

batigolix → credited GPZ → .

batigolix → credited Grimreaper → .

batigolix → credited hmdnawaz → .

batigolix → credited KalaiyarasiThangavel → .

batigolix → credited lisastreeter → .

batigolix → credited mkolar → .

batigolix → credited MrDaleSmith → .

batigolix → credited oleksii.chystiakov → .

batigolix → credited pjovanovic → .

batigolix → credited Rob C → .

batigolix → credited sergeimalyshev → .

batigolix → credited somersoft → .

batigolix → credited sonneworks → .

batigolix → credited vanilla-bear → .

batigolix → credited will_frank → .

Crediting the people that helped out on ✨ Support external Flood Postponed: needs info

Thanks @ELC: we will remove that file in [ #3437875]

@flyke:

✨ Support external Flood Postponed: needs info This is indeed old and should be ignored. But patches could still be used aginast older versions of the module. But the patches from this issue miss many of the improvements made over the years. (for this reason I created new issues)

✨ Add Redis support Active This should be considered fixed. De code is already in the latest version.

✨ Add Redis support Active This is in progress. We just pushed a couple of commits that allow the use of Redis. But this needs thorough testing. Any help testing this would be greatly appreciated.

Automatically closed - issue fixed for 2 weeks with no activity.