Closed (fixed)
Project:
Drupal core
Version:
8.0.x-dev
Component:
theme system
Priority:
Major
Category:
Bug report
Assigned:
Issue tags:
Reporter:
Created:
2 Oct 2011 at 18:55 UTC
Updated:
29 Jul 2014 at 20:01 UTC
Jump to comment: Most recent, Most recent file
Comments
Comment #1
webchickThanks to the magic of git bisect, I discovered that this bug is caused by #918808: Standardize block cache as a drupal_render() #cache. Cross-linking both issues.
Comment #2
reglogge commentedsub
Comment #3
moshe weitzman commentedI'll take this ... IMO this is about as mild a UI bug as one could imagine, and it occurs when we are about 2 years from release.
Comment #4
moshe weitzman commentedPosted in the wrong issue, copied by chx.
Comment #5
chx commentedComment #6
webchickw00t! That takes care of it. Thanks!
Not sure if we need tests for this or not. I spent about an hour trying to write a holistic test that spun through all of the enabled blocks, checked if they had visible content when rendered, and if not, asserting that they didn't show up on the page. After 3 attempts I gave up; our block API sucks. :\
So tentatively moving to RTBC here, but we'll see what catch says.
Comment #7
chx commented$this->assertNoRaw('block-system-help')
Edit: yes, that's the whole test. wrap it in a class with getinfo and a test function but that's all you need to do, check the lack of that string just after install. Alternatively you can sneak it into another test to avoid a whole Drupal install for a single assert.
Comment #8
webchickYeah, that was my first attempt, but that's not really a test. It would prevent this particular problem from happening again, but the real bug is that the logic to prevent empty blocks from showing was broken.
Comment #9
moshe weitzman commentedI think the proposal is a reasonable test for 'prevent empty blocks from showing'.
Comment #10
webchickSorry, accidental cross-post there. Tagging.
Comment #11
webchickOh, really? OK then...
Comment #12
webchickHow about one where I don't take you quite so literally, and include an assertion message? :P
Two patches. One with just the test so you can see it failing, the other one with the test + patch.
Note that the patch resides in an overall test function called testBlockVisibility(), so it seems to be in an appropriate place.
Comment #13
jthorson commentedRe-testing.
Comment #14
chx commentedWell this means this needs a change node then because of what happened to book.
Comment #15
bfroehle commentedMarked #1299022: Block module should not print empty elements as a duplicate.
Comment #16
catchThis looks fine. I'm a bit concerned about adding a test that will continue to pass even if we don't have a help block in core any more, but can't think of a nice generic way to test this either that wouldn't be a pain. Committed and pushed.
The API change should be documented as part of #918808: Standardize block cache as a drupal_render() #cache, so putting straight to fixed.