Problem/Motivation
When using a views exposed form as block. The block does not allow you to provide a title.
Proposed resolution
Patch provided
Remaining tasks
Add test coverage for the bugWrite the upgrade pathReviewed in #61, and re-factored in #66Manually test for the upgrade path#57Update the change record (see #52)Write upgrade path tests#68
User interface changes
None
API changes
None
Data model changes
None
Original Summary
I have created a simple Views page with several exposed filters with Views option: Exposed form in block: Yes.
The block is configured to "Display title" in block config settings.
When the block is configured to display in a region, the Label/title is not displaying.
I have dumped the block variables out with template_preprocess_block() and label key is empty ('label' => string(0) "").
The result is the same when the block is configured to override title.
Is this a bug or have I missed something?
Comment | File | Size | Author |
---|---|---|---|
#72 | 2720101-72.patch | 7.66 KB | Manuel Garcia |
#72 | interdiff-2720101-68-72.txt | 915 bytes | Manuel Garcia |
#71 | after-Fiery_chili_sauce___Umami_Food_Magazine.png | 40.82 KB | joelpittet |
#71 | Before-Fiery_chili_sauce___Umami_Food_Magazine.png | 138.08 KB | joelpittet |
#71 | Configure_block___Umami_Food_Magazine.png | 65.78 KB | joelpittet |
Comments
Comment #2
dobe CreditAttribution: dobe as a volunteer commentedI can confirm this issue does exist. I am going to try an fix.
Comment #3
dobe CreditAttribution: dobe as a volunteer commentedOk so I found the issue. The title is not being set lol... Here is a patch.
Comment #4
dobe CreditAttribution: dobe as a volunteer commentedThis 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.
Comment #5
dobe CreditAttribution: dobe as a volunteer commentedComment #6
dawehnerThis totally makes sense in general! Some kind of tests for that would be nice though.
Comment #7
dobe CreditAttribution: dobe as a volunteer commentedTest should fail with the test patch.
Tests should pass with patch with tests.
If I did this wrong let me know.
Comment #8
dobe CreditAttribution: dobe as a volunteer commentedComment #9
dobe CreditAttribution: dobe as a volunteer commentedComment #13
dobe CreditAttribution: dobe as a volunteer commentedComment #14
dobe CreditAttribution: dobe as a volunteer commentedPatch in #7 passed after migration build issues were fixed today.
Comment #15
dobe CreditAttribution: dobe as a volunteer commentedComment #17
Archual CreditAttribution: Archual commentedThanks! It is works.
Comment #18
davewilly CreditAttribution: davewilly as a volunteer commentedWorks for me also. Thanks.
Comment #19
BenR CreditAttribution: BenR at Studio Matris commentedThanks! it works.
Comment #20
alexpottGiven 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.
Comment #22
acThis needs to be committed ASAP please. It works correctly and without it there is a lot of confusion.
Comment #23
camilocodes CreditAttribution: camilocodes commentedComposer seems unable to apply this patch for 8.3. Any ideas?
Comment #24
camilocodes CreditAttribution: camilocodes commentedRe-rolled for 8.4.x
This is my first contribution ever, so I hope I have proceeded correctly and welcome any pointers.
Comment #26
camilocodes CreditAttribution: camilocodes commentedIt 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.
Comment #27
acThere 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.
Comment #28
jofitz CreditAttribution: jofitz at ComputerMinds commentedPatch in #24 no longer applies. Re-rolled.
Comment #30
Manuel Garcia CreditAttribution: Manuel Garcia as a volunteer and at Appnovation commentedIt 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.Comment #31
Manuel Garcia CreditAttribution: Manuel Garcia as a volunteer and at Appnovation commented@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 byRenderer::ensureMarkupIsSafe()
, which is already tested onRendererTest::providerTestRenderBasic()
so don't think we should be explicitly testing the usage of #allowed_tags here.Comment #33
borisson_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.
Comment #34
larowlannit either
block
orblock's
- we are talking about one block here - can be fixed on commitComment #35
BerdirI 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?
Comment #36
larowlanFor #35
I agree, we should default to no title for existing blocks with instructions in change record
Comment #37
jbitdrop CreditAttribution: jbitdrop commentedAgree on @Berdir and @larowlan. And on @alexpott: what about the security concerns he stated @ #20 ? I didn't saw any further mentioning.
Comment #38
JacobSanford@Manuel Garcia's patch in #30 no longer applied to 8.5.x. A reroll with no further modifications is attached.
Comment #39
dawehnerShould 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.
Comment #40
jbitdrop CreditAttribution: jbitdrop as a volunteer and at MAROQQO studios commented@dawehner: true.
Comment #41
Manuel Garcia CreditAttribution: Manuel Garcia as a volunteer and at Appnovation commentedAddressing #39
Comment #42
alexpottRe #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.
Let's out some html in here that will be filtered and some that won't.
Also #35 still needs to be addressed.
Comment #43
Manuel Garcia CreditAttribution: Manuel Garcia as a volunteer and at Appnovation commentedOK I think I know what you mean @alexpott, thanks for the kind explanation. Attempting to address #20/#42 here :)
Stil to do #35.
Comment #44
alexpott@Manuel Garcia yep that's exactly what I meant. Thanks.
Comment #45
Manuel Garcia CreditAttribution: Manuel Garcia as a volunteer and at Appnovation commentedSo 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.
Comment #46
Manuel Garcia CreditAttribution: Manuel Garcia as a volunteer and at Appnovation commentedAdded a CR for this, tried to be as clear as possible, but it could definetly use a review https://www.drupal.org/node/2925510
Comment #47
Berdir> 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.
Comment #48
Manuel Garcia CreditAttribution: Manuel Garcia as a volunteer and at Appnovation commentedRight @Berdir, makes sense.
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?
Comment #49
imanoop CreditAttribution: imanoop commentedThank you, actually title code was missing.
After this patch now title is appearing on my Views Exposed Form Block.
Thanks.
Comment #51
frankdesign CreditAttribution: frankdesign commentedPatch at #43 works perfectly. Exactly what I needed
Thanks
F
Comment #52
Manuel Garcia CreditAttribution: Manuel Garcia as a volunteer and at Appnovation commentedOK, If I understood the situation correctly, I believe that the upgrade path should update existing views, setting
label_display
toFALSE
. 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:
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:
Comment #53
Manuel Garcia CreditAttribution: Manuel Garcia as a volunteer and at Appnovation commentedHere is a start on the upgrade path.
Comment #54
Cauliflower CreditAttribution: Cauliflower commentedTested and works as expected.
Comment #55
fonant CreditAttribution: fonant commentedTested and works as expected here.
Comment #56
Manuel Garcia CreditAttribution: Manuel Garcia as a volunteer and at Appnovation commentedThanks @Cauliflower & @fonant for reporting on this.
Still to do here as far as I can see:
Comment #57
joelpittetTested the upgrade path:
Comment #58
joelpittetComment #59
raajkumar.kuruCan any one please how can i apply patches using composer
Comment #61
alexpottThis 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.
Comment #62
Manuel Garcia CreditAttribution: Manuel Garcia as a volunteer and at Appnovation commentedThanks for the reviews!
I think this is the tag you meant @alexpott :)
Comment #63
Manuel Garcia CreditAttribution: Manuel Garcia as a volunteer and at Appnovation for Pfizer, Inc. commentedUpdating the
Remaining tasks
section of the issue summary.Comment #64
Manuel Garcia CreditAttribution: Manuel Garcia as a volunteer and at Appnovation for Pfizer, Inc. commentedComment #65
alexpottFYI for updating config entities we have \Drupal\Core\Config\Entity\ConfigEntityUpdater() that helps with the batching.
Comment #66
Manuel Garcia CreditAttribution: Manuel Garcia as a volunteer commentedThanks @alexpott it sure does help :)
Addressing #61 here, we still need an upgrade path test.
Comment #67
Manuel Garcia CreditAttribution: Manuel Garcia as a volunteer commentedOops
/shrug
Comment #68
Manuel Garcia CreditAttribution: Manuel Garcia as a volunteer and at Appnovation for Pfizer, Inc. commentedHere is an upgrade path test
Comment #69
Manuel Garcia CreditAttribution: Manuel Garcia as a volunteer and at Appnovation for Pfizer, Inc. commentedComment #70
frankdesign CreditAttribution: frankdesign commentedJust tested patch #53 on Drupal 8.6.1 and works as expected.
Thanks a mil
F
Comment #71
joelpittetTested this out:
Configuration:
Before:
After:
Also did a scan through the code. A couple of minor nitpiks:
Should it be '0' or FALSE?
Needs another linebreak
Can be just strpos() === 0?
Comment #72
Manuel Garcia CreditAttribution: Manuel Garcia as a volunteer and at Appnovation for Pfizer, Inc. commentedThanks @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.
Comment #73
Manuel Garcia CreditAttribution: Manuel Garcia as a volunteer and at Appnovation for Pfizer, Inc. commentedComment #74
nicholasThompsonThanks - this has resolved my missing title on an exposed views filter block! I thought I was going mad ;)
Applied cleanly to 8.6.1
Comment #75
frankdesign CreditAttribution: frankdesign commentedPatch at #72 applied perfectly to Drupal 8.6.2
Please commit :o)
F
Comment #76
ksi CreditAttribution: ksi commentedSince 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?
Comment #78
catch@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!
Comment #79
Manuel Garcia CreditAttribution: Manuel Garcia as a volunteer and at Appnovation for Pfizer, Inc. commentedThanks for committing catch!
I have updated the CR with regards to #52, fixed the branch & version for it and published it.
Comment #81
nofue CreditAttribution: nofue as a volunteer commentedIt seems labels in views blocks are still missing from 8.8.4 …
Comment #85
quietone CreditAttribution: quietone at PreviousNext commentedClosed #2629566: Label is not shown for blocks with exposed forms as a duplicate. Adding credit for the work done over there.