Problem/Motivation

Field UI does not perform a ModuleHandlerInterface::moduleExists() check when linking to the following route:

  • node.overview_types

If Node module is not installed, this results in a fatal error.

Because this happens everywhere hook_help() is invoked it renders the entire admininstrative section of Drupal unusable.

Proposed resolution

  • Add a $module_handler = \Drupal::moduleHandler(); local variable to the top of field_ui_help()
  • Wrap the Fields on content items and Fields on comments section in if ($module_handler->moduleExists('node'))
  • Write a test that only enabled field_ui and help modules and visits e.g. /admin/help

Note that the Fields on comments section is outdated since comments are no longer node-specific. So for extra credit that section could be updated to reference the correct URL. That would then require adding a if ($module_handler->moduleExists('comment')) check instead of the if ($module_handler->moduleExists('node')) check.

Remaining tasks

User interface changes

API changes

CommentFileSizeAuthor
#14 field_ui_help_should-2354107-13.patch7.35 KBmpdonadio
#10 interdiff.txt2.71 KBmpdonadio
#10 field_ui_help_should-2354107-10.patch8.89 KBmpdonadio
#8 interdiff.txt2.69 KBmpdonadio
#8 field_ui_help_should-2354107-8.patch8.88 KBmpdonadio
#6 2354107-6.patch8.88 KBmpdonadio
#6 interdiff-02-06.txt8.21 KBmpdonadio
#3 2354107-2.patch1.09 KBtstoeckler

Comments

swentel’s picture

swentel commented
Priority: Critical » Major

#2350997: Help module install error when Taxonomy is disabled because of Field UI got in for taxonomy.

Wouldn't call this critical though.

tstoeckler’s picture

tstoeckler
he/him
Primary language German
Location Essen, Germany
commented
Issue summary: View changes
Issue tags: -Needs tests

Oh, I missed that taxonomy issue. Thanks.

But really, triggering fatals from the UI is no longer critical these days? Don't want to play status ping-pong but to me that's a interesting definition of critical, to say the least :-P.

Here's a test to prove what I mean. It's not really hard to trigger it and it's completely impossible to recover, as /admin/modules throws the same fatal. You need to drush en node, but that means

  1. That you have Drush
  2. That you have figured out WTH is going on
tstoeckler’s picture

tstoeckler
he/him
Primary language German
Location Essen, Germany
commented
Status: Active » Needs review
StatusFileSize
new 2354107-2.patch1.09 KB

Status: Needs review » Needs work

The last submitted patch, 3: 2354107-2.patch, failed testing.

swentel’s picture

swentel commented
Priority: Major » Critical

Right, I was thinking about it a bit more, and yeah, you can't recover, so this is critical :)

mpdonadio’s picture

mpdonadio
he/him
Primary language English
Location Philadelphia/PA/USA (UTC-5)
commented
Status: Needs work » Needs review
StatusFileSize
new interdiff-02-06.txt8.21 KB
new 2354107-6.patch8.88 KB

I implemented the proposed resolution, and updated the comment help.

I also updated the new test to cover the three module checks.

It was unrelated to the issue, but I fixed the HTML so it is a proper definition list.

amateescu’s picture

amateescu commented
Status: Needs review » Needs work 
+++ b/core/modules/field_ui/field_ui.module
@@ -39,21 +41,25 @@ function field_ui_help($route_name, RouteMatchInterface $route_match) {
-      $output .= '<dd>' . t('Fields on comments are defined at the content-type level, on the <em>Comment fields</em> tab of the content type edit page (which you can reach from the <a href="@types">Content types page</a>). When you add a field for comments, each comment on a content item of that type will have that field added to it. For example, you could add a website field to the comments on forum posts, to allow forum commenters to add a link to their website.', array('@types' => \Drupal::url('node.overview_types'))) . '</dd>';
...
+        $output .= '<dd>' . t('Fields on comments are comment entity level, on the <em>Manage fields</em> tab of the comment types edit page (which you can reach from the <a href="@types">Comment types page</a>). When you add a field for comments, each comment on an entity with that comment type will have that field added to it. For example, you could add a website field to the comments on forum posts, to allow forum commenters to add a link to their website.', array('@types' => \Drupal::url('comment.type_list'))) . '</dd>';

This change doesn't look right, the "defined at the" part is missing :/

mpdonadio’s picture

mpdonadio
he/him
Primary language English
Location Philadelphia/PA/USA (UTC-5)
commented
Status: Needs work » Needs review
StatusFileSize
new field_ui_help_should-2354107-8.patch8.88 KB
new interdiff.txt2.69 KB

Fixed the help text.

amateescu’s picture

amateescu commented

Sorry but it's still missing "at the" ...

mpdonadio’s picture

mpdonadio
he/him
Primary language English
Location Philadelphia/PA/USA (UTC-5)
commented
StatusFileSize
new field_ui_help_should-2354107-10.patch8.89 KB
new interdiff.txt2.71 KB

Grrr. Bad copy/paste from my wrapped version. I even proofread that several times and didn't catch it.

amateescu’s picture

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

Looks great now.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Reviewed & tested by the community » Needs work

I think we should drop the test - asserting for things that aren't there is extremely susceptible to false positives. Plus other help like this is not tested. Also #2351991: Add a config entity for a configurable, topic-based help system is likely to inverted the dependency and have the modules attached there help information to field ui's.

tstoeckler’s picture

tstoeckler
he/him
Primary language German
Location Essen, Germany
commented

Well we can drop the assertNoText(), but we should definitely keep the drupalGet() to make sure that no fatal is thrown.

mpdonadio’s picture

mpdonadio
he/him
Primary language English
Location Philadelphia/PA/USA (UTC-5)
commented
Status: Needs work » Needs review
StatusFileSize
new field_ui_help_should-2354107-13.patch7.35 KB

Per IRC conversation with @alexpott, removing entire FieldUiHelpTest from the patch.

amateescu’s picture

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

Can't argue with @alexpott :P

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Reviewed & tested by the community » Fixed

Well you can argue with me - and I tend to listen. In this case though we have lots of hook_help implementations that have module existence checks in them and none of them are tested. And we have #2351991: Add a config entity for a configurable, topic-based help system which might change how these are managed.

Committed 2730f9b and pushed to 8.0.x. Thanks!

  • alexpott committed 2730f9b on 8.0.x 
    Issue #2354107 by mpdonadio, tstoeckler: Fixed field_ui_help() should...
amateescu’s picture

amateescu commented

I was just kidding :)

Status: Fixed » Closed (fixed)

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