dev/core#849 Replace get_headers functions call with Guzzle HTTP request.

[agilewarealok]

Base PR: #12938 (New implementation replaces cURL with Guzzle)

Overview

The existing get_headers function call has no explicit timeout and will execute until PHP timeout is reached. This can be problematic in some hosting environments where a PHP request can block further PHP execution - preventing the website from loading at all. A blocking request will eventually time out, causing a HTTP 504 error for the website and all visitors.

This problem most commonly occurs where the internal request is to a resource which cannot be resolved, usually relating to local DNS issue but can also be due to front-end proxy services.

Before

CiviCRM's internal request will execute until PHP timeout is reached and cause HTTP 504 error.

After

CiviCRM uses the Guzzle library to perform the request and now has a timeout of 250ms which will be sufficient for those requests which have no issues and in the event that the request cannot be resolved, will cancel execution earlier than the PHP timeout.

Comments

Agileware Ref: CIVICRM-1163

[civibot]

(Standard links)

• If this is your first pull-request for CiviCRM, please browse CONTRIBUTING.md for information about the development and testing processes.
• If you are reviewing this pull-request, you may wish to consult the test sites and the Review Standards (long template, short template).

[eileenmcnaughton]

@agilewarealok jenkins is upset on whitespace

[eileenmcnaughton]

This looks good to me @totten @seamuslee001 are you ok with 250ms as a default timeout?

[seamuslee001]

I'm fine with it tbh

Seamus

[eileenmcnaughton]

@mlutfy @nganivet should also ping you in on the default timeout - I'm inclined to think that it's fine - the pings should get through sometimes & less sites will have them out like I have done.

[eileenmcnaughton]

Nice!

[eileenmcnaughton]

Adding merge on pass - we can tweak the timeout in the rc pending feedback above but this is definitely a good step & with the test I'm keen to get it merged

[eileenmcnaughton]

Hmm test fails are due to timeouts - @agilewarealok - perhaps try with 500 ms & see if it gets through tests - we can iterate once the first step is merged

[agilewarealok]

@eileenmcnaughton Updated the timeout.

[mattwire]

@agilewarealok Thanks for following up and submitting this PR!

[eileenmcnaughton]

@agilewarealok nice. I suspect we should move the guts of the fileExists function to somewhere more centralised at some point - either CRM_Utils_File or CRM_System_Resource unless @totten has a better suggestion.

We had a LOT of pain around checks etc & eventually found the communityUrl was a painpoint after I 'fixed' (hacked out) all the checks. Fortunately that one can be turned off using a setting but it took me a while

[jusfreeman]

We had a LOT of pain around checks etc & eventually found the communityUrl was a painpoint after I 'fixed' (hacked out) all the checks. Fortunately that one can be turned off using a setting but it took me a while

@eileenmcnaughton this sounds like another one to fix, any more details?

[eileenmcnaughton]

@jusfreeman so from civicrm.settings.php we have

 // Disable display of Community Messages on home dashboard
 // $civicrm_setting['CiviCRM Preferences']['communityMessagesUrl'] = false;

We have a site behind a firewall and setting this to false makes use a lot happier

I looks like the class was written to accept a factory client but that is likely immature

class is
CRM_Core_CommunityMessages