Problem/Motivation

#2349991: Provide a trait for categorizing plugin managers and use it for conditions and actions added this code with no test coverage. The unsetting of 'broken' in getGroupedDefinitions is redundant, since it always calls getSortedDefinitions first.
While unset() doesn't trigger notices for missing keys, it does for subkeys of missing keys: 

unset($definitions[$this->t('Block')]['broken']);

will always fail.

Proposed resolution

Don't do the unnecessary unset, add tests that would have caught this.

Remaining tasks

N/A

User interface changes

N/A

API changes

N/A

Data model changes

N/A

CommentFileSizeAuthor
#10 2884769-10.pass_.patch4.51 KBlarowlan
#10 2884769-10.fail_.patch2.98 KBlarowlan
#9 2884769-9.patch4.51 KBlarowlan
#9 interdiff.2884769-9.txt819 byteslarowlan
#7 2884769-block-broken-7.patch4.43 KBtim.plunkett
#7 2884769-block-broken-7-interdiff.txt1.4 KBtim.plunkett
#4 2884769-block-broken-4.patch4.4 KBtim.plunkett
#4 2884769-block-broken-4-interdiff.txt5.66 KBtim.plunkett
#3 2884769-block-broken-3-interdiff.txt1.39 KBtim.plunkett
#3 2884769-block-broken-3-PASS.patch4.32 KBtim.plunkett
#3 2884769-block-broken-3-FAIL.patch2.79 KBtim.plunkett
#2 2884769-block-broken-2.patch3.5 KBtim.plunkett
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

tim.plunkett created an issue. See original summary.

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
CreditAttribution: tim.plunkett at Acquia commented
Status: Active » Needs review
FileSize
2884769-block-broken-2.patch3.5 KB
tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
CreditAttribution: tim.plunkett at Acquia commented
FileSize
2884769-block-broken-3-FAIL.patch2.79 KB
2884769-block-broken-3-PASS.patch4.32 KB
2884769-block-broken-3-interdiff.txt1.39 KB

The entire method override is unnecessary, actually.

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
CreditAttribution: tim.plunkett at Acquia commented
FileSize
2884769-block-broken-4-interdiff.txt5.66 KB
2884769-block-broken-4.patch4.4 KB

@tedbow points out that asserting for those specific 4 blocks is odd.
Those are the 4 that are provided by subsystems, and are available when no modules are enabled. But this test would be brittle, and fail if other subsystems provide blocks in the future.

Therefore I rewrote the test as a unit test.

tedbow’s picture

tedbow
he/him
Primary language English
Location Ithaca, NY, USA
CreditAttribution: tedbow at Acquia commented 
+++ b/core/tests/Drupal/Tests/Core/Block/BlockManagerTest.php
@@ -0,0 +1,87 @@
+    $this->assertSame(['block2', 'block3', 'block1'], array_keys($definitions['expected_category']));

Could put a 1 block in "expected_category_1" and 2 in expected "expected_category_2" to prove it will be group into multiple categories correctly?

The last submitted patch, 3: 2884769-block-broken-3-FAIL.patch, failed testing. View results

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
CreditAttribution: tim.plunkett at Acquia commented
FileSize
2884769-block-broken-7-interdiff.txt1.4 KB
2884769-block-broken-7.patch4.43 KB

Good idea. I used real strings, since that is the intention of the category.

tedbow’s picture

tedbow
he/him
Primary language English
Location Ithaca, NY, USA
CreditAttribution: tedbow at Acquia commented
Status: Needs review » Reviewed & tested by the community

@tim.plunkett looks good! RTBC 🎉!

larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
CreditAttribution: larowlan at PreviousNext commented
Status: Reviewed & tested by the community » Needs review
FileSize
interdiff.2884769-9.txt819 bytes
2884769-9.patch4.51 KB

So reviewed this and found that the test fails in HEAD, but not because it catches the bug (we don't have a test only patch for the new test).

PHPUnit 4.8.35 by Sebastian Bergmann and contributors.

Testing Drupal\Tests\Core\Block\BlockManagerTest
..E

Time: 945 ms, Memory: 6.00MB

There was 1 error:

1) Drupal\Tests\Core\Block\BlockManagerTest::testGroupedDefinitions
Drupal\Core\DependencyInjection\ContainerNotInitializedException: \Drupal::$container is not initialized yet. \Drupal::setContainer() must be called with a real container.

/Volumes/code/git/drupal/core/lib/Drupal.php:129
/Volumes/code/git/drupal/core/lib/Drupal.php:158
/Volumes/code/git/drupal/core/lib/Drupal/Core/StringTranslation/StringTranslationTrait.php:104
/Volumes/code/git/drupal/core/lib/Drupal/Core/StringTranslation/StringTranslationTrait.php:71
/Volumes/code/git/drupal/core/lib/Drupal/Core/Block/BlockManager.php:70
/Volumes/code/git/drupal/core/tests/Drupal/Tests/Core/Block/BlockManagerTest.php:82

FAILURES!
Tests: 3, Assertions: 2, Errors: 1.

Because the existing code tries to unset on the translated string

    unset($definitions[$this->t('Block')]['broken']);

Attached patch sets a mock string translation service on the manager in the test.

Which then yields

./vendor/bin/phpunit -c core core/tests/Drupal/Tests/Core/Block/BlockManagerTest.php 
PHPUnit 4.8.35 by Sebastian Bergmann and contributors.

Testing Drupal\Tests\Core\Block\BlockManagerTest
..E

Time: 230 ms, Memory: 6.00MB

There was 1 error:

1) Drupal\Tests\Core\Block\BlockManagerTest::testGroupedDefinitions
Illegal offset type

/Volumes/code/git/drupal/vendor/symfony/phpunit-bridge/DeprecationErrorHandler.php:73
/Volumes/code/git/drupal/core/lib/Drupal/Core/Block/BlockManager.php:70
/Volumes/code/git/drupal/core/tests/Drupal/Tests/Core/Block/BlockManagerTest.php:83

FAILURES!
Tests: 3, Assertions: 2, Errors: 1.

Which is the expected fail.

I think this is ready too - thanks.

larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
CreditAttribution: larowlan at PreviousNext commented
FileSize
2884769-10.fail_.patch2.98 KB
2884769-10.pass_.patch4.51 KB

just for completeness here's fail/pass patches

The last submitted patch, 10: 2884769-10.fail_.patch, failed testing. View results

tedbow’s picture

tedbow
he/him
Primary language English
Location Ithaca, NY, USA
CreditAttribution: tedbow at Acquia commented
Status: Needs review » Reviewed & tested by the community

So since @larowlan

I think this is ready too - thanks.

just for completeness here's fail/pass patches

Setting back to RTBC because of expected pass/fail in #10

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
CreditAttribution: tim.plunkett at Acquia commented

Except that string translation is no longer needed after this patch.
While the interdiff in #9 is necessary for the FAIL patch, it's not for the PASS patch.
Doesn't really matter to me...

The confusion stems from the same issue that introduced this bug, linked in the IS:
CategorizingPluginManagerTrait has NO need to use StringTranslationTrait.

But removing that might be a problem, and is out of scope.

Gábor Hojtsy’s picture

Gábor Hojtsy
he/him
Primary language Hungarian
Location Hungary
CreditAttribution: Gábor Hojtsy at Acquia commented

  • Gábor Hojtsy committed 01c7b05 on 8.3.x 
    Issue #2884769 by tim.plunkett, larowlan, tedbow: \Drupal\Core\Block\...

  • Gábor Hojtsy committed 67f58f6 on 8.4.x 
    Issue #2884769 by tim.plunkett, larowlan, tedbow: \Drupal\Core\Block\...
Gábor Hojtsy’s picture

Gábor Hojtsy
he/him
Primary language Hungarian
Location Hungary
CreditAttribution: Gábor Hojtsy at Acquia commented
Version: 8.4.x-dev » 8.3.x-dev
Status: Reviewed & tested by the community » Fixed

Good find @larowlan. Looks like everyone agrees this is the better fix, so went ahead and committed it. Verified that the method from the trait looks good and already filters out the broken plugin.

Status: Fixed » Closed (fixed)

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