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

CommentFileSizeAuthor
#32 block-2060859-32.patch5.46 KBGábor Hojtsy
#29 block-2060859-29.patch5.47 KBtim.plunkett
#23 block-2060859-23.patch5.47 KBtim.plunkett
#23 interdiff.txt577 bytestim.plunkett
#20 block-2060859-20.patch4.9 KBtim.plunkett
#20 interdiff.txt1.18 KBtim.plunkett
#18 block-2060859-18-A.patch4.91 KBtim.plunkett
#18 block-2060859-18-B.patch4.9 KBtim.plunkett
#18 interdiff-A.txt1.02 KBtim.plunkett
#18 interdiff-B.txt1.25 KBtim.plunkett
#14 block-2060859-14.patch4.64 KBtim.plunkett
#10 block-2060859-10.patch4.29 KBtim.plunkett
#10 interdiff.txt1.02 KBtim.plunkett
#8 block-2060859-8.patch4.24 KBtim.plunkett
#6 block-2060859-5.patch4.22 KBtim.plunkett
#6 interdiff.txt3.19 KBtim.plunkett
#3 block-2060859-3.patch2.04 KBtim.plunkett
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Gábor Hojtsy’s picture

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

Where would this string appear on the UI?

EclipseGc’s picture

EclipseGc CreditAttribution: EclipseGc commented

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

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
CreditAttribution: tim.plunkett commented
Status: Active » Needs review
FileSize
block-2060859-3.patch2.04 KB

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")

Gábor Hojtsy’s picture

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

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.

Gábor Hojtsy’s picture

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

#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....

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
CreditAttribution: tim.plunkett commented
FileSize
interdiff.txt3.19 KB
block-2060859-5.patch4.22 KB

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

EclipseGc’s picture

EclipseGc CreditAttribution: EclipseGc commented

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

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
CreditAttribution: tim.plunkett commented
FileSize
block-2060859-8.patch4.24 KB

Reroll, no change.

benjy’s picture

benjy CreditAttribution: benjy commented

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.

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
CreditAttribution: tim.plunkett commented
FileSize
interdiff.txt1.02 KB
block-2060859-10.patch4.29 KB

Fair points!

benjy’s picture

benjy CreditAttribution: benjy commented
Status: Needs review » Reviewed & tested by the community

Looks good.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott commented
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.

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
CreditAttribution: tim.plunkett commented
Assigned: Unassigned » Gábor Hojtsy

No idea.

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
CreditAttribution: tim.plunkett commented
FileSize
block-2060859-14.patch4.64 KB

Rerolled for #2065721: Add a dedicated @Block plugin annotation

Gábor Hojtsy’s picture

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

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"?

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
CreditAttribution: tim.plunkett commented
Status: Needs review » Reviewed & tested by the community

I think if anything, then FeedStorageController needs some context.

catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch commented
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()

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
CreditAttribution: tim.plunkett commented
Assigned: Gábor Hojtsy » catch
Status: Needs work » Reviewed & tested by the community
FileSize
interdiff-B.txt1.25 KB
interdiff-A.txt1.02 KB
block-2060859-18-B.patch4.9 KB
block-2060859-18-A.patch4.91 KB 
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.

catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch commented
Status: Reviewed & tested by the community » Needs work

Sorry wrong function... https://api.drupal.org/api/drupal/core!modules!system!system.module/func...

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
CreditAttribution: tim.plunkett commented
Status: Needs work » Needs review
FileSize
interdiff.txt1.18 KB
block-2060859-20.patch4.9 KB

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

benjy’s picture

benjy CreditAttribution: benjy commented
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.

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
CreditAttribution: tim.plunkett commented
Status: Needs work » Needs review
FileSize
interdiff.txt577 bytes
block-2060859-23.patch5.47 KB

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.

Berdir’s picture

Berdir
Primary language German
Location Switzerland
CreditAttribution: Berdir commented

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.

catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch commented

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...

tim.plunkett’s picture

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

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.

tim.plunkett’s picture

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

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

catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch commented

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

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
CreditAttribution: tim.plunkett commented
FileSize
block-2060859-29.patch5.47 KB

Okay then.

Gábor Hojtsy’s picture

Gábor Hojtsy
he/him
Primary language Hungarian
Location Hungary
CreditAttribution: Gábor Hojtsy commented
Status: Needs review » Reviewed & tested by the community

Looks good to me.

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
CreditAttribution: webchick commented
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. :(

Gábor Hojtsy’s picture

Gábor Hojtsy
he/him
Primary language Hungarian
Location Hungary
CreditAttribution: Gábor Hojtsy commented
Status: Needs work » Reviewed & tested by the community
FileSize
block-2060859-32.patch5.46 KB

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

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
CreditAttribution: webchick commented
Status: Reviewed & tested by the community » Fixed

Committed and pushed to 8.x. Thanks!

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