Argument validations doesn't set the block title anymore

A temporary workaround in the theme layer for views blocks:

function theme_preprocess_block(&$elements) {
  // Check to see if this is a view block.
  if(!empty($elements['content']['#view']) 
    && $elements['content']['#view'] instanceof \Drupal\views\ViewExecutable
  ) {
    $view = $elements['content']['#view'];
    $view->execute();
    // Make sure the label is supposed to be displayed.
    if ($elements['configuration']['label_display']
      && !empty($view->build_info['title'])
      && !empty($view->build_info['substitutions'])
    ) {
      // Set the label.
      $elements['label'] = t($view->build_info['title'], $view->build_info['substitutions']);
  }
}

Updating the IS as it took me a bit to figure out this was about setting block titles. I tested and it works for page titles.

@alexpott, @dawehner, @timplunkett, and I discussed this issue and agreed it does not need to be major priority. While this does indeed sound like a bug, its impact is limited to a specific usecase. (Also, it's technically not a regression for core.)

We did agree that this should be treated as a bug. Steps to reproduce:

1. Add a taxonomy term (tid 1).
2. Go to /admin/structure/views/view/taxonomy_term
3. Add a block display (which will inherit the configuration for the page display).
4. Click on the link to the contextual filter and confirm that it is configured to override the title.
   
5. Change the default behavior for the block's contextual filter to provide a default value for the term (in order to test the argument being passed to the block easily).
   
6. Go to /admin/structure/block and place the block.
7. View the placed block on any page. it might seem to work (the term name appears at the top of the block) but this is actually part of the view header, not the block title.
8. Go back to configure the view at /admin/structure/views/view/taxonomy_term and add a title for the view overall (which should be overridden).
9. Visit /taxonomy/term/1 and see that the page title overrides the overall title, but the block does not.
   

I also checked whether there was test coverage for the page title case. It does not look like Views itself has test coverage for this functionality for arguments:

[mandelbrot:Tests | Thu 18:55:54] $ grep -r "assertTitle" *
Handler/AreaTitleWebTest.php:    $this->assertTitle('test_title_header | Drupal');
Handler/AreaTitleWebTest.php:    $this->assertTitle('test_title_empty | Drupal');
Handler/AreaTitleWebTest.php:    $this->assertTitle('test_title_header | Drupal');
Plugin/DisplayPageWebTest.php:    $this->assertTitle(t('Test default page | Drupal'));
Plugin/DisplayPageWebTest.php:    $this->assertTitle(t('Test local page | Drupal'));

So, adding tests for the bug would be a good next step.

The main usecase in core is the taxonomy term view, which would merit its own test coverage. In TermTest, there is some related coverage, but it is not sufficient:

    // View the term and check that it is correct.                              
    $this->drupalGet('taxonomy/term/' . $term->id());
    $this->assertText($edit['name[0][value]'], 'The randomly generated term name is present.');
    $this->assertText($edit['description[0][value]'], 'The randomly generated term description is present.');

TaxonomyTermViewTest has similar assertions:

    $this->drupalGet('taxonomy/term/' . $term->id());
    $this->assertText($term->label());
    $this->assertText($node->label());

Since the term page has the rendered term in the header as well, that means we could get a false positive even if the page title were not overridden. So we should add test coverage for that. Test coverage for a specific view is out of scope in this issue, so it should be done in a separate issue. (The followup would check assertTitle() and also assertNoText() for the overridden one.) Tagging for a followup for that.

I wonder if this bug has a similar cause as #2629566: Label is not shown for blocks with exposed forms.

Uploading patch that worked for me!

Marking it to needs review to trigger testbot

patch #9 by evanjenkins is working and seems very reasonable to me.

Filling view_build array (which build the view with filters etc) are done in preRenderViewElement (line 42 currently) where the "executedisplay" happens. This is where the view title is filled, so the block title fill should happen after this. That's why I think this addresses the issue very well.

The alternative dawehner said (execute & build more things to buildRenderable) would require to process the view display earlier (running view->executeDisplay in buildrenderable instead ?).

Also, this bug is from 8.0 so I think we need to backport it as well.

I'm marking it to RTBC (although I do not know if tests are needed for this issue or are needed generally in this area)

Also adding the Dublin2016 tag as I'm in the sprints :P

Note:

We have to be careful though with the related issue #2629566: Label is not shown for blocks with exposed forms (mentioned by xjm) as it also contains a change in the same line (a wrapper function for these lines).

I do not know if tests are needed for this issue

Yeah, they are, so leaving at needs work for adding test coverage.

I can confirm that the patch in #9 works as expected (while debugging, I came up with the exact same solution).

What else is needed to get this in? Can someone write a test to make sure that this won't get lost again?

@dawehner Yeah I think I can work on that :)

Test only patch is the interdiff.

Nice, thanks for the tests. I came up with the same solution and tested the patch manually as well. Setting to RTBC.

I'm highly confused that the test don't have arguments

Well the fix has nothing to do with arguments either. I just looked at the fix and wrote a test for that, I didn't look to closely at the rest of the discussion here, my bad! But it seems that the bug here is more general then just argument validation not being able to set the block title.

Nothing that influences the view title in preRenderViewElement is transferred to the block title. In this case, at the least tokens and overrides.

But yeah, lets see if we can add some coverage for overrides too.

Here we go, setting an argument with a fixed value and a title override.

Committed/pushed to 8.3.x and cherry-picked to 8.2.x. Thanks!

This introduced a test failure in HEAD on PHP7/Postgres:
https://www.drupal.org/pift-ci-job/527093

I've reverted that -- let's run tests on all environments for this one before commit once we've sorted the postgres fail. Thanks!

Wonky sort order on Postgres. Setting a fixed sort order makes the test pass locally on Postgres.

That looks great, back to RTBC.

Looks good now. I also re-tested with my STR from #5 and confirmed it did at least resolve the issue. Committed 469d128 and pushed to 8.3.x and 8.2.x. Thanks!