Wherever simple config is used to render output to end user, associate their cache tags

Worked on this together with pjonckiere at the sprint in Ghent. Changed cache tags in:
- core/modules/aggregator/src/Controller/AggregatorController.php
- core/modules/contact/src/Controller/ContactController.php
- core/modules/forum/src/Controller/ForumController.php
- core/modules/search/src/Controller/SearchController.php
- core/modules/user/src/Form/UserCancelForm.php
- core/modules/user/src/Form/UserLoginForm.php

Fixed unneeded changed line, test now passes for me locally.

@borisson_: Please provide interdiffs when you make changes to patches, so that it is easier to review. See https://www.drupal.org/documentation/git/interdiff

Fixed remarks in #11, added interdiff this time, so Berdir is happy.

Added cache tags for the following files as well, couldn't find other forms where this is needed.
- core/modules/block/src/BlockForm.php
- core/modules/book/src/BookManager.php
- core/modules/book/src/Form/BookSettingsForm.php
- core/modules/comment/src/CommentForm.php
- core/modules/contact/src/ContactFormEditForm.php
- core/modules/language/src/Form/NegotiationSelectedForm.php
- core/modules/language/src/Form/NegotiationUrlForm.php
- core/modules/locale/src/Form/LocaleSettingsForm.php
- core/modules/taxonomy/src/TermForm.php

Added password reset form. Will have alook at why the tests are failing in the meantime

Is it a good idea to add a mergeConfigCacheTags($form, $config_tags) in formBase?

Found the problem that made alle tests fail.

Thanks for all your work on this.

A lot thinking out lout below, wondering which cache tags we really need.

Did not read everything, my sprint day is over now :)

1. +++ b/core/modules/aggregator/src/Controller/AggregatorController.php
   @@ -173,6 +173,10 @@ public function adminOverview() {
        $build = $this->buildPageList($items);
   +
   +    $config = $this->config('system.site');
   +    $build['#cache']['tags'] = $config->getCacheTags();
   +
        $build['#attached']['feed'][] = array('aggregator/rss', $this->config('system.site')->get('name') . ' ' . $this->t('aggregator'));
   

   I'm not sure if we should do it here, but considering that we're now doing this, should we consider to go one step further and actually cache this, with a list cache tag for aggregator items? Possibly in a separate issue.

   The cache tag makes sense on a theoretical level, but if you think about it practically, this is the site name we are talking about. If we change the site name, then we have to invalidate every page anyway, because which page doesn't contain the site name? :)

   (Also wondering why this is not a view already :))

2. +++ b/core/modules/block/src/BlockForm.php
   @@ -108,10 +109,11 @@ public static function create(ContainerInterface $container) {
       */
      public function form(array $form, FormStateInterface $form_state) {
        $entity = $this->entity;
   +    $config = $this->config('system.theme');
   

   I'm not sure I see why we need this in entity forms like this one?

   I can not think of a scenario where we would be able to cache this form, why bother with cache tags?

   Adding cache tags/code means we need test coverage usually, so we need a test scenario that makes sense (That's my answer to "Adding it doesn't hurt anything", it delays getting this issue in with the parts that we do need :)

3. +++ b/core/modules/book/src/BookManager.php
   @@ -317,7 +317,8 @@ public function getBookParents(array $item, array $parent = array()) {
      protected function addParentSelectFormElements(array $book_link) {
   -    if ($this->configFactory->get('book.settings')->get('override_parent_selector')) {
   +    $config = $this->configFactory->get('book.settings');
   +    if ($config->get('override_parent_selector')) {
          return array();
   

   This is also stuff that is only used in node/backend forms, but it being a service could theoretically mean that it will be used publicly ;)

4. +++ b/core/modules/book/src/Form/BookSettingsForm.php
   @@ -51,6 +52,7 @@ public function buildForm(array $form, FormStateInterface $form_state) {
        );
        $form['array_filter'] = array('#type' => 'value', '#value' => TRUE);
   +    $form['#cache']['tags'] = Cache::mergeTags(isset($form['#cache']['tags']) ? $form['#cache']['tags'] : [],  $config->getCacheTags());
   

   Uhm. This is a ConfigForm, and $config is the config we are editing. If this would make sense, then we would have to add this by default to all config edit forms (Hint: I do not think this makes sense :))

5. +++ b/core/modules/comment/src/CommentForm.php
   @@ -207,6 +209,8 @@ public function form(array $form, FormStateInterface $form_state) {
        );
    
   +    $form['#cache']['tags'] = Cache::mergeTags(isset($form['#cache']['tags']) ? $form['#cache']['tags'] : [],  $config->getCacheTags());
   +
   

   CommentForm is something we might display to anonymous users, so this makes sense...

6. +++ b/core/modules/comment/src/CommentViewBuilder.php
   @@ -154,7 +155,9 @@ public function buildComponents(array &$build, array $entities, array $displays,
          $account = comment_prepare_author($entity);
   -      if (\Drupal::config('user.settings')->get('signatures') && $account->getSignature()) {
   +      $config = \Drupal::config('user.settings');
   +      $build['#cache']['tags'] = Cache::mergeTags(isset($build['#cache']['tags']) ? $build['#cache']['tags'] : [],  $config->getCacheTags());
   +      if ($config->get('signatures') && $account->getSignature()) {
   

   This one makes sense, wondering if the test below is enough test coverage for this, possibly?

7. +++ b/core/modules/contact/src/ContactFormEditForm.php
   @@ -34,7 +35,8 @@ public function form(array $form, FormStateInterface $form_state) {
    
        $contact_form = $this->entity;
   -    $default_form = $this->config('contact.settings')->get('default_form');
   +    $config = $this->config('contact.settings');
   +    $default_form = $config->get('default_form');
    
   

   Same here, can't see how we would ever cache this.

Removed Cache tags for BlockForm.php, BookSettingsForm.php & ContactFormEditForm.php. I'll see if I can change the test for CommentViewBuilder.

I'm not sure about AggregatorController, that is actually frontend, it's just the fact that it is the site name that made me think about it. (which is displayed everywhere, so everything would need to be invalidated I guess, do we need to worry about that?)

CommentDefaultFormatterCacheTagsTest is actually correct I think, that's from the comment view builder, which is frontend and cached, no?

Also note that I didn't get past "c" (in the module/file list of the patch) in my review above, so reviewed the changes and rest of the file as well.

1. +++ b/core/modules/block/src/BlockForm.php
   @@ -10,6 +10,7 @@
    use Drupal\block\Event\BlockEvents;
    use Drupal\Component\Utility\Html;
   +use Drupal\Core\Cache\Cache;
    use Drupal\Core\Entity\EntityForm;
    use Drupal\Core\Entity\EntityManagerInterface;
    use Drupal\Core\Executable\ExecutableManagerInterface;
   @@ -108,10 +109,11 @@ public static function create(ContainerInterface $container) {
   
   @@ -108,10 +109,11 @@ public static function create(ContainerInterface $container) {
       */
      public function form(array $form, FormStateInterface $form_state) {
        $entity = $this->entity;
   +    $config = $this->config('system.theme');
    
        // Store theme settings in $form_state for use below.
        if (!$theme = $entity->getTheme()) {
   -      $theme = $this->config('system.theme')->get('default');
   +      $theme = $config->get('default');
        }
   

   Those changes are no longer necessary.

2. +++ b/core/modules/book/src/Form/BookSettingsForm.php
   @@ -7,6 +7,7 @@
   
   @@ -7,6 +7,7 @@
    
    namespace Drupal\book\Form;
    
   +use Drupal\Core\Cache\Cache;
    use Drupal\Core\Form\ConfigFormBase;
    use Drupal\Core\Form\FormStateInterface;
   

   Same.

3. +++ b/core/modules/contact/src/ContactFormEditForm.php
   @@ -7,6 +7,7 @@
   +use Drupal\Core\Cache\Cache;
    use Drupal\Core\Config\ConfigFactoryInterface;
   
   @@ -34,7 +35,8 @@ public function form(array $form, FormStateInterface $form_state) {
        $contact_form = $this->entity;
   -    $default_form = $this->config('contact.settings')->get('default_form');
   +    $config = $this->config('contact.settings');
   +    $default_form = $config->get('default_form');
    
   

   Same :)

4. +++ b/core/modules/language/src/Form/NegotiationSelectedForm.php
   +++ b/core/modules/language/src/Form/NegotiationUrlForm.php
   

   Those are also backend forms.

5. +++ b/core/modules/locale/src/Form/LocaleSettingsForm.php
   @@ -85,6 +86,8 @@ public function buildForm(array $form, FormStateInterface $form_state) {
        );
    
   +    $form['#cache']['tags'] = Cache::mergeTags(isset($form['#cache']['tags']) ? $form['#cache']['tags'] : [],  $config->getCacheTags());
   

   This too.

6. +++ b/core/modules/search/src/Controller/SearchController.php
   @@ -89,7 +90,9 @@ public function view(Request $request, SearchPageInterface $entity) {
            // Log the search.
   -        if ($this->config('search.settings')->get('logging')) {
   +        $config = $this->config('search.settings');
   +        $build['#cache']['tags'] = Cache::mergeTags(isset($build['#cache']['tags']) ? $build['#cache']['tags'] : [],  $config->getCacheTags());
   +        if ($config->get('logging')) {
              $this->logger->notice('Searched %type for %keys.', array('%keys' => $keys, '%type' => $entity->label()));
            }
   

   Note that the configuration here does not affect the output, only if we log or not, so not relevant for the render cache.

7. +++ b/core/modules/taxonomy/src/TermForm.php
   @@ -88,6 +90,8 @@ public function form(array $form, FormStateInterface $form_state) {
          '#value' => $term->id(),
        );
    
   +    $form['#cache']['tags'] = Cache::mergeTags(isset($form['#cache']['tags']) ? $form['#cache']['tags'] : [],  $config->getCacheTags());
   +
   

   Same for the term form, then?

8. +++ b/core/modules/user/src/AccountForm.php
   @@ -74,6 +74,7 @@ public function form(array $form, FormStateInterface $form_state) {
        $user = $this->currentUser();
        $config = \Drupal::config('user.settings');
   +    $form['#cache']['tags'] = $config->getCacheTags();
   

   This is also an entity form.

9. +++ b/core/modules/user/src/Form/UserCancelForm.php
   @@ -76,6 +77,7 @@ public function getConfirmText() {
        $user = $this->currentUser();
        $this->cancelMethods = user_cancel_methods();
   +    $config = $this->config('user.settings');
   

   This too

=> There won't be much left, which is fine :)

I kept the changes in CommentDefaultFormatterCacheTagsTest, removed all other changes.

Needs work for tests then.

It still feels wrong to have a getCacheTags method on ConfigBase, but well, we decided to go the couple everything route.

1. +++ b/core/modules/user/src/Form/UserPasswordResetForm.php
   @@ -97,7 +97,7 @@ public function buildForm(array $form, FormStateInterface $form_state, AccountIn
   -   * {@inheritdoc}
   +   * {@inheritdoc}o
   

   ooh ooh ooh

2. +++ b/core/modules/user/src/Tests/UserLoginTest.php
   @@ -16,6 +16,16 @@
   +    $this->drupalGet('user/login');
   +    $cache_tags = explode(' ', $this->drupalGetHeader('X-Drupal-Cache-Tags'));
   +    $this->assertTrue(in_array('config:system.site', $cache_tags));
   
   +++ b/core/modules/user/src/Tests/UserRegistrationTest.php
   @@ -24,6 +24,15 @@ class UserRegistrationTest extends WebTestBase {
   +  function testCacheTags() {
   +    $this->drupalGet('user/register');
   +    $cache_tags = explode(' ', $this->drupalGetHeader('X-Drupal-Cache-Tags'));
   +    $this->assertTrue(in_array('config:user.settings', $cache_tags));
   

   It would be nice to have a one line comment why we expect it to be there. Its not obvious, really.

Thank you for addressing my feedback.

Shouldn't we be adding the system.site cache tag to SystemBrandingBlock::getCacheTags()?

+++ b/core/modules/user/src/Form/UserPasswordResetForm.php
@@ -97,7 +97,7 @@ public function buildForm(array $form, FormStateInterface $form_state, AccountIn
-   * {@inheritdoc}
+   * {@inheritdoc}o

It looks like the patch in #37 does not contain the changes that the interdiff says it does.

Last few interdiffs look good, verified that those changes are actually in the patch ;)

Back to RTBC.

RTBC + 1

1. +++ b/core/modules/book/src/BookManager.php
   @@ -352,6 +353,7 @@ protected function addParentSelectFormElements(array $book_link) {
   +    $form['#cache']['tags'] = Cache::mergeTags(isset($form['#cache']['tags']) ? $form['#cache']['tags'] : [],  $config->getCacheTags());
   

   This mergeTags() with ternary logic as an argument is used a lot and it's quite cumbersome to write, how about adding a Cache::mergeRenderArrayTags() (or similar) that receives a render array and does all the isset logic for you?

   It could even receive the render array by reference to make it even shorter to write.

2. +++ b/core/modules/book/src/Tests/BookTest.php
   @@ -77,6 +77,17 @@ protected function setUp() {
   +  function testCacheTags() {
   

   A lot of these new test methods are added in web tests so they're doing a full Drupal install just for a single assertion. That's.. not very nice of them :)

3. +++ b/core/modules/book/src/Tests/BookTest.php
   @@ -77,6 +77,17 @@ protected function setUp() {
   +    $this->drupalGet('node/add/book');
   +    $cache_tags = explode(' ', $this->drupalGetHeader('X-Drupal-Cache-Tags'));
   +    $this->assertTrue(in_array('config:book.settings', $cache_tags));
   

   This logic is also repeated a lot, how about introducing a method on the base test classes for it? It could receive the cache tag as the first argument and an optional path or Url object for the drupalGet() part.

re #46.1 I agree a method here would be helpful - I don't think the render array should be passed in by reference though.
re #46.2 but are there obvious places to add this?
re #46.3 yep maybe an assertCacheTag method on WebTestBase

I agree with #49, but I am 100% for option B).

The reason being, that those are internal properties that no user of render arrays or CacheableInterface should know about.

About #46.2, I don't know... any existing method in those test classes? :P

And #49 option B looks great.

Agree, but we should probably then not introduce a new option a) in between, lets create the follow-up and back to RTBC for the original patch?

Here's an interdiff for #46.2, feel free to apply or discard it if you don't like it :)

Otherwise, RTBC++.

I like #56 - let's do it. amateescu++

Committed/pushed to 8.0.x, thanks!