Label (Title) not set for Views block (exposed filters in Block)

I can confirm this issue does exist. I am going to try an fix.

Ok so I found the issue. The title is not being set lol... Here is a patch.

This issue applies to 8.x-2.x as well. The attached patch fixes it.

**Update** apologize didn't realize you can run patch against multiple versions. #3 and #4 should be the exact same.

This totally makes sense in general! Some kind of tests for that would be nice though.

Test should fail with the test patch.
Tests should pass with patch with tests.

If I did this wrong let me know.

Patch in #7 passed after migration build issues were fixed today.

Thanks! It is works.

Thanks! it works.

+++ b/core/modules/views/src/Plugin/Block/ViewsExposedFilterBlock.php
@@ -32,6 +33,14 @@ public function build() {
+        '#allowed_tags' => Xss::getHtmlTagList()

Given this is an important security consideration I think this should have test coverage - both that certain html tags like strong are permitted but also that others like script are stripped.

This needs to be committed ASAP please. It works correctly and without it there is a lot of confusion.

Composer seems unable to apply this patch for 8.3. Any ideas?

Re-rolled for 8.4.x
This is my first contribution ever, so I hope I have proceeded correctly and welcome any pointers.

It seems unlikely but apparently the patch has stopped working altogether in 8.3. I re-rolled it for 8.4 and composer patches it correctly, but my exposed filter title, which was displaying in 8.2 using the patch from comment #7, is nowhere to be found.

There are two patches in #7 - one is a bugfix for the issue and one is adding a test. The bugfix applies cleanly to 8.3.*, the test does not.

Patch in #24 no longer applies. Re-rolled.

It looks like the previews reroll on #24 was missing the changes to core/modules/views/src/Plugin/Block/ViewsExposedFilterBlock.php - adding that back now on top of #28.

@alexpott Re #20, Xss::getHtmlTagList() will just allow ['a', 'em', 'strong', 'cite', 'blockquote', 'code', 'ul', 'ol', 'li', 'dl', 'dt', 'dd'], and using that with #allowed_tags means that this will be handled by Renderer::ensureMarkupIsSafe(), which is already tested on RendererTest::providerTestRenderBasic() so don't think we should be explicitly testing the usage of #allowed_tags here.

I agree with #31, extra testcoverage for the xss coverage seems to be overkill. While this might seem like trusting the implicit testcoverage, I feel that it's not the responsability of everything that uses '#allowed_tags' to also write explicit testcoverage.

This has specific testcoverage and does work, so marking as RTBC.

+++ b/core/modules/views/src/Plugin/Block/ViewsExposedFilterBlock.php
@@ -46,6 +47,14 @@ public function build() {
+    // Set the blocks title.

nit either block or block's - we are talking about one block here - can be fixed on commit

I noticed this as well and was about to create a new issue for it but luckily found this.

What I was wondering is the impact this will have on existing sites. It is a bugfix but it has a visual effect on existing sites that are using such a block with the default configuration which is configured to show titles.

It's quite likely that sites either didn't want a title and are happy with the current output or they did some sort of workaround like a custom block template to get one. And if they update, they might end up with an unexpected new title or even a duplicate title now, depending on what they did.

Considering that, we might want to at least do a change record to notify people about this change/fix and if we want to be extra strict, we could consider to disable the show title checkbox on existing blocks to basically keep the same behavior as we have now?

For #35

I agree, we should default to no title for existing blocks with instructions in change record

Agree on @Berdir and @larowlan. And on @alexpott: what about the security concerns he stated @ #20 ? I didn't saw any further mentioning.

@Manuel Garcia's patch in #30 no longer applied to 8.5.x. A reroll with no further modifications is attached.

+++ b/core/modules/views/tests/src/Functional/Plugin/ExposedFormTest.php
@@ -199,7 +199,30 @@ public function testExposedBlock() {
+    $this->assertText($view->getTitle(), 'Block title found.');
...
+    $this->assertText('Custom title', 'Custom block title found.');

Should we also test that the original views title is not displayed? If you think about it, it could easily still be the case that the views title is rendered somehow.

@dawehner: true.

Addressing #39

Re #20 given that we expect to be able to handle markup in the label I think we should test this. It's not testing #markup and its abilities, it is asserting on our expectations.

+++ b/core/modules/views/tests/src/Functional/Plugin/ExposedFormTest.php
@@ -199,7 +199,31 @@ public function testExposedBlock() {
+    $block->getPlugin()->setConfigurationValue('views_label', 'Custom title');

Let's out some html in here that will be filtered and some that won't.

Also #35 still needs to be addressed.

OK I think I know what you mean @alexpott, thanks for the kind explanation. Attempting to address #20/#42 here :)

Stil to do #35.

@Manuel Garcia yep that's exactly what I meant. Thanks.

So by the sounds of it, #35 and #36, it seems that we need an upgrade path to set the label_display configuration for all existing exposed filter blocks to FALSE, right? So that means we'd also need upgrade path tests...

I'm not sure what's best to be honest, if we go the upgrade path way, people will need to not revert their configuration before updating their exports, or they could see the new titles showing up.

So although admitedly less so, that's stil risky, its just one less step if I'm not mistaken, assuming all they want to do is disable the new funcionality.

I suppose what I am trying to say is that website devs will need to take action in any case, even if its just to make use (or not) of this functionality and/or remove their template workarounds.

Added a CR for this, tried to be as clear as possible, but it could definetly use a review https://www.drupal.org/node/2925510

> I'm not sure what's best to be honest, if we go the upgrade path way, people will need to not revert their configuration before updating their exports, or they could see the new titles showing up.

That's always the case for all updates. updates can change configuration. In most cases that is reproducible, you run updates locally/on test, then export config, then deploy and updates then make the change and then you don't have to import it (exceptions are updates that generate config/random-ish values, those are a problem)

The bigger problem is that block config entities isn't the only thing that use block config entities. Writing an update function for that isn't going to help those that placed this block with page_manager/panels.

Right @Berdir, makes sense.

The bigger problem is that block config entities isn't the only thing that use block config entities. Writing an update function for that isn't going to help those that placed this block with page_manager/panels.

So then, how to procede from here? Is the CR enough or do we need an upgrade path?
If we need an upgrade path, what should it look like?

Thank you, actually title code was missing.
After this patch now title is appearing on my Views Exposed Form Block.

Thanks.

Patch at #43 works perfectly. Exactly what I needed

Thanks

F

OK, If I understood the situation correctly, I believe that the upgrade path should update existing views, setting label_display to FALSE. This is to keep these blocks working as before. See #35 and #36.

Once that is done, we will need to update the CR to reflect this change:

The update path updated existing views disabling the label_display setting, so as preserve expected previous functionality. If you'd like to make use of this setting now, please make sure you were not already manually printing this on your templates so as to avoid displaying the block titles twice.

We should also include in the CR a mention of other modules that may be inserting this blocks on the site, like page_manager/panels, because the update path cannot cover for this, something like:

If your site is using other means of inserting these blocks into the page, like page_manager/panels, make sure to revisit this setting so as to prevent presenting the titles twice.

Here is a start on the upgrade path.

Tested and works as expected.

Tested and works as expected here.

Thanks @Cauliflower & @fonant for reporting on this.

Still to do here as far as I can see:

• Test for the upgrade path
• Update the CR (see #52)

Tested the upgrade path:

1. Applied the patch
2. rebuilt cache
3. looked for title, shows up
4. run database update and is hidden
5. Enable it again (yay)

Can any one please how can i apply patches using composer

+++ b/core/modules/views/views.install
@@ -508,3 +508,26 @@ function views_update_8500() {
+/**
+ * Update exposed filter blocks label display to be disabled.
+ */
+function views_update_8600() {
+  /** @var \Drupal\block\BlockInterface $block_storage */
+  $block_storage = \Drupal::entityManager()->getStorage('block');
+  $config_factory = \Drupal::configFactory();
+
+  foreach ($config_factory->listAll('views.view.') as $view_config_name) {
+    $view = $config_factory->get($view_config_name);
+    foreach ($view->get('display') as $display_id => $display) {
+      if (isset($display['display_options']['exposed_block']) && $display['display_options']['exposed_block'] == 1) {
+        $plugin_id = 'views_exposed_filter_block:' . $view->get('id') . '-' . $display_id;
+        $blocks = $block_storage->loadByProperties(['plugin' => $plugin_id]);
+        foreach ($blocks as $block) {
+          $block->getPlugin()->setConfigurationValue('label_display', FALSE);
+          $block->save();
+        }
+      }
+    }
+  }
+}

This should be a post update function as we're using the full entity API to save the blocks and probably should be batched in some way since some sites can have a lot of blocks.

Also we need an update test.

Thanks for the reviews!

I think this is the tag you meant @alexpott :)

Updating the Remaining tasks section of the issue summary.

FYI for updating config entities we have \Drupal\Core\Config\Entity\ConfigEntityUpdater() that helps with the batching.

Thanks @alexpott it sure does help :)
Addressing #61 here, we still need an upgrade path test.

+++ b/core/modules/views/views.post_update.php
@@ -366,3 +366,18 @@ function views_post_update_table_display_cache_max_age(&$sandbox = NULL) {
\ No newline at end of file

Oops /shrug

Here is an upgrade path test

Just tested patch #53 on Drupal 8.6.1 and works as expected.

Thanks a mil

F

Tested this out:

1. Currently no block titles are shown for exposed filter blocks.
2. After the patch applied,
3. The update hook updates preserves that state and ensures the titles continue to be as they where which is great so we don't have complaints of exposed titles show up after updates, and it preserves my overwritten title even thought it's now displayed unchecked now the updb ran.
4. I check to show the title again and now it shows!

Configuration:

Before:

After:

Also did a scan through the code. A couple of minor nitpiks:

1. +++ b/core/modules/views/tests/src/Functional/Plugin/ExposedFormTest.php
   @@ -199,7 +199,31 @@ public function testExposedBlock() {
   +    $block->getPlugin()->setConfigurationValue('label_display', FALSE);
   
   +++ b/core/modules/views/views.post_update.php
   @@ -366,3 +366,18 @@ function views_post_update_table_display_cache_max_age(&$sandbox = NULL) {
   +      $block->getPlugin()->setConfigurationValue('label_display', '0');
   

   Should it be '0' or FALSE?

2. +++ b/core/modules/views/tests/src/Functional/Update/ExposedFilterBlocksUpdateTest.php
   --- a/core/modules/views/views.post_update.php
   +++ b/core/modules/views/views.post_update.php
   
   +++ b/core/modules/views/views.post_update.php
   @@ -366,3 +366,18 @@ function views_post_update_table_display_cache_max_age(&$sandbox = NULL) {
   \ No newline at end of file
   

   Needs another linebreak

3. +++ b/core/modules/views/views.post_update.php
   @@ -366,3 +366,18 @@ function views_post_update_table_display_cache_max_age(&$sandbox = NULL) {
   +    if (substr($block->getPluginId(), 0, strlen('views_exposed_filter_block:')) === 'views_exposed_filter_block:') {
   

   Can be just strpos() === 0?

Thanks @joelpittet !

Re #71.1 Unfortunately that is defined in the schema as a string, so its either that or it will fail (see core/config/schema/core.data_types.schema.yml line 309).

Fixed the #71.2 and #71.3 nitpicks here.

Thanks - this has resolved my missing title on an exposed views filter block! I thought I was going mad ;)

Applied cleanly to 8.6.1

Patch at #72 applied perfectly to Drupal 8.6.2

Please commit :o)

F

Since 8.1 and for some years this error is present in every version. And after finding and applying a (version specific) patch the title is (was) displayed.

I don't understand why this bug can't be fixed in all versions and for the future. Why is there a need to detect, report, check, fix (or search for a patch) and finally patch everywhere the same bug from version to version?

@ksi issues have to get to RTBC, reviewed by a core committer, then committed in order to make it into a release. There are hundreds of issues in the queue, and while some are fixed within days, some take several years to get resolved - this could be down to any number of factors depending on the issue. This one's done though now and will be in 8.7.0 when it comes out. I didn't commit to 8.6.x due to the configuration update.

Committed ff031bb and pushed to 8.7.x. Thanks!

Thanks for committing catch!

I have updated the CR with regards to #52, fixed the branch & version for it and published it.

It seems labels in views blocks are still missing from 8.8.4 …

Closed #2629566: Label is not shown for blocks with exposed forms as a duplicate. Adding credit for the work done over there.