dev/core#2369 - Change missing image fatal error to 404

[magnolia61]

Overview

A missing profile image results in a unnecessary fatal error

Before

Use a profile in order for users to be able to upload a profile image (image url field in contact table). Viewing the field while an image is missing results in a fatal error (user does not notice, it is only logged that way). Being a fatal error the report error extension would also notify us by email each time.

After

A missing image url field does not result in a fatal error being logged anymore. The regular 404 rules apply.

Technical Details

Removed the lines that would throw the error

Comments

The world will be a better place after this commit is merged

Issue can be found over here: https://lab.civicrm.org/dev/core/-/issues/2369

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

[demeritcowboy]

There was some talk in the ticket of making it a 404. Right now it has response code 200. If it is desired to make it 404, which seems reasonable, then in the else clause you could do CRM_Utils_System::sendResponse(new \GuzzleHttp\Psr7\Response(404));

[magnolia61]

I had no clue how this would work @demeritcowboy Thanks for your suggestion.

When I search the core code I see these examples of throwing a 404
header("HTTP/1.0 404 Not Found");
and
CRM_Utils_System::setHttpHeader("Status", "404 Not Found");

Would the guzzle way be preferable you think?

[demeritcowboy]

Good point. Looks like they all do the same thing. And it seems even when the header/code is missing either php or the browser figures it out and adds it if the other is present. So maybe header("HTTP/1.0 404 Not Found"); is best since then it's consistent with the rest of that file.

[magnolia61]

@demeritcowboy changed the PR to the header 404 version. Would you be able to test, review and approve?

[demeritcowboy]

Ok cool. I don't think the return does anything since the function just returns right after anyway. But otherwise looks good and tests ok.

[eileenmcnaughton]

tests have hung?

[eileenmcnaughton]

test this please

[seamuslee001]

I would say rather than doing a return here we should call CRM_Utils_System::civiExit() but this could also be a follow up to this PR.

[magnolia61]

Why would you suggest this a follow up? Or would more returns in civicrm code be candidates for CRM_Utils_System::civiExit() ?

[demeritcowboy]

He might have just been politely trying to keep the PR from going on too long but if you want to update it now then you could pull the exit call out of the if, which funnily enough, makes it look more like it did 7 years ago.

    if ($cid) {
      $config = CRM_Core_Config::singleton();
      $fileExtension = strtolower(pathinfo($_GET['photo'], PATHINFO_EXTENSION));
      $this->download(
        $config->customFileUploadDir . $_GET['photo'],
        'image/' . ($fileExtension == 'jpg' ? 'jpeg' : $fileExtension),
        $this->ttl
      );
    }
    else {
      header("HTTP/1.0 404 Not Found");
    }
    CRM_Utils_System::civiExit();

[magnolia61]

I pulled the exit outside of the if now. @demeritcowboy @seamuslee001 agree & approve?

[demeritcowboy]

Can you squash the commits?

[magnolia61]

yep, done

[demeritcowboy]

Thanks. Looks good - have updated label.

[demeritcowboy]

jenkins retest this please (some kind of intermittent fail - rollover timing issue)

[yashodha]

@seamuslee001
The tests are passing. It looks like ready to merge.
@magnolia61 Can you do the follow up PR as per suggestion?

[magnolia61]

@yashodha I can also adjust this PR and change the
return
for
CRM_Utils_System::civiExit();
as @seamuslee001 suggests. Consensus on what would be best?