Problem/Motivation

Currently the request service is injected into the Flood\DatabaseBackend. In Symfony 2.4 the request_stack service replaces the request service.

Proposed resolution

Inject the request_stack service instead of the request service into the flood database backend. Also add test coverage fro the Flood\DatabaseBackend backend.

Remaining tasks

None

User interface changes

None

API changes

None

CommentFileSizeAuthor
#9 2223631-use-request-stack-in-database-flood-backend-8.diff9.52 KBznerol
#3 interdiff.txt8.49 KBznerol
#3 2223631-use-request-stack-in-database-flood-backend-3.diff9.52 KBznerol
#1 2223631-use-request-stack-in-database-flood-backend.diff3.45 KBznerol

Comments

znerol’s picture

znerol commented
Status: Active » Needs review
StatusFileSize
new 2223631-use-request-stack-in-database-flood-backend.diff3.45 KB
dawehner’s picture

dawehner
Primary language German
commented 
+++ b/core/lib/Drupal/Core/Flood/DatabaseBackend.php
@@ -99,4 +100,17 @@ public function garbageCollection() {
+    if (!$request) {
+      $request = Request::createFromGlobals();
+    }

Is there a particular reason why this is done like that? it feels more like fixing a symptom than the actual underlying problem.

znerol’s picture

znerol commented
StatusFileSize
new 2223631-use-request-stack-in-database-flood-backend-3.diff9.52 KB
new interdiff.txt8.49 KB

Addressed #2. Discussed with @dawehner: We can safely expect flood control to not be used outside the kernel request/response loop.

Also converted Drupal\Core\Flood\MemoryBackend (which only seems to be used in the test case) and copy-pasted the test-case for memory-backend such that database-backend is covered also. The test strategy should probably be revisited in a follow-up, but let's not block the request-stack conversion on this.

dawehner’s picture

dawehner
Primary language German
commented
Status: Needs review » Reviewed & tested by the community
Issue tags: -Needs tests

It is quite cool that you actually added new test coverage

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 3: 2223631-use-request-stack-in-database-flood-backend-3.diff, failed testing.

znerol’s picture

znerol commented

3: 2223631-use-request-stack-in-database-flood-backend-3.diff queued for re-testing.

znerol’s picture

znerol commented
Status: Needs work » Reviewed & tested by the community

Back to RTBC, last test-failure was a test-bot-failure.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 3: 2223631-use-request-stack-in-database-flood-backend-3.diff, failed testing.

znerol’s picture

znerol commented
Status: Needs work » Reviewed & tested by the community
StatusFileSize
new 2223631-use-request-stack-in-database-flood-backend-8.diff9.52 KB

Reroll.

znerol’s picture

znerol commented
Issue summary: View changes
dawehner’s picture

dawehner
Primary language German
commented
alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Reviewed & tested by the community » Fixed

Committed 797c8de and pushed to 8.x. Thanks!

  • Commit 797c8de on 8.x by alexpott: 
    Issue #2223631 by znerol: Use request stack in database flood backend.

Status: Fixed » Closed (fixed)

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