Comments

Status: Needs review » Needs work

The last submitted patch, fix-empty-block.patch, failed testing.

anavarre’s picture

Status: Needs work » Needs review
StatusFileSize
new1.51 KB
new972 bytes

I'm not sure I understand the point of the test in BlockExampleTest.php::testBlockExampleBasic(). We have a comment saying "Verify that blocks are there. Empty block will not be shown, because it is empty." but the goal is to display an empty block, not to demonstrate that an empty array prevents the block from showing up, right?

With the above patch, the below assertion will thus fail and we'd need to use assertRaw() instead.

$this->assertNoRaw($settings_empty['label'], 'Block empty not found.');

If we really, truly think that behavior is correct, then it'd require more documentation in ExampleEmptyBlock::build() because it's confusing for sure.

Attached patch fixes the test accordingly to what's the assumed normal behavior.

marvil07’s picture

I guess the confusion comes from the interpretation of what an empty block means. In the current code the idea seems to be referring to an empty block array instead of an empty block content. I would say it is OK to extend documentation to mention the difference.

anavarre’s picture

StatusFileSize
new1.25 KB

I still don't really understand why we'd prefer an empty array to an empty block content (especially since it was an empty block content before) but here's another approach with better documentation to hopefully clarify the intent.

Status: Needs review » Needs work

The last submitted patch, 4: 2510746-4-improve-documentation.patch, failed testing.

anavarre’s picture

Status: Needs work » Needs review
StatusFileSize
new1.25 KB
mile23’s picture

Status: Needs review » Fixed

Nice documentation patch, thanks.

  • Mile23 committed c40800f on 8.x-1.x authored by anavarre
    Issue #2510746 by anavarre, marvil07: Empty Block doesn't show up...

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.