While working on a test case for #2031859-35: Invoke an event[s] when a plugin ID disappears, 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 a plugin ID disappears which will test this function too. Maybe this patch could go in without the test case. Thoughts, please?
Comment | File | Size | Author |
---|---|---|---|
#5 | aggregator-category-blocks-2057355-5.patch | 1.1 KB | hussainweb |
#1 | aggregator-category-blocks-2057355-1.patch | 1.09 KB | hussainweb |
Comments
Comment #1
hussainwebThe patch as promised. :)
Comment #2
hussainwebGreat, its green. The test for this is in the patch in #2031859-49: Invoke an event[s] when a plugin ID disappears.
Comment #3
dawehnerWe certainly needs tests! The actual code looks fine, though.
Comment #4
hussainwebYes, and the tests are a part of the patch in #2031859-49: Invoke an event[s] when a plugin ID disappears:
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 a plugin ID disappears. Just introduces more work. Is that necessary? I mean, the tests are there.
Let me know what you think.
Comment #5
hussainwebAttaching just a small tweak in the comment, for better clarity.
Comment #6
ParisLiakos CreditAttribution: ParisLiakos commentedwell 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
Comment #7
hussainwebIf 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 a plugin ID disappears.
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. :)
Comment #8
ParisLiakos CreditAttribution: ParisLiakos commentedthose 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
Comment #9
BerdirNoticed 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 :)
Comment #10
Berdir#1888702: Use configuration selection instead of derivatives for some blocks is RTBC and has a test for category blocks.