WSOD on admin/modules if description is set but is NULL in module.info.yml

Usually we add a test for something like this.

Try not to write a new test class. The test for the form you modified is Drupal\system\Tests\Form\ModulesListFormWebTest. Other places are Drupal\KernelTests\Core\Extension\ModuleInstallerTest or Drupal\system\Tests\Module\InstallUninstallTest. But in thinking about the patch in #2, it fixes this superficially (and I don't mean that negatively) in the UI, but if a module were enabled any other way, this would still be a problem. So in addition to a test I think we need a deeper fix. I think something at the level of module discovery would be better.

ThemeHandler::rebuildThemeData() has a way of merging defaults. I wonder if we can do something analogous for modules.

_system_rebuild_module_data() is supposed to do this but does not.

This is what I meant by "deeper" in #12. However, I now realize that #2 is probably more appropriate because the form function itself is calling t() on something that may be null and that is probably its problem.

In a case like this we are talking about valid YAML with description set, but empty, which means null. It seems other parts of Drupal don't have a problem with this. But notice that the defaults array in _system_rebuild_module_data() specifies that description is an empty string, assuming it was unset.

Great to see that there are already multiple approaches to solving this!

Reading through this issue and your patches, I was thinking along the following lines:

1. versantus linked to this page, where it states that a description is required
2. cilefen stated in #16, "the defaults array in _system_rebuild_module_data() specifies that description is an empty string, assuming it unset"
3. _system_rebuild_module_data() uses the InfoParser service to parse .yml files
4. which in turn make use of the function getRequiredKeys (core/lib/Drupal/Core/Extension/InfoParserDynamic)
5. however the function getRequiredKeys() doesn't define the description as a required key (which is the reason it is assumed an empty string in _system_rebuild_module_data() ?)

So, I'm wondering, does it make sense to add the description as a required key in getRequiredKeys()? Does that achieve that 'deeper fix' cilefen is looking for? And catches the error if the module is enabled in a non-GUI way?

@MF82: While we think about this, would you like to try writing the test? You can ping me on IRC for tips.

@cilefen: Yes, I would like to give it a try. I will first do some digging based on your pointers from #12. And, thank you!

I think a more generic solution for all .info.yml files where required keys are missing would be to throw a MissingRequiredInfoKeyException (or some such) which could be caught and properly displayed on the UI.

@almaudoh, I think that's a good suggestion generally. I posted your suggestion on #2558645: Malformed module.info.yml prevents install with a confusing error because I think that's the correct solution for required keys.

In this case though it's not a required key, but an incorrectly empty optional key. In #2594937: Empty 'libraries:' in theme .info.yml file produces confusing PHP warning, there is a case to support the key being present but empty for DX/TX reasons.

I also agree that it's non-ideal to go though and add isset()/!empty() checks all over the place for every individual, optional key.

So both modules and themes have a default array to merge to the information in .info.yml - see _system_rebuild_module_data() and \Drupal\Core\Extension\ThemeHandler::rebuildThemeData(). I think it is worth considering that if the yaml value is NULL we should use the default. That would fix both #2730807: WSOD on admin/modules if description is set but is NULL in module.info.yml and #2594937: Empty 'libraries:' in theme .info.yml file produces confusing PHP warning

So I guess it might be best to postpone this issue on that followup, which would also remove the then-redundant emptiness checks for individual keys?

Marking as postponed as suggested above. Also a test is still needed if this approach is eventually pursued.

Closed #3243208: Empty description in a custom module info.yml leads to WSOD as a duplicate, Adding credit.

This issue is still present in 9.4.x. But there's been some serious refactoring in core since the last patch here.For example, _system_rebuild_module_data no longer exists.

Following the pattern of the related issue, which adds a little check to ThemeHandler::addTheme(), we could add a check to ModuleHandler::addModule() that does something special when the description is null.

However, the same WSOD occurs on the Appearance page if the description of a Theme is set to null. If we wanted to fix both of these at the same time I would be in favor of that. (edit: here's an issue for the appearances WSOD: #3213079: When creating a theme, if you leave the description field empty in the themename.info.yml file, it throws a white screen error)

In a comment on the related issue a follow-up was eluded to, but I don't think it was ever made. Comment #24 here even refers to "that followup", but I don't see any evidence that followup was created. From an issue queue standpoint, how should we proceed? Do we adjust this one to act as the followup? Do we close this and create a new issue from scratch?

From a code standpoint, we have some nice options.

We could add a check to Drupal\Core\Extension\InfoParserDynamic::parse(), which is used for parsing module, theme, and profile .info files.

But I think I like @alexpott's idea in #23 a little better though, which is to leverage the default values of the ExtensionList classes better. We could alter ExtensionList::createExtensionInfo() to be more clever with the default values. The current code is below. It adds default values if the keys are missing, but it won't overwrite a value of null.

  protected function createExtensionInfo(Extension $extension) {
    $info = $this->infoParser->parse($extension->getPathname());

    // Add the info file modification time, so it becomes available for
    // contributed extensions to use for ordering extension lists.
    $info['mtime'] = $extension->getMTime();

    // Merge extension type-specific defaults.
    $info += $this->defaults;

    return $info;
  }

There could be some simple code before the merge to unset any keys where the value is null. Then the default values would get used in those cases.

Update: But there's even another issue I found that wants to handle null values in TranslatableMarkup: #2719553: Log error when a TranslatableMarkup is created with a non-string input, which would make this issue moot.

This seems to have gotten fixed in the intervening years, this test should be green

Ah wait, not fixed, it has to be NULL to fail, see test

Ah wait, not fixed, it has to be NULL to fail, see test

Here is a fix that implements #23, no interdiff since its a different approach than previously

If we feel this is a good fix, we can close #3213079: When creating a theme, if you leave the description field empty in the themename.info.yml file, it throws a white screen error too

So the IS points to #21 which is to throw a MissingRequiredInfoKeyException but is it now just to display a message?

We can't/shouldn't throw an exception for description as if it is required, because it's not required. So the solution is now to make sure the defaults are used if something is set to NULL. I removed #21 from the IS.

Gotcha.

Verified the problem described in the IS
Edited the action module description to be description:
Got a fatal error
Applied patch #45 and no issue now.

Committed/pushed to 11.x and cherry-picked to 10.1.x, thanks!

Thanks! I forgot to tag with ddd23, fixing that now.