dev/core#2340 Skip rather than fail on bad menu item #19464
Conversation
|
(Standard links)
|
CRM/Core/Menu.php
Outdated
| } | ||
| } | ||
| catch (CRM_Core_Exception $e) { | ||
| CRM_Core_Session::setStatus($e->getMessage(), ts('Path skipped')); |
There was a problem hiding this comment.
In general I think CRM_Core_Session::setStatus() is only meant for use on forms, and lower-down BAO and Util classes should not call it. Consider the case of an API calling a BAO function and that BAO function calling CRM_Core_Session::setStatus() to handle errors. In that case the api might be running over REST or other headless state so there isn't even a user session to set messages for.
There was a problem hiding this comment.
yeah - but then it just gets swallowed doesn't it? Which seems OK - outside forms.
There was a problem hiding this comment.
Well it's led to some weird bugs in the past. Typically we expect the BAO to throw an exception and the layer that calls it to catch it and respond appropriately (if it's an api, generate an api error, if it's a form, display a status message, if it's somewhere else, maybe log it?).
There was a problem hiding this comment.
The problem is that the iterate-through-array-of-menu-items is still at the BAO layer - we want that layer to continue. I guess it could throw an exception at the end of the iteration - but then it might still kill api calls
There was a problem hiding this comment.
@eileenmcnaughton I recommend logging the error instead of calling setStatus()
There was a problem hiding this comment.
OK - I've updated it
There was a problem hiding this comment.
I don't know everything about logging in Civi but wouldn't this be classified as an error message rather than a debug message? e.g. Civi::log()->error()
There was a problem hiding this comment.
@colemanw done - can we merge now?
eileenmcnaughton
force-pushed
the
menu
branch
from
aa9ee2b to
92c0a44
Compare
eileenmcnaughton
force-pushed
the
menu
branch
from
92c0a44 to
bc537a0
Compare
Overview
Reduces the severity of having an invalid menu item by displaying an error rather rather tha not loading the page
Before
After configuring an afform with a path of 'trxn' (instead of 'civicrm/trxn') the api explorer, the main dashboard and various other pages faile to load with an uncaught exception
After
Technical Details
Comments