-
-
Notifications
You must be signed in to change notification settings - Fork 825
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
CRM-21795 - Avoid fatal error to be displayed in log files. #11712
Conversation
CRM/Utils/System/Drupal.php
Outdated
@@ -853,10 +853,12 @@ public function synchronizeUsers() { | |||
* Similar to drupal_exit(). | |||
*/ | |||
public function onCiviExit() { | |||
if (!defined('MAINTENANCE_MODE') || MAINTENANCE_MODE != 'update') { | |||
module_invoke_all('exit'); | |||
if (function_exists('drupal_session_commit')) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is it the case that module_invoke_all would also not exist? Just wondering why we skip both parts of this if drupal_session_commit does not exist
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ie. if feels like separately checking for module_invoke_all makes sense or only checking for module_invoke_all as the first one seems slightly better than just checking for drupal_session_commit - as it stands I feel it needs at least a comment to explain why we skip module_invoke_all if drupal_session_commit doesn't exist
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The logged error was related to module_invoke_all
function itself. My thought was to check either of the function just to ensure CMS is bootstrapped which will avoid error to be recorded. Updated the above line to check for module_invoke_all
function for readability 👍.
seems fine to me - I think it's intuitive now that the first function implies the second function is not going to work wanna squash them? |
e7f0ae3
to
eb3277f
Compare
Done @eileenmcnaughton |
Ok - this appears safe, sensible & clean now - merge on pass |
This happens when users visit
Update: my bad, I had incorrectly applied the patch. Works fine! |
Overview
Fix fatal error - CRM/Utils/System/Drupal.php on line 857
Before
This doesn't occur when we navigate through the site but are recorded silently in the error log files. Maybe, CMS isn't bootstrapped and some function call happens at the backend.
After
Added a safety check to
onCiviExit()
function which avoids the fatal error to be recorded.