In system_schema() we create a flood table. We should change Drupal\Core\Flood\DatabaseBackend to only create a table if necessary - this means that alternate flood implementations can safely remove the flood table and know that if the site swaps back to the DatabaseBackend the table will be created.

CommentFileSizeAuthor
#9 2662990-9.patch10.36 KBalexpott
#9 6-9-interdiff.txt779 bytesalexpott
#6 2662990-6.patch10.36 KBalexpott
#6 4-6-interdiff.txt442 bytesalexpott
#4 2-4-interdiff.txt2.85 KBalexpott
#4 2662990-4.patch10.17 KBalexpott
#2 2662990-2.patch7.81 KBalexpott
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

alexpott created an issue. See original summary.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Chapter Three commented
Status: Active » Needs review
FileSize
2662990-2.patch7.81 KB

One less table to create during system install.

Status: Needs review » Needs work

The last submitted patch, 2: 2662990-2.patch, failed testing.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Chapter Three commented
Status: Needs work » Needs review
FileSize
2662990-4.patch10.17 KB
2-4-interdiff.txt2.85 KB

Fixing the test fails.

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner as a volunteer and at Chapter Three commented
Status: Needs review » Needs work

Lovely!

+++ b/core/modules/contact/src/Tests/ContactPersonalTest.php
@@ -232,11 +232,6 @@ function testPersonalContactFlood() {
-    // Clear flood table in preparation for flood test and allow other checks to complete.
-    db_delete('flood')->execute();
-    $num_records_flood = db_query("SELECT COUNT(*) FROM {flood}")->fetchField();
-    $this->assertIdentical($num_records_flood, '0', 'Flood table emptied.');

+++ b/core/modules/contact/src/Tests/ContactSitewideTest.php
@@ -185,10 +185,6 @@ function testSiteWideContact() {
-    // Clear flood table in preparation for flood test and allow other checks to complete.
-    db_delete('flood')->execute();
-    $num_records_after = db_query("SELECT COUNT(*) FROM {flood}")->fetchField();
-    $this->assertIdentical($num_records_after, '0', 'Flood table emptied.');

It still seems to be that we better truncate with a check whether the table exists. We don't want to break the test when something else in the system adds flood entries.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Chapter Three commented
Status: Needs work » Needs review
FileSize
4-6-interdiff.txt442 bytes
2662990-6.patch10.36 KB

@dawehner well wouldn't that'd be a sign of unexpected testing. I think that code is overly defensive.

Fixed a missing use statement.

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner as a volunteer and at Chapter Three commented

Well yeah, this is test resilience vs. test coverage in general.

One question: is drupal_get_module_schema() considered as API or rather its return values? Theoretically something could rely on it.

  1. +++ b/core/lib/Drupal/Core/Flood/DatabaseBackend.php
@@ -80,23 +119,120 @@ public function isAllowed($name, $threshold, $window = 3600, $identifier = NULL)
+      $return = $this->connection->delete(static::TABLE_NAME)

    $return is unused 

  2. +++ b/core/lib/Drupal/Core/Flood/DatabaseBackend.php
@@ -80,23 +119,120 @@ public function isAllowed($name, $threshold, $window = 3600, $identifier = NULL)
+      //crecreate it will throw an exception. In this case just catch the

    This british english is also a bit weird these days: 'crecreate'

catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch at Third and Grove commented

drupal_get_module_schema() considered as API

Yes.

or rather its return values?

No.

That's explicitly covered in https://www.drupal.org/core/d8-bc-policy (near the top).

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Chapter Three commented
FileSize
6-9-interdiff.txt779 bytes
2662990-9.patch10.36 KB

Thanks @dawehner the 7.1 was there already. Patch attached fixes 7.2

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner as a volunteer and at Chapter Three commented
Status: Needs review » Reviewed & tested by the community

Thank you alex!

jibran’s picture

jibran
he/him
Primary language Urdu
Location Sydney, Australia
CreditAttribution: jibran commented 
+++ b/core/lib/Drupal/Core/Flood/DatabaseBackend.php
@@ -67,10 +101,15 @@ public function clear($name, $identifier = NULL) {
+    try {
+      $this->connection->delete(static::TABLE_NAME)
+        ->condition('event', $name)
+        ->condition('identifier', $identifier)
+        ->execute();
+    }
+    catch (\Exception $e) {
+      $this->catchException($e);
+    }

Inset I can understand but how can this throw an exception?

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Chapter Three commented

@jibran being called before the table created is one reason - but some networking problem with db might throw an exception

  • catch committed c938dc0 on 
    Issue #2662990 by alexpott: Flood's database backend is a core service...
catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch at Third and Grove commented
Status: Reviewed & tested by the community » Fixed

Committed/pushed to 8.1.x, thanks!

catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch at Third and Grove commented

Committed/pushed to 8.1.x, thanks!

Status: Fixed » Closed (fixed)

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

cilefen’s picture

cilefen CreditAttribution: cilefen as a volunteer commented

#2926226: The ::garbageCollection() pattern from flood/cache can cause logged MySQL errors on cron