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 offield_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
andhelp
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
Comment | File | Size | Author |
---|---|---|---|
#14 | field_ui_help_should-2354107-13.patch | 7.35 KB | mpdonadio |
#10 | interdiff.txt | 2.71 KB | mpdonadio |
#10 | field_ui_help_should-2354107-10.patch | 8.89 KB | mpdonadio |
#8 | interdiff.txt | 2.69 KB | mpdonadio |
#8 | field_ui_help_should-2354107-8.patch | 8.88 KB | mpdonadio |
Comments
Comment #1
swentel CreditAttribution: swentel commented#2350997: Help module install error when Taxonomy is disabled because of Field UI got in for taxonomy.
Wouldn't call this critical though.
Comment #2
tstoecklerOh, 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 todrush en node
, but that meansComment #3
tstoecklerComment #5
swentel CreditAttribution: swentel commentedRight, I was thinking about it a bit more, and yeah, you can't recover, so this is critical :)
Comment #6
mpdonadioI 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.
Comment #7
amateescu CreditAttribution: amateescu commentedThis change doesn't look right, the "defined at the" part is missing :/
Comment #8
mpdonadioFixed the help text.
Comment #9
amateescu CreditAttribution: amateescu commentedSorry but it's still missing "at the" ...
Comment #10
mpdonadioGrrr. Bad copy/paste from my wrapped version. I even proofread that several times and didn't catch it.
Comment #11
amateescu CreditAttribution: amateescu commentedLooks great now.
Comment #12
alexpottI 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.
Comment #13
tstoecklerWell we can drop the assertNoText(), but we should definitely keep the drupalGet() to make sure that no fatal is thrown.
Comment #14
mpdonadioPer IRC conversation with @alexpott, removing entire FieldUiHelpTest from the patch.
Comment #15
amateescu CreditAttribution: amateescu commentedCan't argue with @alexpott :P
Comment #16
alexpottWell 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!
Comment #18
amateescu CreditAttribution: amateescu commentedI was just kidding :)