Replace calls to the flood service with Drupal::flood()

There are no more calls to drupal_container()->get('flood') but there are 3 calls to \Drupal::service('flood')

$ ack -a "service\(\'flood"

core/modules/contact/lib/Drupal/contact/MessageFormController.php
198:    \Drupal::service('flood')->register('contact', \Drupal::config('contact.settings')->get('flood.interval'));

core/modules/system/lib/Drupal/system/Tests/System/FloodTest.php
51:    $flood = \Drupal::service('flood');

core/modules/system/system.module
1737:  \Drupal::service('flood')->garbageCollection();

Let's see how this goes...

Edit: I think this is open again since #1938390: Convert contact_site_page and contact_person_page to a new-style Controller is fixed? If not, feel free to postpone again.

+++ b/core/modules/contact/lib/Drupal/contact/MessageFormController.php
+++ b/core/modules/contact/lib/Drupal/contact/MessageFormController.php
@@ -195,7 +195,7 @@ public function save(array $form, array &$form_state) {

@@ -195,7 +195,7 @@ public function save(array $form, array &$form_state) {
       drupal_mail('contact', 'page_autoreply', $sender->getEmail(), $language_interface->id, $params);
     }
 
-    \Drupal::service('flood')->register('contact', \Drupal::config('contact.settings')->get('flood.interval'));
+    \Drupal::flood()->register('contact', \Drupal::config('contact.settings')->get('flood.interval'));
     if (!$message->isPersonal()) {
       watchdog('contact', '%sender-name (@sender-from) sent an e-mail regarding %category.', array(
         '%sender-name' => $sender->name,
diff --git a/core/modules/system/lib/Drupal/system/Tests/System/FloodTest.php b/core/modules/system/lib/Drupal/system/Tests/System/FloodTest.php

Given that the patch is not really big we should inject the dependencies properly here.

Good point.

!

1. +++ b/core/modules/contact/lib/Drupal/contact/MessageFormController.php
   @@ -25,6 +28,43 @@ class MessageFormController extends ContentEntityFormController {
      /**
   +   * The entity manager.
   +   *
   +   * @var \Drupal\Core\Entity\EntityManagerInterface
   +   */
   +  protected $entityManager;
   ...
   +    $this->entityManager = $entity_manager;
   

   No reason to document this. just call parent::__construct($entity_manager);

2. +++ b/core/modules/contact/lib/Drupal/contact/MessageFormController.php
   @@ -195,7 +235,8 @@ public function save(array $form, array &$form_state) {
   +    $config = \Drupal::config('contact.settings');
   

   So we should/could also inject config? I know we would need probably a lot more but this would be good for now.

3. +++ b/core/modules/system/lib/Drupal/system/Tests/System/FloodTest.php
   @@ -48,7 +48,7 @@ function testCleanUp() {
   +    $flood = \Drupal::flood();
   
   +++ b/core/modules/system/system.module
   @@ -1731,7 +1731,7 @@ function system_get_module_admin_tasks($module, $info) {
   +  \Drupal::flood()->garbageCollection();
   

   Does anyone know why flood is part of \Drupal even it is certainly not used often in drupal sites?

1. +++ b/core/modules/contact/lib/Drupal/contact/MessageFormController.php
   @@ -25,6 +28,43 @@ class MessageFormController extends ContentEntityFormController {
   +   * The entity manager.
   +   *
   +   * @var \Drupal\Core\Entity\EntityManagerInterface
   +   */
   +  protected $entityManager;
   +
   +  /**
   +   * The flood control mechanism.
   +   *
   +   * @var \Drupal\Core\Flood\FloodInterface
   +   */
   +  protected $flood;
   +
   

   Should we document the flood control mechanism or just pass them both in __construct() ?

2. +++ b/core/modules/contact/lib/Drupal/contact/MessageFormController.php
   @@ -25,6 +28,43 @@ class MessageFormController extends ContentEntityFormController {
   +  public static function create(ContainerInterface $container) {
   +    return new static(
   +      $container->get('entity.manager'),
   +      $container->get('flood')
   +    );
   

   I thought about that but I didn't know if I COULD inject config. Will look into that.

3. +++ b/core/modules/system/lib/Drupal/system/Tests/System/FloodTest.php
   @@ -48,7 +48,7 @@ function testCleanUp() {
   +    $flood = \Drupal::flood();
        $flood->register($name, $window_expired);
        // Verify event is not allowed.
   

   Your guess is as good as mine, working on this issue is the first time I've seen the flood control mechanism.

Would using the flood control mechanism help fix errors like this one that I see every time I load the testing suite for the first time?

Edit: No, the Flood Control Mechanism does things like checking whether a user is allowed to proceed with the specified event - ie: to prevent a user from spamming the contact form.

Re #6.1 and #6.2

1. +++ b/core/modules/contact/lib/Drupal/contact/MessageFormController.php
   @@ -25,6 +29,39 @@ class MessageFormController extends ContentEntityFormController {
   +    $this->configFactory = $config_factory;
   

   Nitpick: Is there any special reason not lave out the documentation for the config factory?

2. +++ b/core/modules/contact/lib/Drupal/contact/MessageFormController.php
   @@ -195,7 +232,8 @@ public function save(array $form, array &$form_state) {
    
   -    \Drupal::service('flood')->register('contact', \Drupal::config('contact.settings')->get('flood.interval'));
   +    $config = $this->configFactory->get('contact.settings');
   +    $this->flood->register('contact', $config->get('flood.interval'));
        if (!$message->isPersonal()) {
          watchdog('contact', '%sender-name (@sender-from) sent an e-mail regarding %category.', array(
            '%sender-name' => $sender->name,
   diff --git a/core/modules/system/lib/Drupal/system/Tests/System/FloodTest.php b/core/modules/system/lib/Drupal/system/Tests/System/FloodTest.php
   
   diff --git a/core/modules/system/lib/Drupal/system/Tests/System/FloodTest.php b/core/modules/system/lib/Drupal/system/Tests/System/FloodTest.php
   index e7cbdb2..76c00b4 100644
   

   Thank you!

+++ b/core/modules/contact/lib/Drupal/contact/MessageFormController.php
@@ -25,6 +29,39 @@ class MessageFormController extends ContentEntityFormController {
+   * @param \Drupal\Core\Config\ConfigFactoryInterface $config_factory
+   *   The factory for configuration objects.

Do you mean this? I thought it was best practice to include the @param docblocks but we can remove if thats the common pattern. Its just my ignorance, really.

Edit: worked this out in IRC, he was referring to the protected var...

Adding the variable.

1. +++ b/core/modules/contact/lib/Drupal/contact/MessageFormController.php
   @@ -25,6 +29,47 @@ class MessageFormController extends ContentEntityFormController {
   +  public function __construct(ConfigFactoryInterface $config_factory, EntityManagerInterface $entity_manager, FloodInterface $flood) {
   

   $entity_manager also needs documenting (you need to document the parameter, but you don't need to add or set the new member variable, becuase that's done by the parent class).

2. +++ b/core/modules/system/lib/Drupal/system/Tests/System/FloodTest.php
   @@ -48,7 +48,7 @@ function testCleanUp() {
   -    $flood = \Drupal::service('flood');
   +    $flood = \Drupal::flood();
   

   I expect this can be replaced with $this->containter->get('flood')

Re 16.1 - I think this patch should do it for the parameter documentation...
Re 16.2 - You were right!

#15 is a known random failure (i.e. not an issue with the patch). Hopefully #17 will be green.

In tests we though use \Drupal::flood() directly, for various reasons.

Please can you explain or link to previous discussion? I thought we tried to use DI whereever possible.

Note: This is a test class.

See #2087751: [policy, no patch] Stop using $this->container in web tests

Should we change it back to \Drupal::flood() in /core/modules/system/lib/Drupal/system/Tests/System/FloodTest.php ?

Yeah. It's not a biggie either way, but we probably shouldn't change that line until that debate is settled, so let's go back to what you had before.

#26 addresses all the comments from previous reviews. Patch applies, verified the flood mechanism manually on local install, this is good to go.

Nice!

Committed 8d98947 and pushed to 8.x. Thanks!