Contact forms don't have necessary cache contexts & tags; flood control should work on validate, not on view

"Needs work" per last sentence in #7.

Looks pretty reasonable.

1. +++ b/core/lib/Drupal/Core/Access/AccessResult.php
   @@ -339,7 +339,14 @@ public function setCacheMaxAge($max_age) {
   +      ->addCacheTags(Cache::buildTags('config:user.role', \Drupal::currentUser()->getRoles(), '.'));
   

   What happens if the AccessResult doesn't relate to the current logged in user? Isn't there places where we can formulate an AccessResult based on another user? e.g. anywhere we do an access check we can pass an optional account, with default falling back to the logged in user. If so, that would mean that a) this isn't correct and b) the user should be passed along to the access result and we don't need the call out to the container? Happy to be wrong on this.

2. +++ b/core/lib/Drupal/Core/Routing/AccessAwareRouterInterface.php
   @@ -16,6 +16,11 @@
   +   * Attribute name of the access result for the request..
   

   nit: two .

3. +++ b/core/tests/Drupal/Tests/Core/Entity/EntityAccessCheckTest.php
   @@ -22,6 +23,27 @@
   +  protected function setUp() {
   
   +++ b/core/tests/Drupal/Tests/Core/Route/RoleAccessCheckTest.php
   @@ -22,6 +23,27 @@
   +  protected function setUp() {
   

   Could we move the duplicated code to a trait and then we only need to add a use statement, with the only change to the ::setUp being a call to the trait method? Saves the duplication.

+++ b/core/modules/contact/src/Controller/ContactController.php
@@ -74,10 +33,6 @@ public static function create(ContainerInterface $container) {
   public function contactSitePage(ContactFormInterface $contact_form = NULL) {
-    // Check if flood control has been activated for sending emails.
-    if (!$this->currentUser()->hasPermission('administer contact forms')) {
-      $this->contactFloodControl();

@@ -141,24 +92,8 @@ public function contactPersonalPage(UserInterface $user) {
-  protected function contactFloodControl() {
...
-    if (!$this->flood->isAllowed('contact', $limit, $interval)) {
...
-      throw new AccessDeniedHttpException();

So maybe better to move that check to access handler, we should not show the form and try to serve POST requests for "flood"

I agree with #28, the validation message comes when trying to post more not when going to the site.

The code changes for the contact flood changes look good to me.

+++ b/core/lib/Drupal/Core/EventSubscriber/FinishResponseSubscriber.php
@@ -167,7 +167,7 @@ public function onRespond(FilterResponseEvent $event) {
+    if (strlen($response->headers->get('X-Drupal-Cache-Tags')) > 0) {

nitpick: any reason why strlen and not != ''?
Apart from that (or even including that) rtbc

+++ b/core/lib/Drupal/Core/EventSubscriber/FinishResponseSubscriber.php
@@ -167,7 +167,7 @@ public function onRespond(FilterResponseEvent $event) {
     $cache_tags = $cacheable_access_result->getCacheTags();
-    if ($response->headers->has('X-Drupal-Cache-Tags')) {
+    if (strlen($response->headers->get('X-Drupal-Cache-Tags')) > 0) {

That, or just switch has() to get(). That's exactly the difference between the two, has is isset(), if (get()) is basically an !empty(), it will return an empty string, that will be cast to FALSE and it won't go inside the if.

Looks fine to me as well. For the record, I'm pretty sure it would have worked if you just changed to submit in the tests because the form post would run through the uncached controller and should have triggered validation there, but this is cleaner and more reliable if we ever switch to using a separate route or so for form submissions.

Needs a reroll..

+++ b/core/lib/Drupal/Core/EventSubscriber/FinishResponseSubscriber.php
@@ -167,7 +167,7 @@ public function onRespond(FilterResponseEvent $event) {
-    if ($response->headers->has('X-Drupal-Cache-Tags')) {
+    if ($response->headers->get('X-Drupal-Cache-Tags')) {

@@ -175,7 +175,7 @@ protected function updateDrupalCacheHeaders(Response $response, CacheableInterfa
-    if ($response->headers->has('X-Drupal-Cache-Contexts')) {
+    if ($response->headers->get('X-Drupal-Cache-Contexts')) {

I'm not particularly happy happy about the concept of http headers with no value. Something feels really off. Discussed with @Wim Leers in IRC. We decided that we'll open a followup to discuss how to prevent this.

RTBC + 1 to #38

I take that back, I don't think the rebase was wrong.

I think #38 misses parts of #35 now.

E.g. the flood and dateFormatter are with #35 removed from the ContactController, but in #38 those remain.

I am confused ...

RTBC, this now looks great!

I gave some thought to the change of making flood control on the validate since this is a change to how users experience the site - i.e.they'll only get informed on submit instead of just visiting the page. I think this an acceptable price for a cacheable site so +1.

This issue addresses a major bug and is allowed per https://www.drupal.org/core/beta-changes. Committed eebf493 and pushed to 8.0.x. Thanks!

Do we need a change record / doc update of some kind to inform other authors to ensure they use flood correctly?

Yes, I think so.

Right, to:
- inform site builders about the change to site behavior with respect to Contact module
- inform contrib modules that create their own flood-controlled forms about the new best practice
- inform modules that extend/alter Contact and/or other forms and might care about this (e.g., Mollom?)

CR looks great

Does this need to be ported to Drupal 7?