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

Command icon 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

batigolix created an issue. See original summary.

batigolix’s picture

batigolix’s picture

Issue summary: View changes
batigolix’s picture

Issue summary: View changes
batigolix’s picture

Related issues: +#3437875: Add Redis support
batigolix’s picture

StatusFileSize
new21.24 KB

This is a first stab at it

batigolix’s picture

Status: Active » Needs review
anybody’s picture

Guess this would be easier to review as MR?

ELC made their first commit to this issue’s fork.

elc’s picture

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.

elc’s picture

Status: Needs review » Needs work

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

elc’s picture

Status: Needs work » Needs review
Related issues: +#3442144: Add GitLab CI template

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 #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.

batigolix’s picture

Status: Needs review » Needs work

The 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

batigolix’s picture

Issue tags: +finalist-sprint
batigolix’s picture

Status: Needs work » Needs review
fabianderijk’s picture

Assigned: Unassigned » fabianderijk

I will test the patch this week!

fabianderijk’s picture

Assigned: fabianderijk » Unassigned
Status: Needs review » Reviewed & tested by the community

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

  • cfdad02c committed on 2.3.x
    Issue #3437860 by catch, vanilla-bear, Grimreaper, ELC, will_frank,...
batigolix’s picture

Status: Reviewed & tested by the community » Fixed

I 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.

batigolix’s picture

I created an issue for the phpunit tests: #3451521: Fix phpunit tests

batigolix’s picture

Related issues: +#3451521: Fix phpunit tests

@ELC: I created this issue for the phpunit test changes: #3451521: Fix phpunit tests

elc’s picture

Status: Fixed » Needs work

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.

flyke’s picture

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: 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 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 catch.

batigolix credited GPZ.

batigolix credited mkolar.

batigolix credited Rob C.

batigolix’s picture

Crediting the people that helped out on #2928007: Support external Flood (Redis, etc.)

batigolix’s picture

Thanks @ELC: we will remove that file in #3437875: Add Redis support

batigolix’s picture

@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.

batigolix’s picture

Status: Needs work » Fixed

Status: Fixed » Closed (fixed)

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