Add CRM_Utils_System::sendResponse(). Fix AssetBuilder's status-code on WP.

[seamuslee001]

…r_system based method to set HTTP Status code

Overview

This aims to ensure that there is a consistent response returned between Drupal/Backdrop/WordPress in regards to Asset Builder URLs, at the moment when a URL is not found Drupal/Backdrop return a 404 status which Guzzle can pick up but WordPress returns 200 because of internal WordPress 404 handling, this fixes it so that WordPress will also return status of 404 if we want it to

Before

Different Responses sent and E2E Unit Tests fail on WordPress

After

Consistent passing on all 3 CMSes

ping @totten @eileenmcnaughton @kcristiano

[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).

[seamuslee001]

I altered the E2E-Matrix test in Jenkins to apply this as a patch when creating the builds and then did a manual run just against the Master Branch and got all green https://test.civicrm.org/job/CiviCRM-E2E-Matrix/333/

[eileenmcnaughton]

if the above is php 5.4+ what is this one?

[totten]

1. Right, so this is capturing the main goal of extracting that logic and creating a CRM_Utils_System::sendResponse(...) function.

2. I think this is too conservative in refactoring-technique -- i.e. it uses (by happenstance) the "HTTP-response data-model" from AssetBuilder rather than an "HTTP-response data-model" that's designed for general HTTP expectations. In particular, note that it supports one setting for one HTTP header (mimeType / Content-Type) -- but HTTP end-points may need to send many other headers. There are also edge-casey bits in handling the HTTP data (like distinguishing the status-code and the status-reason; like case-sensitivity in header names) which we haven't specified. PSR-7 is not perfect, but it does address the edgy bits, and it's standard enough that we may eventually get some leverage out of it.

   Consider this signature:

   function sendResponse(\Psr\Http\Message\ResponseInterface $response)

   and usage:

   CRM_Utils_System::sendResponse(
     new \GuzzleHttp\Psr7\Response(200, ['Content-Type' => 'text/csv'], "ab,cd\n12,34\n")
   );

3. IMHO, the entire implementation of the sendResponse() function should live in CRM_Utils_System_Base so that it can be overriden on a per-CMS basis.

4. As long as the interface is based on PSR-7, I don't think there's much risk that we're going to be changing the signature/inputs.

[seamuslee001]

@totten i have updated now as per your sample code and i think it looks pretty sensible now

[eileenmcnaughton]

@seamuslee001 jenkins wants a word with you

[seamuslee001]

@eileenmcnaughton should be sorted now also ping again @totten

[eileenmcnaughton]

Can we at least have a comment why we need this - if we are keeping support for php < 5.4 to reduce the riskiness of the PR can we flag that?

[totten]

Yeah, I think it'd be fine to just use http_response_code() and remove the conditional.

However, that's a minor cleanup - doesn't need to block this.

[eileenmcnaughton]

typo

[eileenmcnaughton]

Since you need to fix this comment block it's worth mentioning that the phpdoc std is a blank line after the first 'short sentence describing the function'

[eileenmcnaughton]

Looks good to me - I made a couple of stylistic comments - hopefully @totten will give it another once over

[seamuslee001]

I did another manual run of the e2e-matrix with this patch applied and it passed for all distros we are testing https://test.civicrm.org/job/CiviCRM-E2E-Matrix/339/

[totten]

For WP, isn't status_header() sufficient? (Guess the E2E tests would tell!) The http_response_code() and header() formulations feel redundant.

[totten]

Hrm, shouldn't this be more like:

foreach ($message->getHeaders() as $name => $values) {
  foreach ($values as $value) {
    CRM_Utils_System::setHttpHeader($name, $value);
   }
}

// or

foreach ($response->getHeaders() as $name => $values) {
    CRM_Utils_System::setHttpHeader($name, implode(', ', $values));
}

(Riffing on the docblock for MessageInterface::getHeaders() -- but I'm not sure which formulation is better here.)

[seamuslee001]

@totten have updated now

[seamuslee001]

Confirmed on the latest patches it ran successfully on E2E https://test.civicrm.org/job/CiviCRM-E2E-Matrix/345/

[totten]

Huzza! Looking good.

I also did some r-run (click-around testing on pages that rely on dynamic assets and curl to check some of the HTTP headers on typical request) and executed the E2E test locally (D7 and WP). All looking good.

Added a small cleanup.

(CiviCRM Review Template WORD-1.2)

• General standards
  • (r-explain) Needs work: This is both fixing a bug and exposing an improved API for sending AJAXy/non-HTML responses. We should communicate that.
  • (r-user) Pass: No UI impact
  • (r-doc) Pass
  • (r-run) Pass: Diid some click-around testing on pages that rely on dynamic assets. Used curl to fetch some of those assets and verify HTTP header comes through.
• Developer standards
  • (r-tech) Pass
  • (r-code) Pass
  • (r-maint) Pass: Already has implicit test-coverage.
  • (r-test) Pending: New test-run is active. The relevant E2E_Core_AssetBuilderTests passes on my local D7+WP.

Merge on pass.