Make FloodUnblockManager generic

Created on 3 April 2024, 3 months ago
Updated 24 June 2024, 2 days ago

As described in Support external Flood Postponed: needs info the FloodUnblockManager contains database specific code which makes it unsuitabke for Redis.

The patch in Support external Flood Postponed: needs info 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. ( Add Redis support Active ) This will be achieve by adding the FloodUnblockManagerRedis

Feature request
Status

Needs work

Version

2.3

Component

Code

Created by

🇳🇱Netherlands batigolix Utrecht

Live updates comments and jobs are added and updated live.
Sign in to follow issues

Merge Requests

Comments & Activities

  • Issue created by @batigolix
  • 🇳🇱Netherlands batigolix Utrecht
  • 🇳🇱Netherlands batigolix Utrecht
  • 🇳🇱Netherlands batigolix Utrecht
  • 🇳🇱Netherlands batigolix Utrecht
  • 🇳🇱Netherlands batigolix Utrecht
  • 🇳🇱Netherlands batigolix Utrecht

    This is a first stab at it

  • Status changed to Needs review 3 months ago
  • 🇳🇱Netherlands batigolix Utrecht
  • 🇩🇪Germany Anybody Porta Westfalica

    Guess this would be easier to review as MR?

  • First commit to issue fork.
  • Merge request !45[#3437860] Make FloodUnblockManager generic → (Open) created by ELC
  • Pipeline finished with Failed
    2 months ago
    Total: 198s
    #149178
  • Pipeline finished with Failed
    2 months ago
    Total: 175s
    #149232
  • Pipeline finished with Failed
    2 months ago
    Total: 468s
    #149267
  • Pipeline finished with Success
    2 months ago
    Total: 162s
    #149294
  • Pipeline finished with Failed
    2 months ago
    Total: 161s
    #149300
  • 🇦🇺Australia ELC

    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.

  • Pipeline finished with Failed
    2 months ago
    Total: 188s
    #149325
  • Status changed to Needs work 2 months ago
  • 🇦🇺Australia ELC

    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

  • Pipeline finished with Failed
    2 months ago
    Total: 165s
    #151949
  • Pipeline finished with Failed
    2 months ago
    Total: 182s
    #151960
  • Pipeline finished with Failed
    2 months ago
    Total: 165s
    #151971
  • Pipeline finished with Failed
    2 months ago
    Total: 169s
    #152263
  • Pipeline finished with Failed
    2 months ago
    Total: 242s
    #152480
  • Pipeline finished with Failed
    2 months ago
    Total: 168s
    #152493
  • Pipeline finished with Failed
    2 months ago
    #152505
  • Pipeline finished with Success
    2 months ago
    Total: 979s
    #152536
  • Pipeline finished with Failed
    2 months ago
    Total: 191s
    #152565
  • Pipeline finished with Failed
    2 months ago
    Total: 160s
    #152570
  • Pipeline finished with Success
    2 months ago
    Total: 220s
    #152576
  • Status changed to Needs review 2 months ago
  • 🇦🇺Australia ELC

    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.

  • Status changed to Needs work 2 months ago
  • 🇳🇱Netherlands batigolix Utrecht

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

  • 🇳🇱Netherlands batigolix Utrecht
  • Status changed to Needs review about 2 months ago
  • Assigned to fabianderijk
  • 🇳🇱Netherlands fabianderijk Alphen aan den Rijn

    I will test the patch this week!

  • Issue was unassigned.
  • Status changed to RTBC 27 days ago
  • 🇳🇱Netherlands fabianderijk Alphen aan den Rijn

    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,...
  • Status changed to Fixed 26 days ago
  • 🇳🇱Netherlands batigolix Utrecht

    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.

  • 🇳🇱Netherlands batigolix Utrecht

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

  • 🇳🇱Netherlands batigolix Utrecht

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

  • Status changed to Needs work 20 days ago
  • 🇦🇺Australia ELC

    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.

  • 🇧🇪Belgium flyke

    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 ?
Production build 0.69.0 2024