Fixing allowing use of xvfb-run in wkhtmltopdf path in env.php verification checks

[mxroo]

Overview

Applying the fix from previous commit to CRM/Admin/Form/Setting/Miscellaneous.php to this file
1834031
http://issues.civicrm.org/jira/browse/CRM-13292

Before

System status page showed Missing System Package: wkhtmltopdf, when using /usr/bin/xvfb-run -- wkhtmltopdf as the wkhtmltopdf path.

After

Using /usr/bin/xvfb-run -- wkhtmltopdf as the wkhtmltopdf path works without errors.

[civicrm-builder]

Can one of the admins verify this patch?

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

[colemanw]

Thanks for the PR @mxroo!
Indentation needs updating now that you've nested in another conditional.
Also, I'm wondering if this file also needs to be patched with a similar fix:

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

Lines 211 to 214 in 6bbe0cf

	// if the path doesn't exist fall back on the current backup which is DOMPDF.
	if (!file_exists($config->wkhtmltopdfPath)) {
	return self::_html2pdf_dompdf($paper_size, $orientation, $html, $output, $fileName);
	}

[colemanw]

ping @seamuslee001

[colemanw]

@civicrm-builder add to whitelist

[seamuslee001]

I would say @colemanw is right that we need to patch that other place but otherwise I think this looks good

[eileenmcnaughton]

test this please

[mxroo]

Made changes to remove $fields and fix syntax and indentation with help from @scoobird, and tested. This now appears to work as desired for CRM/Utils/Check/Component/Env.php
Agreed that CRM/Utils/PDF/Utils.php also needs a change, working on that now.

[demeritcowboy]

I wonder if the status check is doing more harm than good. There's now another report of it failing if you have open_basedir. The original purpose of it seemed to be if you switched servers and forgot to reinstall wkhtmltopdf - maybe it's better to just let it crash in that situation?

[agileware-justin]

Can we just remove this status check entirely, it's not helpful.

[agileware-justin]

Rather than fallback to domPDF which actually produces very different PDF output. Would prefer that an error be raised, error logged and the operation cancelled. Some manual action is required to fix the issue.

Continuing the operation and sending a weird looking PDF is just as bad as not sending a PDF at all.

[seamuslee001]

kind of the point of the status check was that that was how the 'error' was being raised

[agileware-justin]

then why do this? return self::_html2pdf_dompdf

[agileware-justin]

The status check is incorrect, returns false positive - so the value of this status check is "kind of" negated.

[seamuslee001]

@agileware-justin the reason to do that is because dompdf is the default fall back if wkhtmltopdf setting is not set https://github.com/civicrm/civicrm-core/blob/master/CRM/Utils/PDF/Utils.php#L104 so in this case we are saying that whilst it is set it can't be found so we are reverting back to what would be the default if the setting was just blank.

My point was that the status check is how the error about if wkhtmltopdf can be run is alerted rather than in each individual potential PDF generation process. My opinion that is the more correct place.

I can accept that the status check / this code is giving some false positives but they should be fixed

[agileware-justin]

So Utils.php#L104

Already implements the fall back if wkhtml is undefined, that's OK.

 if (CRM_Core_Config::singleton()->wkhtmltopdfPath) {
      return self::_html2pdf_wkhtmltopdf($paper_size, $orientation, $margins, $html, $output, $fileName);
    }
    else {
      return self::_html2pdf_dompdf($paper_size, $orientation, $html, $output, $fileName);
    }

Then in _html2pdf_wkhtmltopdf - I just don't understand why these checks are being performed again. Another check of the configuration, again check the paths and again use a fall back. This code seems redundant to me.

  if (!empty($config->wkhtmltopdfPath)) {
      $pieces = explode(' ',$config->wkhtmltopdfPath);
      $path = $pieces[0];
      if (!file_exists($path) || !is_executable($path)) {
        return self::_html2pdf_dompdf($paper_size, $orientation, $html, $output, $fileName);

[agileware-justin]

@seamuslee001 just a FYI you missed the opportunity to discuss this one at the virtual meet up yesterday 😄

[seamuslee001]

@jusfreeman so the reason for the additional check here is for the following scenario

You move from one server to another (mysql dump and load in process etc) However on your new host wkhtmltopdf doesn't exist or doesn't exist in the same location. However wkhtmltopdfPath is still set in civicrm_settings table

this if if (CRM_Core_Config::singleton()->wkhtmltopdfPath) { only checks to see that the CiviCRM setting is set. It does not check that the path actually is valid.

the new checks are saying ok we have the setting set but lets try and see if that path actually exists on the server before trying to run it because we ran into a hard fail with a client site where they were using the attach PDF receipt to a contribution receipt email.

If we find that it isn't there or perhaps it can't be accessed by the www user then we are saying ok well lets not run through this function and use the backup function as we can't be sure that wkhtmltopdf is workable for us and we don't want to cause hard fails later on.

The status check is there to prompt the users to ask for assistance because as you say they will likely be producing different content and the check in the utils is there to prevent any actual hard fails in between the point at which in my example the server migration occurs and either wkhtmltopdf is reinstalled or the setting is blanked out.

[eileenmcnaughton]

@mxroo we discussed this & ary e going to revert #19311 in the rc as a quick fix and then consider whether https://lab.civicrm.org/dev/core/-/issues/2028 has a better fix.

That will probably make this stale/ redundant but you might like to follow the gitlab to ensure your situation is tested in any future attempts to fix it

[mxroo]

Understood, if you are reverting the code that caused this, that works for us.
I'm going to close this pull request.