While working on a test case for #2031859-35: Invoke an event[s] when something happens that makes a block plugin go away, I found that aggregator category blocks do not render at all because of changes in #1535868: Convert all blocks into plugins. The fix is simple, hopefully and I will be posting it in a comment.

I am not sure if this qualifies as a critical or major bug. Given that one of the system is not at all working and there is no workaround, I think it is at least major. It is quite surprising that it was not spotted earlier. I guess there are no tests for this. I am working on a test for #2031859: Invoke an event[s] when something happens that makes a block plugin go away which will test this function too. Maybe this patch could go in without the test case. Thoughts, please?

Files: 
CommentFileSizeAuthor
#5 aggregator-category-blocks-2057355-5.patch1.1 KBhussainweb
PASSED: [[SimpleTest]]: [MySQL] 57,588 pass(es).
[ View ]
#1 aggregator-category-blocks-2057355-1.patch1.09 KBhussainweb
PASSED: [[SimpleTest]]: [MySQL] 57,865 pass(es).
[ View ]

Comments

Status:Active» Needs review
StatusFileSize
new1.09 KB
PASSED: [[SimpleTest]]: [MySQL] 57,865 pass(es).
[ View ]

The patch as promised. :)

Issue tags:+Needs tests

We certainly needs tests! The actual code looks fine, though.

Yes, and the tests are a part of the patch in #2031859-49: Invoke an event[s] when something happens that makes a block plugin go away:

+
+    // Place the category 2 block.
+
+    // Clear the block cache to load the new block definitions.
+    $this->container->get('plugin.manager.block')->clearCachedDefinitions();
+
+    // Need admin user to be able to access block admin.
+    $admin_user = $this->drupalCreateUser(array(
+      'administer blocks',
+      'access administration pages',
+      'administer news feeds',
+      'access news feeds',
+      'create article content',
+    ));
+    $this->drupalLogin($admin_user);
+
+    $block = $this->drupalPlaceBlock("aggregator_category_block:{$category_2_record->cid}", array('label' => 'category-' . $category_2_new_title, 'block_count' => 2));
+
+    // The block should not appear as there are no feed items.
+    $this->drupalGet('test-page');
+    $this->assertNoText($block->label(), format_string('Feed Category block %label is not displayed on the page.', array('%label' => $block->label())));
+
+    // Create some nodes and update the feed items.
+    $this->createSampleNodes();
+    $this->updateFeedItems($db_feed, $this->getDefaultFeedItemCount());
+
+    // The block should appear now.
+    $this->drupalGet('test-page');
+    $this->assertText($block->label(), format_string('Feed Category block %label is displayed on the page.', array('%label' => $block->label())));

It is not just a matter of convenience. The test is necessary for that issue as well. If I have to put the test in this commit, then it means that we have to wait for this to get committed, then reroll the patch in #2031859-49: Invoke an event[s] when something happens that makes a block plugin go away. Just introduces more work. Is that necessary? I mean, the tests are there.

Let me know what you think.

StatusFileSize
new1.1 KB
PASSED: [[SimpleTest]]: [MySQL] 57,588 pass(es).
[ View ]

Attaching just a small tweak in the comment, for better clarity.

Status:Needs review» Needs work

well we either put a test here, or include this fix in the issue above, its a one liner anyway..i dont think it will be committed without tests

If those two are the only choices we have, then I think I'd prefer adding tests here. The issue here is only tangential to the issue in #2031859: Invoke an event[s] when something happens that makes a block plugin go away.

I would still say to just commit it here and wait for the tests in the other issue, as it becomes a consolidated fix (and the test is really valid there too). I see the point in adding tests here, but it is just extra work. :)

those two are the only choices indeed:)

imagine if we get this one line fix in here, but the issue above does not ship in D8..it means that this is still untested and subject to break without us knowing ;)
so we need to make sure this fix gets in with its test the same time. being in the issue here or the issue above;)

thanks for working on this btw

Noticed this too when I worked on #1888702: Use configuration selection instead of derivatives for some blocks. Help me get that in and we can close this and another major bug :)

Status:Needs work» Closed (duplicate)