Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
The flood control functions are only used by contact.module and user.module in core. Let's move the functions to an autoloaded class so we're not loading code in common.inc that we don't need.
Comment | File | Size | Author |
---|---|---|---|
#52 | 1795186-make_flood_control_pluggable-52.patch | 20.87 KB | paranojik |
#52 | interdiff-49-52.txt | 2.45 KB | paranojik |
#49 | 1795186-make_flood_control_pluggable-49.patch | 20.83 KB | paranojik |
#49 | interdiff-47-49.txt | 1.46 KB | paranojik |
#47 | 1795186-make_flood_control_pluggable-47.patch | 20.77 KB | paranojik |
Comments
Comment #1
cweagansComment #2
Lars Toomre CreditAttribution: Lars Toomre commentedThis is a great idea @cweagans.
Can we enhance the documentation in the new Flood.php class to match D8 documentation standards? In particular, that would mean adding type hinting to all @param and @return directives as well as starting all variable descriptions that have a default with the phrase '(optional)' as well as stating what the default option is. Such care greatly improves how the class appears in the API documentation system. Thanks!
Comment #3
catchYeah this is really something that should exist as a standalone class. Few thoughts though:
- every static method is taking $name. We can move that into a constructor and remove the static from the methods
- there should be an interface, then a database implementation of the interface to allow for alternative implementations. Ideally we could inject the database connection into the class as well, although not sure there's any existing examples of that in core yet.
- register the flood system to the DIC similar to #1764474: Make Cache interface and backends use the DIC. I'm a bit concerned about this though since that registration is going to be more expensive than just loading the procedural code would have been, and this is rarely used. So maybe we don't want to register this to the DIC for now, not sure.
- add a garbageCollection method to the flood class, and call that from system_cron() instead of the direct database query that's there now.
- not in this patch but in a follow-up, it'd be great if the flood API could take care of it's own storage similar to #1167144: Make cache backends responsible for their own storage.
Comment #4
cweagansI'm not sure I agree with this. That feels way over-engineered for what this code is used for. Plus, having the methods static makes a lot of sense when you look at how the methods are used: simple one-line checks, rather than having to instantiate the class first. If this is really a hard requirement to moving cruft out of common.inc, I guess I'll do it, but it doesn't sound like a good idea to me.
@Lars, I won't have time to get back to this in the next couple days, but feel free to reroll the patch with the changes that you'd like.
Comment #5
catchThe difference with the non-static methods is you can then inject the flood control into unit tests and/or swap out the storage. I agree there's not much reason to do it otherwise but those two are enough.
Comment #6
Lars Toomre CreditAttribution: Lars Toomre commentedI am working on this issue. It is my first experience wiring objects with interfaces into Drupal's procedural code.
I have created FloodInterface which the new FloodDatabase implements. However, I am unsure how to use it.
For instance, the patch in #1 did this in contact.pages.inc:
With $name now in the constructor of FloodDatabase, I think we want to do something like:
Is this correct? The $flood assignment confuses me.
I am wondering too whether the database implementation should be called FloodDatabase or just Flood. Thoughts??
Comment #7
Lars Toomre CreditAttribution: Lars Toomre commentedAttached is the first draft of a patch that addresses all of the points that @catch raised in #3 (including the last one).
I expect this patch to fail however, as I am unsure how we want to initiate the class so that it swapable. Hence, I left the rest of the static version's implementation as it was from #1.
It also is missing the new file flood.setting.yml file because I do not know what directory to put that function in.
All comments are welcome on how to proceed further.
Comment #8
cweagansComment #10
Lars Toomre CreditAttribution: Lars Toomre commentedFor now, #1 is the good patch in this issue. #7 is incomplete. I need some feedback before I can advance it further.
Comment #11
cweagans@catch, with the patch in #1, everything works exactly as it did before, but the code doesn't live in common.inc anymore. That's the goal of this issue.
Can we keep the re-engineering for a separate issue? Pretty please?
Comment #12
Lars Toomre CreditAttribution: Lars Toomre commentedI like the idsea of committing #1 and then moving #3 through #7 to a follow-up issue.
Comment #13
chx CreditAttribution: chx commentedyes to the issue but totally no to Flood:: . You wanted drupal_container()->get('flood')->....
Comment #14
chx CreditAttribution: chx commentedAnd/or a plugin. Yes. I didn't write a D8 plugin yet...
Comment #15
Lars Toomre CreditAttribution: Lars Toomre commented@chx: Off topic, I would enjoy your thoughts and/or input on turmimg actions into a plug-in. #1788104: Convert actions to plugin sub-system
Comment #16
BerdirAs suggested in #1801962: [meta] Convert custom non-volatile cache-alike things to k/v store, maybe we can use the upcoming expirable key value store for this?
Comment #17
amonteroIf flood has to provide some config UI, http://drupal.org/project/flood_control might prove useful.
Comment #18
marcingy CreditAttribution: marcingy commentedI'll take this, this should be a simple to make it plugable if we base it off file managed conversion.
* Create interface
* Convert the current code into a class
* Add a flood function to instanate the class
Key is lets convert and then do other changes afterwards.
Comment #19
paranojik CreditAttribution: paranojik commentedThis is my take on this patch. It's basically as simple as #1, but implements some suggestions from #3 (and #13).
Pluggability can still be addressed in a follow-up issue.
Comment #21
paranojik CreditAttribution: paranojik commented#19: 1795186-move_flood_control_to_class-18.patch queued for re-testing.
Comment #23
marcingy CreditAttribution: marcingy commentedSimple port of what we have now making the storage plugable.
Comment #25
marcingy CreditAttribution: marcingy commentedComment #27
marcingy CreditAttribution: marcingy commentedOk there was issue in test but beyond that not sure why enabling the module fails
Comment #28
Lars Toomre CreditAttribution: Lars Toomre commentedWhen is the appropriate point to do a documentation oriented review of this issue? The patch in #27 has a number of issues that should be addressed. However, that patch appears to be still under development.
Comment #29
marcingy CreditAttribution: marcingy commented@lars feel free to make docs reviews, it is passing locally so at the moment it is a fight with the bot, and also feel more than free to fix it up in a patch of your own.
Comment #30
cweagansI don't think we need this. It's handy, but it's extra cruft we don't need in common.inc
Also, can you turn on your editor's "Trim trailing whitespace" option and go through and resave all those files?
Comment #31
paranojik CreditAttribution: paranojik commentedContinuing the work from #27:
-renamed DatabaseFloodBackend to FloodDatabaseBackend - seems more appropriate,
-added the garbageCollection() method and removed the query from system_cron,
-also removed $table parameter of FloodDatabaseBackend as I don't see how this could be changed anyway,
-removed flood() from common.inc
Comment #33
paranojik CreditAttribution: paranojik commentedLooks like the testbot doesn't like me...
Comment #35
paranojik CreditAttribution: paranojik commented...never blame the computer :)
Comment #36
marcingy CreditAttribution: marcingy commentedLooks good to me.
Comment #37
sunAwesome patch, folks!
I have a couple of questions:
Hm. I think we need an alternate MemoryBackend implementation for the testing framework, so as to prevent tests from talking to the database when they should not. The MemoryBackend should be part of this patch, IMO.
Is there any reason for why we're not putting this into a proper Flood component instead of mixing it into Utility?
Hm. It would be good if the phpDoc would clarify who is responsible for calling it (Am I?), and when that would be appropriate to do, and what it really does.
The db implementation seems to be delete all records from the flood table, disregarding event expirations... I wonder whether this is correct?
Minor: The data types in @param should be lowercase.
Is it only me or are the method names unnecessarily verbose? :)
Why don't we just go with:
register()
clear()
allowed()
KISS? :)
What always bugged me in the registered flood event are the uncontrolled and random event identifiers — there's a huge chance for a unintended name clashes throughout contrib with these identifiers, since everyone can register any event name.
Couldn't we address that while we're already touching those lines?
The most simple way would obviously be to always put the module name first in the event identifiers, and ideally also use a dot to separate the owning module from the remaining event name; e.g.:
The more advanced option would be to add a $module or $owner argument to actually enforce a proper event "namespace", but that might be overkill; clear guidelines can be sufficient, too.
Comment #38
Berdir- Not exactly sure what we need a memory backend for?
- Also, it's @param int and not @param integer according to http://drupal.org/coding-standards/docs. Note: We have quite a few incorrect examples in core that use @param integer, most in symfony code, though. And symfony also is mixing int and integer, but has more integer than int.
- The database implementation for garbageCollection() includes the expiration condition, just like the old query? Consider me confused...
- Cleaning up the identifiers sounds good to me, there aren't that many that we need a follow-up for this.
Comment #39
paranojik CreditAttribution: paranojik commentedThanks for the reviews, guys!
I tried to address as many issues as you mentioned:
- moved from Utility to Flood component
- updated phpDocs
- renamed FloodInterface methods except for isAllowed() as I've seen this kind of naming elsewhere in core (e.g. CacheBackendInterface::isEmpty())
- cleaned up event names as suggested in #37
I don't know how to properly implement the MemoryBackend, so I've left that out...
Comment #40
BerdirThanks for the re-roll.
Suggestion: Including an interdiff makes it easier to review the actual changes. You can create those for example with the interdiff tool ( but it often fails with more complex patches) or by using branches, see http://xjm.drupalgardens.com/blog/interdiffs-how-make-them-and-why-they-.... Personally, I use a feature branch for each issue that I'm rebasing on 8.x so creating an interdiff is as easy as diffing against the previous commit.
Per our new coding standards, references to classes should always start with a \, e.g. \Drupal\...
Indendation looks off, should be two spaces of the previous line like the previous queries.
Comment #41
paranojik CreditAttribution: paranojik commentedThanks for the tip! The interdiff between #35 and #39 is non-representative, because I moved the files from Utilty to Flood and also made other changes. I'm attaching a simple diff between the two.
Comment #42
BerdirHm, instead of ideally, maybe something like this?
", optionally followed by a dot and the actual event name." + maybe an example.
Or just make that part recommended, like we're afaik doing for the state system and leave out the optionally.
Because otherwise, you first just use the module name and when you add a second flood event, you have to rename the existing one?
Other than that, this looks good to me. Maybe sun can explain why a memory backend is required. It wasn't until now, obviously? Most other such pluggable services do have one now too (cache, queue, state, keyvalue, ...), is that the argument?
Comment #43
sunIn #1774388: Unit Tests Remastered™, I had to override the service definition for the Lock backend class with its NullBackend in order to make DrupalUnitTestBase work (so as to prevent it from querying the database where no database table is).
Flood is used less often, but still by Contact and User, but also system_cron(). In particular System and User modules are very likely candidates for modules to get enabled for DrupalUnitTestBase without getting installed, so we will want to override the service definition with a MemoryBackend/NullBackend. (I'm not sure whether NullBackends are actually correct for DrupalUnitTestBase, but I'm absolutely confident that MemoryBackend implementations are.)
However, the interface here is simple enough, so I think I can perfectly add the MemoryBackend myself over there when it is needed. Of course, I'd appreciate it if you'd simply add it here, but I certainly don't want to block this issue on that. :)
Comment #44
BerdirOk, I see. I've thought about doing similar garbage collection than we're doing in the expirable key value store (trigger flag on set/delete that does garbage collection on destruct) but I don't think this makes sense here because we're doing much more set and delete than there.
A different backend that can for example rely on actual expiration of items can just leave the method empty, so that's not a problem either.
Here's how a memory implementation could work: Have a single array property and add added events in there like this:
$this->events[$name][$identifier][] = array('timestamp' => $timestamp, 'expiration' => $timestamp + $window);
clear() is a straight-forward unset(), isAllowed() would then need to loop over the events of the given name and identifier (if any) and count the events that have not yet expired with a similar check like the one in the query.
I guess we'd also need tests. Having unit tests for both the database an the memory implementation would be nice. expiration can't be simulated in the current implementation anyway, so you just need to add a few events with different identifiers and check that it allows when it should and doesn't when not. then do a clear() and it should work again for that event/identifier, but not for others. Then execute them against both backends. See the KeyValue tests for ideas.
Do you think you can implement that? If you run into blockers with the memory backend, report them and we'll figure it out or postpone it until it's required.
Comment #45
sunYup! #44 could only be simplified by doing:
isAllowed() should translate into this direct equivalent with that:
(give or take some PHP array internals I might have overlooked)
Comment #46
BerdirThis will only work if $timestamp is guaranteed to be unique, so we would be required to use microtime() instead of REQUEST_TIME.
Comment #47
paranojik CreditAttribution: paranojik commentedDoes this look like something you had in mind? The garbageCollection() method looks a bit convoluted...
Comment #48
BerdirHaven't noticed before. In #1794754: Convert ban_boot() to an event listener, there was quite the discussion about using ip_address() in services and the result was that we shouldn't. So we should probably update it here and in the database backend as well. Suggestion: Make it a constructor argument (default_identifier) and pass it in when registering the service).Forget that, looks like this was changed again, so let's not change this. On the other side, we won't have an $event to work with here, so I'm not sure. Anyway, follow-up material IMHO
This should then use microtime() as well, because otherwise it might result in unexpected results when working with small windows.
It might be possible to reverse the condition and then do something like $this->events[$name][$identifier] = array_filter()?
Also, as long as we have something that works correctly, it doesn't matter much, this isn't something that we're actually going to use in a real-world scenario.
Tests look good enough to me. Maybe we could open a follow-up to refactor them to unit tests and have a base class that is executed against both backends, similar to the key value tests.
Comment #49
paranojik CreditAttribution: paranojik commentedRedone the garbageCollection() as you suggested, and the microtime() bit...
I'll see what I can do about the tests.
Thanks for the review!
Comment #50
BerdirAh, I see the problem, that's a bit black magic :) Not sure which approach is better.
Re tests, as I said, these look fine for me to get this in, we can look into improving it in a follow-up issue.
Comment #51
marcingy CreditAttribution: marcingy commentedThe memory back end tests do not do asserts against anything.
eg
vs
Comment #52
paranojik CreditAttribution: paranojik commentedRerolled and fixed the memory backend tests. Thanks @marcingy!
Comment #53
BerdirComment #54
cweagansThis looks good, and all the feedback has been captured, so I say back to RTBC!
Comment #55
catchThis looks great. It'd be good to factor out the ip_address() calls but that's not a problem created by this patch, I opened #1847768: Remove ip_address() and committed/pushed this one. Thanks folks!
This will need a change notice.
Comment #56
plachComment #57
YesCT CreditAttribution: YesCT commentedI'll do the change notice for this.
Comment #58
YesCT CreditAttribution: YesCT commentedhttp://drupal.org/node/1848332 is the change notice.
Comment #59
sunThanks!
Unfortunately, we're missing upgrade docs for module developers there. Essentially, the change notice needs to clarify that developers need to perform these changes when interacting with the Flood API:
Comment #60
BerdirStarted working on that.
Comment #61
BerdirOk, updated the change notice, contains some more detailed description, removed irrelevant parts and added before/after code snippets.
http://drupal.org/node/1848332
Comment #62
paranojik CreditAttribution: paranojik commentedNice write-up. I think it's accurate enough.
Comment #63
chx CreditAttribution: chx commentedComment #64
Tor Arne Thune CreditAttribution: Tor Arne Thune commented