As described in #2928007: Support external Flood (Redis, etc.) the FloodUnblockManager contains database specific code which makes it unsuitabke for Redis.
The patch in #2928007: Support external Flood (Redis, etc.) is problematic, so I propose a 2 step solution:
- 1. Add the generic FloodUnblockManager. (This issue) This will be achieved by adding the generic FloodUnblockManagerBase and the database specific FloodUnblockManagerDatabase. This should be done while ensuring that everything keeps functioning as it does in the latest release.
- 2. Add the Redis FloodUnblockManager. (#3437875: Add Redis support) This will be achieve by adding the FloodUnblockManagerRedis
| Comment | File | Size | Author |
|---|
Issue fork flood_control-3437860
Show commands
Start within a Git clone of the project using the version control instructions.
Or, if you do not have SSH keys set up on git.drupalcode.org:
Comments
Comment #2
batigolixComment #3
batigolixComment #5
batigolixComment #6
batigolixComment #7
batigolixThis is a first stab at it
Comment #8
batigolixComment #9
anybodyGuess this would be easier to review as MR?
Comment #12
elc commentedOne 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.
Comment #13
elc commentedNeed 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
Comment #14
elc commentedI 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 #3442144: Add GitLab CI template 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.
Comment #15
batigolixThe merge request is great for reviewing the code changes but already includes much more changes then I envisioned.
Let us keep tests, CI, and static code checking out of this issue and fix all of that in follow up issues.
It would be super helpful if someone could review the patch from comment #7
Comment #16
batigolixComment #17
batigolixComment #18
fabianderijkI will test the patch this week!
Comment #19
fabianderijkI've just testen the MR. Everything seems to be working fine, so for me, it is ready to be committed.
Comment #21
batigolixI commit the patch from #7 and I credited everybody that provided patches and participated in the issue #2928007: Support external Flood (Redis, etc.)
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.
Comment #22
batigolixI created an issue for the phpunit tests: #3451521: Fix phpunit tests
Comment #23
batigolix@ELC: I created this issue for the phpunit test changes: #3451521: Fix phpunit tests
Comment #24
elc commentedOne 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.
Comment #25
flyke commentedI 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
Comment #44
batigolixCrediting the people that helped out on #2928007: Support external Flood (Redis, etc.)
Comment #45
batigolixThanks @ELC: we will remove that file in #3437875: Add Redis support
Comment #46
batigolix@flyke:
#2928007: Support external Flood (Redis, etc.) 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)
#3437860: Make FloodUnblockManager generic This should be considered fixed. De code is already in the latest version.
#3437875: Add Redis support 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.
Comment #47
batigolix