search

backdrop / backdrop-issues

Issue tracker for Backdrop core.
144 stars 39 forks source link

Notice: Undefined index: markup in theme_help() (line 2374 and 2375 of theme.inc) #2590

Open klonos opened 7 years ago

klonos commented 7 years ago

While testing on the PR sandbox from https://github.com/backdrop/backdrop/pull/1802#issuecomment-286274376 I get these two messages when trying to run a missing db update for user.module ("1013 - Grant accounts with 'administer permissions' the new 'assign roles' permission."):

Notice: Undefined index: markup in theme_help() (line 2374 of /home/qa/github/backdrop/backdrop/pr/1802/core/includes/theme.inc).
Notice: Undefined index: markup in theme_help() (line 2375 of /home/qa/github/backdrop/backdrop/pr/1802/core/includes/theme.inc).
ghost commented 5 years ago

PR available (untested).

klonos commented 5 years ago

Thanks @BWPanda 👍 ..in theory, your PR should work, but I have no way to reproduce the problem currently, so cannot confirm 🤷‍♂

jenlampton commented 4 years ago

@klonos have you seen this with any of the recent db updates?

ghost commented 2 years ago

New, updated PR: https://github.com/backdrop/backdrop/pull/3949

klonos commented 2 years ago

Thanks @BWPanda 🙏🏼 ...the code changes in the PR make absolute sense to me. It is basically adding some defensive coding.

One thought I had is should we add an else there, and add a watchdog message if 'markup' is not actually set (for better DX)? The way it is now, if there is no 'markup' nothing will be printed, and no indication.

jenlampton commented 2 years ago

If markup is required for theme_help() to do anything, it would be better to find out what is calling theme_help() without the #markup key, and fix that, because without also fixing that, whatever code was doing that will remain broken.

edit: Hold on... @klonos this PR was filed in 2017 --- wasn't that long before we added the new theme_help() into backdrop core?

indigoxela commented 2 years ago

Personally, I find the PR a bit confusing.

The official docs state that #markup is necessary to let this API item do anything useful, so the isset seems redundant.

it would be better to find out what is calling theme_help() without the #markup key, and fix that

I totally agree with that. The "undefined index" happens because of some faulty usage of that FAPI type.

Checking for a <p> before wrapping seems useful, but that's not actually what this issue addresses - regarding the issue title. At least, we should clarify the issue title and description. If these nested <p> really happen, we should clarify that the #markup should only contain inline tags, no? Or simply not wrap in <p>?

ghost commented 2 years ago

Since this is 'cannot reproduce', I've closed my PR and unassigned myself. I'd love to close this issue as well, but will let someone else do that.