Some block categories are not translatable

@dschenk and me are working on this, while sprinting at DrupalCon in Barcelona.

In this patch we added the category explicitly to the corresponding blocks in the system module, to make them translatable.

I helped in the creation of the patch.

I was able to reproduce the issue and confirm that the patch in #5 works.

Nice find! I noticed this once before but did not track down why it was happening. The fix seems correct.

I think we could provide test coverage to this to make sure future system blocks also get the correct category. The test might load the translated admin listing, assert that expected blocks are displayed, and assert that the untranslated word "system" does not appear.

Alternately (or additionally), is declaring a block without a category even valid? Should we warn developers of this somehow?

I think that declaring a block without a category is valid, at least that's what I understand from the default empty value in the annotation. I haven't found out why and how (and I would like know if someone can illuminate me), but when we don't declare a category, it uses the module's name.

What's happening here is that the 5 blocks from the system module don't declared a category (so it uses 'System' as a fall back), and that two blocks from other modules declared there category as 'System' and made it translatable. I think the module's name fall back is OK.

On a different note, the admin label too is optional, and it doesn't have a fall back, so when you don't declare one it stays blank. Shouldn't we use the block's ID in that case for the admin label?

Sounds like a backwards incompatible change to require categories. But fixing this problem by adding categories sounds fine. So maybe we should test that all core blocks provide a category as a best practice, but not as a requirement.

Here's a test to check that every core block have a category.

Hmm, it seems like my test does not catch all blocks without a category.
I guess it's because the namespaces array is only filled with enabled modules.
Is there a way to get ALL namespaces?

Here's a way to find all block plugins: loading every (non-test) modules to fill the namespaces array.

Just found a better way to get a list of all modules, thanks to @heddn!

I started out by testing the patch from #20 using the Korean translation.

Before:

After:

So I would say that the patch works as designed.

Then I followed the code a bit down the call stack and made a new patch. The only change is in core/lib/Drupal/Core/Plugin/CategorizingPluginManagerTrait.php:processDefinitionCategory() where I've passed the retrieved module name into t(). This would allow all categories that haven't been set to be translatable.

Changed the test to expect a TranslatableMarkup object instead of a string as default category.

The category was accidentally removed from PageTitleBlock.

Uploading re-rolled patch. It fixes the bug (see attached screenshot), but I haven't ran the tests to see if anything breaks. Lots of changes between 8.4.x and 8.7.x!

Ran tests -- the 2 that had been altered passed. All the reroll changes were the same as in the 8.4.x patch so marking this RTBC. Great job on this, looks great!

+++ b/core/lib/Drupal/Core/Plugin/CategorizingPluginManagerTrait.php
@@ -31,7 +31,7 @@ protected function processDefinitionCategory(&$definition) {
-      $definition['category'] = $this->getProviderName($definition['provider']);
+      $definition['category'] = $this->t($this->getProviderName($definition['provider'])); ¶

I'm not sure this is correct. The @translation should pass a TranslationMarkup object into the definition.

Also we should have the test that ensures all the blocks have categories that was in #27.

This is still relevant and the patch applies to 9.5.x.

Spun up an MR and started with the (slightly refactored) test from #27. Can see the test-only results at https://git.drupalcode.org/issue/drupal-2500607/-/jobs/5680572

1) Drupal\Tests\block\Kernel\BlockDefinitionsTest::testBlockCategory
Block plugins missing categories: views_exposed_filter_block, views_block, system_powered_by_block, system_messages_block, system_main_block, system_breadcrumb_block, system_branding_block, system_clear_cache_block, navigation_user, navigation_shortcuts, navigation_link, field_block, extra_field_block, help_block, announce_block, local_tasks_block, local_actions_block, page_title_block
Failed asserting that an array is empty.

I then applied all the same categories from the existing patches, but of course we have attributes instead of annotations now, so had to be done manually. There were some new plugins revealed by the failing test so I added categories for those as well.

Then I added in the changes to CategorizingPluginManagerTrait and CategorizingPluginManagerTraitTest, but didn't quite follow @alexpott's comment in #33. Maybe it wasn't clear that this is a fallback, so we get the "provider" and try to translate that.

Guessing the IS needs an update, maybe some new screenshots.

The Needs Review Queue Bot tested this issue.

While you are making the above changes, we recommend that you convert this patch to a merge request. Merge requests are preferred over patches. Be sure to hide the old patch files as well. (Converting an issue to a merge request without other contributions to the issue will not receive credit.)

Ok thanks bot. Hiding patches, even though I think they are useful for confirming that the MR is on the right track.

Amending attribution.

Can we update the summary with a proposed solution.

Also this brings up a question about attributes, if we are adding a new key how do we ensure contrib/custom modules update theirs too? WOuld have to be done in the attribute itself?

Can we update the summary with a proposed solution.

Done

Also this brings up a question about attributes, if we are adding a new key how do we ensure contrib/custom modules update theirs too? WOuld have to be done in the attribute itself?

The key already exists in the attribute, but it's optional. We're ensuring all core blocks have a category set. Contrib/custom will fallback to the module name, which we're now translating. Although I was under the impression we're only meant to translate string literals, so not sure if that part is suitable. We could always deprecate not passing a category, but that could be pretty annoying.

Did a search for #[Block and there appear to still be other instances

Mainly test blocks, if those aren't needed can we least update the block.api

If you are another contributor eager to jump in, please allow the previous poster(s) at least 48 hours to respond to feedback first, so they have the opportunity to finish what they started!

I don't think we need to do the test blocks. Category is still optional, we're just ensuring core blocks set the category. I've updated the example in block.api.php

I'm still unsure about the fallback translation in CategorizingPluginManagerTrait. If we have that, why bother enforcing all the core blocks have a category?

idk if a good search but I looked for $definition['provider'] and didn't see any other spots that are doing translations.

what you say that's a blocker for this moving forward?

MR still applies and don't think my comment should hold things

I have a problem with this line, There is not a finite amount of values for $definition['provider'] passing that through t() is not a good idea.

      $definition['category'] = $this->t($this->getProviderName($definition['provider']));

The rest is fine.

Updated the proposed resolution to delete "Translate the fallback category (module name) for other blocks". Updated the MR as per #58.

Since the change to CategorizingPluginManagerTrait was reverted now CategorizingPluginManagerTraitTest is failing.