Send an 500 http response code when an exception is encountered.

[michaelmcandrew]

Overview

When we exit with a non zero exit code, we should generate an appropriate http repsonse code. 500 internal server error seems most appropriate.

This is useful, for example when callbacks are trying to communicate with CiviCRM.

Lets say an exception occurs. If we send a 200 along with this exception, the 3rd party will have no idea something went wrong. If we send a 500, they will know something is up and be able to take appropriate action (try again later, alert the admin, etc.)

Before

We always exited with 200.

After

When CRM_Utils_System is called with a status > 0, we exit with 500.

I grepped for CRM_Utils_System::civiExit and saw that a couple of calls to ::civiExit were passing 'error'. I have switched these to 1.

[seamuslee001]

@michaelmcandrew can you fix the style error? https://test.civicrm.org/job/CiviCRM-Core-PR/19650/checkstyleResult/new/

[michaelmcandrew]

@seamuslee001 - yeah done

[mfb]

This issue hits me all the time!

Side note: Aside from the 200 status being wrong, printing any output is sometimes the wrong thing, e.g. when CiviCRM is called in a command-line app or JSON output is being generated.

Would it be possible for CiviCRM to just throw an exception? That should work with e.g. Drupal's exception handler, but maybe doesn't work elsewhere?

[michaelmcandrew]

@mfb - can you give an example of where it is outputting stuff that you aren't expecting? My guess is that they would be need to be fixed on a one by one basis, but maybe I am not understanding you.

[mfb]

@michaelmcandrew calls to CRM_Core_Error::fatal() are what I'm talking about, and I see it's already being phased out :)

• https://issues.civicrm.org/jira/browse/CRM-20181 Deprecate CRM_Core_Error::fatal()
• https://issues.civicrm.org/jira/browse/CRM-20185 Eliminate calls to CRM_Core_Error::fatal()

[eileenmcnaughton]

test this please

[eileenmcnaughton]

@totten would like you to eyeball this but think it might be an easy merge

[eileenmcnaughton]

test this please

[seamuslee001]

@michaelmcandrew Michael i support the idea tho we either need to decide if to change to use another number other than 1 or change these places to give off 200 status code or similar.

civicrm-core/CRM/Contact/Form/Task/Label.php

Line 354 in dbf3ef2

CRM_Utils_System::civiExit(1);

civicrm-core/CRM/Member/Form/Task/Label.php

Line 145 in dbf3ef2

CRM_Utils_System::civiExit(1);

civicrm-core/CRM/Utils/PDF/Utils.php

Line 192 in dbf3ef2

CRM_Utils_System::civiExit(1);

civicrm-core/CRM/Badge/BAO/Badge.php

Line 76 in dbf3ef2

CRM_Utils_System::civiExit(1);

[eileenmcnaughton]

Good spotting @seamuslee001 - I'd be inclined to pass in 500 & check for 500 as that is the actual status code. But 500 doesn't work with exit()

From what I can tell we don't have any meaningful pattern to $status here

exit($status);

it seems more like something someone thought was a good idea once?

[michaelmcandrew]

Hey @eileenmcnaughton and @seamuslee001,

From what I can tell we don't have any meaningful pattern to $status here

Apart from 0 is non error and > 0 is error.

it seems more like something someone thought was a good idea once?

When I was looking around, I found the following https://github.com/michaelmcandrew/civicrm-core/blob/master/CRM/Core/Error.php#L77-L80 which looks like a start at such an idea, though it was not very implemented.

I did consider using http codes but decided against it for a couple of reasons:

• exiting with non zero for errors is a reasonable standard
• not all processes are in the context of http (though admittedly my patch does presume that)
• the error codes were there and I didn't want to touch too much

Up for being persuaded but just wanted to put these arguments out there.

[michaelmcandrew]

@mfb - ok cool. so I will presume no further action for your comments on this issue (unless I hear otherwise).

[mfb]

I'm not sure about the answer to my question

Would it be possible for CiviCRM to just throw an exception? That should work with e.g. Drupal's exception handler, but maybe doesn't work elsewhere?

i.e. could CiviCRM simply throw an exception and rely on the CMS exception handler to output the 500 status code. If CiviCRM is not running under a CMS (e.g. executing via its own front controller, standalone CiviCRM, etc.) then it would need its own exception handler to do so.

[michaelmcandrew]

@mfb I am not sure either :)

[eileenmcnaughton]

@michaelmcandrew I took a look and found some places in the code where this would be explictly the wrong thing to do - ie.
CRM_Contact_Form_Task_Label & a few other places that pass the 1 when they have succeeded in outputting a pdf

So, I don't think we can do this with the existing status field without risk. I think I would be OK with adding a second parameter to the function - e.g http error code or $isHttpLevelError which would default to null. Then separately & very specifically we can alter calls to the function which need to return a 500 error due to the problems described above

[tttp]

We should as well finally specify what this status values are meant to be, because

   * Exit with provided exit code.
   *
   * @param int $status
   *   (optional) Code with which to exit.
   */
   public static function civiExit($status = 0)

For me, it was clear we were following the *nix C "By convention, a status of 0 means normal termination any other value indicates an error or unusual occurrence."

But CRM_Contact_Form_Task_Label and presumably others are doing it differently, so let's make it explicit that 0 means success and everything else is an error.

Or whatever convention you prefer... as long as it's an explicit one ;)

I'm not a big fan of adding another status "just" because the first one is not clearly specified, I'd rather follow @eileenmcnaughton suggestion of putting 500, and adding a "if status_id >= 255 exit(1)"

[eileenmcnaughton]

yeah - Iike the idea of only changing this to handle is_numeric($status) && $status> 255 differently and not touching existing calls. Code doc would need updating to say what should be passed in

[tttp]

@eileenmcnaughton I was suggesting >= 255 instead of > 255 because php on exit()

...but I'm bikeshedding, I'm sure enough that 255 is never used as a status in the first place.

http://php.net/manual/en/function.exit.php

If status is an integer, that value will be used as the exit status and not printed. Exit statuses should be in the range 0 to 254, the exit status 255 is reserved by PHP and shall not be used. The status 0 is used to terminate the program successfully.

[eileenmcnaughton]

@michaelmcandrew ^^

[michaelmcandrew]

My vote is to fix the status to follow one convention rather than add another parameter that could be called with paradoxical parameters, i.e.

1. decide on the convention for civiExit() - lets just use the same as exit()
2. update the code that is calling civiExit() on success with $status > 0 to $status = 0
3. Keep the CiviCRM exit code and setting the http status seperate like this.

[eileenmcnaughton]

so as pre-cleanup change all instances of

CRM_Utils_System::civiExit(1); 

to

CRM_Utils_System::civiExit(); 

because we agree they currently are not achieving anything?

[davejenx]

I agree with @michaelmcandrew's proposal of 30th July: seems to me the calls to:
CRM_Utils_System::civiExit(1);
in civicrm-core/CRM/Contact/Form/Task/Label.php etc are passing an incorrect exit code. I say incorrect because I share the assumption/prejudice that this should act like a *NIX exit code, so 0 should be passed on success.

[eileenmcnaughton]

@davejenx now I've merged @seamuslee001 tidy up do you think this is mergeable

[davejenx]

@eileenmcnaughton @michaelmcandrew In CRM/Utils/System.php I spotted that redirect() calls self::civiExit(). In that case, should we be returning a 301 or 302 HTTP response code? That got me thinking that if so, maybe civiExit needs another parameter to specify response code, or some sort of convention for passing an HTTP response code via the status parameter, along the lines discussed above. If it's just for the redirect() case though, the easiest way would be to add a http_response_code(301) or 302 in redirect() before civiExit(). Can people think of other cases where we may want to exit with a code other than 200 or 500, that might justify handling this flexibly in civiExit() ?

[davejenx]

I've tested on URL /civicrm/ajax/contactref , which calls CRM_Contact_Page_AJAX::contactReference, which now calls CRM_Utils_System::civiExit(1) in conditions such as:

• custom field id missing or field inactive or wrong type or filter invalid
• API Contact get returns is_error = 1
  Tested with missing id.
  Without PR: HTTP response code = 200
  With PR: HTTP response code = 500
  As expected.

I think you could argue the toss about what response code is most appropriate for particular error conditions. But if we're happy that a choice of 200 or 500 covers our needs for cases where we're calling civiExit(), with the option of calling http_response_code() before civiExit(0) if we want something different, then I think this PR is good.

[eileenmcnaughton]

Ok let's now agonise any more over it :-) It feels agreed

[xurizaemon]

Seems like this may have had an unintended consequence with PayPal, which Stoob reported in chat as sending a lot of notifications about unprocessed IPNs (CiviCRM civicalls CiviExit() from IPN, eg if it gets an IPN for a recurring payment which last I looked CiviCRM does not implement IPN handling for). See also CiviCRM.SE: anyone getting “IPN warning” emails from PayPal?

I don't know how PayPal handles response codes, but I can see why they might retry if they received a generic 500 response code, and if they consistently received 500s they might assume the site is too broken to talk to and give up IPN entirely.

After receiving the IPN message from PayPal, your listener returns an empty HTTP 200 response to PayPal. Otherwise, PayPal resends the IPN message. - https://developer.paypal.com/docs/classic/ipn/integration-guide/IPNImplementation/ 🤢

Anyway, posting this here so Relevant People see that discussion!

[mattwire]

I've recently gone through a similar thing with the stripe extension (not because core broke it but because the IPN needed refactoring). It now explicitly specifies http response codes as appropriate, eg. https://lab.civicrm.org/extensions/stripe/blob/master/CRM/Core/Payment/StripeIPN.php#L103