Problem/Motivation

All titles in the block drupal_coverage_core_module_block need to be displayed using their full name.

Core modules are being displayed using the machine name instead of the full name (which contains capitalized text).
Screenshot:

Proposed resolution

Make sure to use the Human readable name instead of the machine name.

Comments

legovaer created an issue. See original summary.

joaopauloc’s picture

Assigned: Unassigned » joaopauloc
bramdriesen’s picture

Issue summary: View changes
joaopauloc’s picture

Assigned: joaopauloc » Unassigned
akashkrishnan01’s picture

I would like to work on this issue, can you give me some pointers for getting started

akashkrishnan01’s picture

Assigned: Unassigned » akashkrishnan01
jonathan1055’s picture

Hi akashkrishnan,
I don't know much about how the internals of this module works, so here is what I just did to start to discover where you would fix this. I am posting it here to show how I would go about it, when you do not know where to begin. I started by trying to find when the module title was displayed:

  1. Downloaded the latest dev version and installed the module
  2. Placed the 'DC modules overview' block in the sidebar
  3. Went to 'create content' and added a core module, cleared the cache so that the block showed the text
  4. Searched the entire source folder for 'block'. This only found 4 files, so that is nice, not too much to look through
  5. The most likelist was Plugin/Block/ModulesBlock.php because this loops through all modules
  6. I added some extra text to both of the 'title' rows, and this showed me that it was the $analysis_manager->getTitle($analysis) line which was used as the title, not the $module_manager->getTitle($module) line.
  7. In my database I looked at the various node tables, and saw that node_field_data and node_field_revision hold the module name.
  8. I manually entered extra text in title fields in both tables then refreshed the block.
  9. The extra text showed me that it was the direct text in the title of node_field_revision which is displayed. This showed that the text is not derived for display, it is the actual stored db title field which is used.
  10. Therefore the way to fix this must be at node creation time, as there is not a later process which alters the text for display.

The next step would be to find if/how the title is manipulated when the node is created. I guess we need to take the users input and derive the actual human-readable module name. That might sound like an obvious conclusion, but it is worth laying it out here. Hope that helps you to get started ...

akashkrishnan01’s picture

Hey jonathan1055,
Thanks for the reply. But I am a newbie here, so can you be more specific about the drupal version you cloned and what exactly drupal 8.x-1.0alpha1 is?

bramdriesen’s picture

Hi akashkrishnan,

8.x-1.0-alpha1 is the latest "stable" alpha release of the module. This is the module that a typical user would install to test out the module. As a contributor you could checkout that version of the code to test the alpha version, but development should happen on the 8.x-1.x-dev branch. Make sure to checkout this page on how to create a patch after you have worked on an issue: https://www.drupal.org/node/707484. You can install the module on a clean install of Drupal 8.

akashkrishnan01’s picture

Hey BramDriesen,
Thanks for the reply, now it is easy to figure out this issue.

akashkrishnan01’s picture

Is this issue is to change the machine name to human readable name, i.e. from dcmodulesoverview to drupal_coverage_core_module_block?

bramdriesen’s picture

Yes that's correct. Currently core modules are displayed using the machine_name, instead of the human readable name.

akashkrishnan01’s picture

@BramDriesen, thanks

quocnht’s picture

Assigned: akashkrishnan01 » quocnht
StatusFileSize
new1.65 KB

Module node saved machine name as title and use as parameter for generating build data. As seen in ModuleManager

  /**
   * Creates a machine name for a module.
   *
   * @param string $module_name
   *   The full name of a module.
   * @param string $seperator
   *   The seperator which will be used for replacing spaces.
   *
   * @return string
   *   The machine name of the given module name.
   */
  public static function cleanModuleName($module_name, $seperator = "-") {
    return strtolower(str_replace(' ', $seperator, $module_name));
  }

- Can we just use Twig filter to change machine name to full name in block template?

- For core modules, I try to get name in configuration and pass to block. Patch attached.

bramdriesen’s picture

- Can we just use Twig filter to change machine name to full name in block template?

Sounds good to me, nice work so far.

quocnht’s picture

@BramDriesen
For Twig filter replace '_' with ' ' blank and make it titlecased ?

{{ module.title|replace({'_':' '})|title }}
jonathan1055’s picture

Status: Active » Needs review
StatusFileSize
new10.57 KB

Hi quocnht,
Thanks for the patch and your ideas on manipulating text using filters in twig. Good stuff.

I've been thinking about this more, though, and it is a bigger problem than just how we display the text in the block. Converting from the readable name to machine name and vice versa is not the best thing in the long term - it is not robust enough to cope with cases where the title and machine name are different, for example the D7 contrib module 'Chaos Tools' has a machine name of 'ctools'. This does not work with the current codebase. I think we need to store the readable module name in the node title and have a specific machine_name field which will permanently hold the machine name, for both core and contrib module nodes. In de-coupling the machine name from the title we make is safer for future development. This also simplifies several parts of the existing code as we no longer need to use cleanModuleName() at all. The modules are identified in the db tables by the machine name, which is safer than using the title text.

I have a working version of these changes and here is a patch. I have exported the content type config files using Configuration Update which has altered the indentation from the existing code (hence the config changes appear more complex than they are. I have also moved the fields from hidden into display, as it makes sense if an admin can edit the node then they should be able to fix the values).

Summary of the changes:

  1. "config: - field.field.node.module.field_machine_name" added to core.entity_form_display.node.module.default.yml and core.entity_view_display.node.module.default.yml
  2. Generator.php - 3 changes, replacing cleanModuleName() with field_machine_name
  3. in ModuleManager::createCoreModule() add extra parameter $title, and use this value for the node title. Store $name in the new field_machine_name
  4. In ModuleManager::getCoreModule() rename the parameter from $title to $name as it holds the machine name, and use this to retrive the readable title from the module list in config settings.
  5. Removed ModuleManager::cleanModuleName() as this is not required now.
  6. In ModuleManagerStorage::getCoreModule() rename the parameter from $title to $name, and change the condition to use field_machine_name
  7. In ModuleManagerTest.php remove the testing of cleanModuleName()
  8. Add new files field.field.node.module.field_machine_name.yml and field.storage.node.field_machine_name.yml

This all works with my local manual testing (including Chaos Tools/ctools) but I cannot run the automated tests, so do not know if they will pass yet.

Status: Needs review » Needs work

The last submitted patch, 17: 2823702-17.readable_name_and_machine_name.patch, failed testing.

jonathan1055’s picture

Status: Needs work » Needs review
StatusFileSize
new10.59 KB

Ha ha, that's funny. That single failure is due to the fact that the test I removed was the only test in that file! I did not notice that. I guess we need to add some more tests to ModuleManagerTest.php. I have added a dummy test so that we can keep the file, until we add more tests later.

quocnht’s picture

Great work @jonathan1055,

I apply patch, diff show also the default_config_hash. Is it needed in module config?

jonathan1055’s picture

Pleased you like the work. Always a slight worry when someone replaces the work of a previous patch.

Thanks for reminding me about config hash. I do not actually know what that relates to. Is is a hash-total of the specifc config export file, or of the whole entity? What is is used for? There were values in the files before, and when I exported via Config Update. it gave the new values, so I left them in. I did not have time to research that detail, but yes we need to understand its purpose and/or whether we can remove it.

bramdriesen’s picture

Status: Needs review » Reviewed & tested by the community

Looks okay! Worked on my local

jonathan1055’s picture

Thanks for testing, Bram. I have just checked, and the patch in #19 still applies OK after the recent commits.