Skip to content
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

Fix PHP Warnings. Replace fatal with statusBounce. Mark appendBreadCr… #10888

Merged

Conversation

mattwire
Copy link
Contributor

…umbs parameter as array instead of string so editors don't give a warning

Overview

Fix some PHP Warnings - NFC.
Replace fatal with statusBounce (as the form doesn't redirect when exporting a second click on the export link causes a fatal error). With this change it redirects and shows a helpful error message.
Mark CRM_Utils_System appendBreadCrumbs parameter as array instead of string so editors don't give a warning.

@@ -97,7 +97,9 @@ public function preProcess() {

foreach ($batchStatus as $batchStatusId) {
if ($batchStatusId == $this->_exportStatusId) {
CRM_Core_Error::fatal(ts('You cannot exported the batches which were exported earlier.'));
//CRM_Core_Session::singleton()->user
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mattwire any reason for leaving this commented out line here

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No.. :-)

…umbs parameter as array instead of string so editors don't give a warning
@mattwire mattwire force-pushed the CRM-21092_financial_batch_fixes branch from 04e7228 to 0efdabe Compare August 22, 2017 20:32
@@ -97,7 +97,8 @@ public function preProcess() {

foreach ($batchStatus as $batchStatusId) {
if ($batchStatusId == $this->_exportStatusId) {
CRM_Core_Error::fatal(ts('You cannot exported the batches which were exported earlier.'));
$url = CRM_Core_Session::singleton()->readUserContext();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think you need this $url bit here - check the start of the statusBounce fn

  public static function statusBounce($status, $redirect = NULL, $title = NULL) {
    $session = CRM_Core_Session::singleton();
    if (!$redirect) {
      $redirect = $session->readUserContext();
    }

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's what I thought... but it doesn't work properly without as the user context is not set otherwise in this case.

@eileenmcnaughton eileenmcnaughton added the merge ready PR will be merged after a few days if there are no objections label Aug 23, 2017
@eileenmcnaughton
Copy link
Contributor

I think this is a good improvement. I have added merge-ready flag rather than merged it to give you a chance to respond to my comment. However, I don't think merging this is conditional on you making any further changes

@mattwire
Copy link
Contributor Author

@eileenmcnaughton Thanks for reviewing. See response to comment :-)

@eileenmcnaughton eileenmcnaughton merged commit 1f0a86b into civicrm:master Aug 28, 2017
@eileenmcnaughton
Copy link
Contributor

ok - I won't try to understand why then!

@mattwire mattwire deleted the CRM-21092_financial_batch_fixes branch September 5, 2017 14:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merge ready PR will be merged after a few days if there are no objections
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants