Updated: Comment #0

Problem/Motivation

In #2056513: Remove PluginUI subsystem, provide block plugins UI as one-off, a new annotation key was added to Block plugins: category
This would allow future categorization of blocks, but initially was just defaulted to the module providing the plugin.

Now that would like to use this in the UI in #2058321: Move the 'place block' UI into the block listing, we should make it a proper string, and translatable.

Proposed resolution

Use @Translation in annotations, and t() in any dynamic additions of category

Remaining tasks

Write patch

User interface changes

Proper strings for block categories

API changes

Is this an API change? No idea.

#2058321: Move the 'place block' UI into the block listing

Files: 
CommentFileSizeAuthor
#32 block-2060859-32.patch5.46 KBGábor Hojtsy
PASSED: [[SimpleTest]]: [MySQL] 58,992 pass(es).
[ View ]
#29 block-2060859-29.patch5.47 KBtim.plunkett
PASSED: [[SimpleTest]]: [MySQL] 58,808 pass(es).
[ View ]
#23 block-2060859-23.patch5.47 KBtim.plunkett
PASSED: [[SimpleTest]]: [MySQL] 58,420 pass(es).
[ View ]
#23 interdiff.txt577 bytestim.plunkett
#20 block-2060859-20.patch4.9 KBtim.plunkett
FAILED: [[SimpleTest]]: [MySQL] 58,412 pass(es), 0 fail(s), and 2 exception(s).
[ View ]
#20 interdiff.txt1.18 KBtim.plunkett
#18 block-2060859-18-A.patch4.91 KBtim.plunkett
FAILED: [[SimpleTest]]: [MySQL] Setup environment: Test cancelled by admin prior to completion.
[ View ]
#18 block-2060859-18-B.patch4.9 KBtim.plunkett
FAILED: [[SimpleTest]]: [MySQL] 58,410 pass(es), 0 fail(s), and 2 exception(s).
[ View ]
#18 interdiff-A.txt1.02 KBtim.plunkett
#18 interdiff-B.txt1.25 KBtim.plunkett
#14 block-2060859-14.patch4.64 KBtim.plunkett
PASSED: [[SimpleTest]]: [MySQL] 58,115 pass(es).
[ View ]
#10 block-2060859-10.patch4.29 KBtim.plunkett
PASSED: [[SimpleTest]]: [MySQL] 58,064 pass(es).
[ View ]
#10 interdiff.txt1.02 KBtim.plunkett
#8 block-2060859-8.patch4.24 KBtim.plunkett
PASSED: [[SimpleTest]]: [MySQL] 57,881 pass(es).
[ View ]
#6 block-2060859-5.patch4.22 KBtim.plunkett
PASSED: [[SimpleTest]]: [MySQL] 58,136 pass(es).
[ View ]
#6 interdiff.txt3.19 KBtim.plunkett
#3 block-2060859-3.patch2.04 KBtim.plunkett
PASSED: [[SimpleTest]]: [MySQL] 57,957 pass(es).
[ View ]

Comments

Where would this string appear on the UI?

It's the category title on the right side. Instead of forcing this to be the provider, you're giving people the ability to actually group blocks by something more relevant, and it also lets you expose this from the ui. Views (for example) will almost certainly want to take advantage of this. I could see Custom Blocks doing it too.

Eclipse

Status:Active» Needs review
StatusFileSize
new2.04 KB
PASSED: [[SimpleTest]]: [MySQL] 57,957 pass(es).
[ View ]

ModulesListForm doesn't seem to translate the human-readable module name:

    $row['name']['#markup'] = $module->info['name'];
    $row['description']['#markup'] = $this->translationManager->translate($module->info['description']);

So I'm not sure if that should be wrapped in t() or not.

This contains the change to SystemMenuBlock that is in #2058321: Move the 'place block' UI into the block listing, except that sets it to "menu" instead of @Translation("Menu")

I think there has been some fierce discussions historically about whether module names should even be possible to be translated. They are made available for translation on localize.drupal.org but maybe not actually translated on the UI. It is important that people know they use the 'Views' module and not 'Ansichten', 'Blick', or 'Meinung', all of which are different translations of the words 'View'/'Views'. That helps finding tutorials, updates, and so on. Once they become exposed as block categories, that may be a different question is we deliberately want to deviate from them being module names and be free-form plugin provided strings instead. Translatability applies then.

#152375: Implement translatable module names (with context) has lots of the arguments around module name translation. It was marked won't fix more than once....

StatusFileSize
new3.19 KB
new4.22 KB
PASSED: [[SimpleTest]]: [MySQL] 58,136 pass(es).
[ View ]

Confirmed with @Gábor Hojtsy that this was okay.

So, I don't really have an opinion with regard to the whole translating languages thing. I expected us to process in a "Miscellaneous" option into the plugin definitions & just allow plugins to override that with specific annotations of what category they belong in. Anything beyond that is over and above what I was expecting here. I'm fine either way, and in general the patch is looking ok to me. If Gabor's on board with your specific translation approach for module names in the UI, then I'm fine with that too.

Eclipse

StatusFileSize
new4.24 KB
PASSED: [[SimpleTest]]: [MySQL] 57,881 pass(es).
[ View ]

Reroll, no change.

Yeah this looks good, a few small doc things and then this is RTBC for me

  1. +++ b/core/modules/block/lib/Drupal/block/Plugin/Type/BlockManager.php
    @@ -22,6 +23,20 @@
    +   * Array of all available modules and their data.
    Would this be better as, "An array of...."?
  2. +++ b/core/modules/block/lib/Drupal/block/Plugin/Type/BlockManager.php
    @@ -47,9 +65,31 @@ public function processDefinition(&$definition, $plugin_id) {
    +   *   The human-readable name of the module.
    Since this can also return a machine name, should we improve this? Or maybe the description above.

StatusFileSize
new1.02 KB
new4.29 KB
PASSED: [[SimpleTest]]: [MySQL] 58,064 pass(es).
[ View ]

Fair points!

Status:Needs review» Reviewed & tested by the community

Looks good.

Status:Reviewed & tested by the community» Needs review

I'm wondering whether or not we should be using the translation context system here? For example, t('Block') exists in \Drupal\aggregator\FeedStorageController.

Assigned:Unassigned» Gábor Hojtsy

No idea.

StatusFileSize
new4.64 KB
PASSED: [[SimpleTest]]: [MySQL] 58,115 pass(es).
[ View ]

Currently the module names are extracted from .info.yml files but not assigned any context. If this code would use a context when translating, that would mean that (a) the annotation for the category should also use a context (b) the .info.yml extractor should extract the module name ALSO with that a context for this. So while doing the context would possibly be useful, its not trivial work :) Also it would expose all the module names once more to translators under a different context name. Not sure that is useful.

What would the context be? "Block category"?

Status:Needs review» Reviewed & tested by the community

I think if anything, then FeedStorageController needs some context.

Status:Reviewed & tested by the community» Needs work

+++ b/core/modules/block/lib/Drupal/block/Plugin/Type/BlockManager.phpundefined
@@ -48,10 +67,36 @@ public function processDefinition(&$definition, $plugin_id) {
+    if (!isset($this->moduleData)) {
+      $this->moduleData = system_rebuild_module_data();
+    }
+    // If the module exists, return its human-readable name.
+    if (isset($this->moduleData[$module])) {
+      return $this->translationManager->translate($this->moduleData[$module]->info['name']);

This should use system_get_info(), no need to rebuild.

Also needs a re-roll for $this->t()

Assigned:Gábor Hojtsy» catch
Status:Needs work» Reviewed & tested by the community
StatusFileSize
new1.25 KB
new1.02 KB
new4.9 KB
FAILED: [[SimpleTest]]: [MySQL] 58,410 pass(es), 0 fail(s), and 2 exception(s).
[ View ]
new4.91 KB
FAILED: [[SimpleTest]]: [MySQL] Setup environment: Test cancelled by admin prior to completion.
[ View ]

function system_get_info($type, $name = NULL) {
  $info = array();
  if ($type == 'module') {
    $data = system_rebuild_module_data();
    foreach (Drupal::moduleHandler()->getModuleList() as $module => $filename) {
      $info[$module] = $data[$module]->info;
    }
  }

There is no extra rebuilding here, all we're doing is not checking it against getModuleList()...
But that's your call.

Uploading both just in case.

Status:Reviewed & tested by the community» Needs work

Status:Needs work» Needs review
StatusFileSize
new1.18 KB
new4.9 KB
FAILED: [[SimpleTest]]: [MySQL] 58,412 pass(es), 0 fail(s), and 2 exception(s).
[ View ]

Hot damn, I didn't know we could do that. ModuleInfo++

Status:Needs review» Reviewed & tested by the community

If the bot likes it, looks good to me.

Status:Reviewed & tested by the community» Needs work

The last submitted patch, block-2060859-20.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new577 bytes
new5.47 KB
PASSED: [[SimpleTest]]: [MySQL] 58,420 pass(es).
[ View ]

WTF.

Undefined index: minimal Notice system.module 2442 system_get_info()

'system_get_info(\'module\')
Drupal\\Core\\Utility\\ModuleInfo->resolveCacheMiss(\'name\')
Drupal\\Core\\Utility\\CacheArray->offsetGet(\'name\')
system_get_module_info(\'name\')
Drupal\\block\\Plugin\\Type\\BlockManager->getModuleName(\'node\')
Drupal\\block\\Plugin\\Type\\BlockManager->processDefinition(Array, \'node_recent_block\')
Drupal\\Core\\Plugin\\DefaultPluginManager->findDefinitions()
Drupal\\Core\\Plugin\\DefaultPluginManager->getDefinitions()
Drupal\\Core\\Plugin\\DefaultPluginManager->getDefinition(\'system_menu_block:menu-admin\')
Drupal\\Core\\Plugin\\Factory\\ContainerFactory->createInstance(\'system_menu_block:menu-admin\', Array)
Drupal\\Component\\Plugin\\PluginManagerBase->createInstance(\'system_menu_block:menu-admin\', Array)
Drupal\\Component\\Plugin\\DefaultSinglePluginBag->initializePlugin(\'system_menu_block:menu-admin\')
Drupal\\block\\BlockPluginBag->initializePlugin(\'system_menu_block:menu-admin\')
Drupal\\Component\\Plugin\\PluginBag->get(\'system_menu_block:menu-admin\')
Drupal\\block\\BlockPluginBag->get(\'system_menu_block:menu-admin\')
Drupal\\block\\Entity\\Block->getPlugin()
Drupal\\block\\{closure}(Object)
array_filter(Array, Object)
Drupal\\block\\BlockStorageController->loadMultiple()
Drupal\\Core\\Config\\Entity\\ConfigStorageController->loadByProperties(Array)
entity_load_multiple_by_properties(\'block\', Array)
block_list(\'sidebar_first\')
block_get_blocks_by_region(\'sidebar_first\')
block_page_build(Array)
drupal_render_page(Array)
Drupal\\Core\\Controller\\HtmlPageController->content(Object, \'\\Drupal\\user\\Controller\\UserController::userPage\')
call_user_func_array(Array, Array)
Symfony\\Component\\HttpKernel\\HttpKernel->handleRaw(Object, 1)
Symfony\\Component\\HttpKernel\\HttpKernel->handle(Object, 1, 1)
Drupal\\Core\\HttpKernel->handle(Object, 1, 1)
Drupal\\Core\\DrupalKernel->handle(Object)
drupal_handle_request()

I don't even want to know.

Hm. that function and class is removed in #2025779: Remove ModuleInfo as it is no longer necessary as it was considered to be no longer necessary (by @catch :)) as we don't store that much in .info files anymore. As it didn't get in in time, we might want to keep the function but drop the caching.

I don't think we need CacheCollector there, but we could use a persistent cache for this still I think. I thought system_get_info() had a basic persistent cache (which is why I originally mentioned that function), but clearly it doesn't. Everything about system info is still a big mess...

So where do we go from here? I'm inclined to just go back to 18B, system_get_info('module') for now, in case #2025779: Remove ModuleInfo as it is no longer necessary happens, and if not, add a persistent cache in another issue.

#18: block-2060859-18-B.patch queued for re-testing.

Argh sorry. Let's do that and open a separate issue to add a simple cache to system_get_info().

StatusFileSize
new5.47 KB
PASSED: [[SimpleTest]]: [MySQL] 58,808 pass(es).
[ View ]

Okay then.

Status:Needs review» Reviewed & tested by the community

Looks good to me.

Assigned:catch» Unassigned
Status:Reviewed & tested by the community» Needs work

Seems like catch's concern is resolved here, Gábor is +1. Looks good to me, too.

However, seems to no longer apply. :(

Status:Needs work» Reviewed & tested by the community
StatusFileSize
new5.46 KB
PASSED: [[SimpleTest]]: [MySQL] 58,992 pass(es).
[ View ]

This was an easy reroll :) The system menu block category annotation change was the only thing that did not apply anymore.

Status:Reviewed & tested by the community» Fixed

Committed and pushed to 8.x. Thanks!

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