CRM-19303 - Consolidate getCiviSourceStorage and getDefaultFileStorage functions

[colemanw]

This change was to delete the CRM_Utils_System_Base version of getDefaultFileStorage and getCiviSourceStorage and replace them with the code from CRM_Utils_System_Wordpress, which is cleaner & ought to work for every CMS.

• CRM-19303: CKEditor configuration can't be edited on a Drupal multisite installation

[totten]

In no particular order:

1. Concept: Agree that WP version is cleaner, clearer, and more generic.
2. Symlinking: In the past, we've had some issues with auto-constructing paths/URLs when site admins setup symlinks. My non-analytic/gut reaction is that this has a good chance of working (in fact, I think the WP logic was written to resolve one symlinking scenario, but maybe @kcristiano recalls better). Regardless, this stuff can get mind-bending and realistically needs some thoughtful QA.
   a. Example: Does it work with a site-build where $www/sites is a symlink outside the web root?
   b. Example: Does it work with a site-build where $www/sites is a symlink inside the web root?
   c. Example: Does it work with a site-build where $www/sites/default is a symlink?
   d. Example: Does it work with a site-build where $www/sites/$name is a symlink?
3. General QA: It's actually really hard to think about or test all these scenarios. A couple possible approaches (probably picking at least one):
   a. Test a whole bunch of cases manually
   b. Write a series of test-cases/build-types.
   c. Highlight this as an issue for focused testing in the release-candidate phase and the release-notes.
   d. Riff on "scientist". Basically, the next version should ship with two variants: the old-code and new-code. Execute both. If the results differ, then throw a warning/error and ask the user to send feedback. After a month or two of real-world validation, remove the old-code.

My favorite options are probably "scientist" and/or highlighting in the release-notes.

[colemanw]

I agree about the need for testing, but would temper it by noting that:

• This changes nothing for WP.
• Some of the above scenarios were already broken with the existing Drupal code that this replaces. So in the "scientist" approach, it's not necessarily a bad thing if the results differ; the point was to fix the broken paths for multisites.
• The existing Joomla code was pretty basic and hacky and probably broken for most of the above scenarios. So if this works on a vanilla J! site I think that's sufficient testing for Joomla.

[kcristiano]

@colemanw We did make changes to deal with problematic symlinking with one host for WordPress. We also needed to make changes to support alternative directory structures.

From reading through your changes, I would not expect any issues with WP, but I will need to test out all the use cases I run through.

[colemanw]

@kcristiano it's not easy to tell from the diff, but my change was simply to cut the functions out of CRM_Utils_System_Wordpress and paste them into CRM_Utils_System_Base, overwriting what was already there.
So as far as WP is concerned, the code hasn't changed at all. Other CMSs will need more thorough testing.

[kcristiano]

@colemanw Understood, I expect no changes. However, these functions have been a source of issues before, so I want to test just ensure nothing changes.

[davejenx]

A colleague has tested this PR and found a problem on a Civi 4.7 / Drupal 7 site on a server with a tricky symlink arrangement:

Document root is a symlink:
/var/docroots/mysite.com -> /var/www/drupal752civi4714

Civi is at:
/var/www/drupal752civi4714/sites/all/modules/civicrm

Site directory is a symlink:
/var/www/drupal752civi4714/sites/mysite.com -> /var/sites/mysite.com

civicrm.settings.php has:
$civicrm_root = '/var/docroots/mysite.com/sites/all/modules/civicrm';
if (!defined('CIVICRM_TEMPLATE_COMPILEDIR')) {
define( 'CIVICRM_TEMPLATE_COMPILEDIR', '/var/sites/mysite.com/files/civicrm/templates_c/');
}

/civicrm/admin/setting/path has:
Temporary Files Directory:
/var/sites/mysite.com/files/civicrm/upload/
Image Directory:
/var/sites/mysite.com/files/civicrm/persist/contribute/
Custom Files Directory:
/var/sites/mysite.com/files/civicrm/custom/

Expected result
URL path to CKEditor conf should be:
/sites/mysite.com/files/civicrm/persist/crm-ckeditor-civimail.js

Actual result
URL path to CKEditor conf incorrectly identified as:
/var/sites/mysite.com/files/civicrm/persist/crm-ckeditor-civimail.js

Another colleague had tried an alternative fix, using Drupal API function conf_path() in CRM/Utils/System/DrupalBase.php . This was successful apart from giving "undefined function" errors on standalone scripts such as /extern/open.php . His feeling is that the right way to fix is to use a Drupal API function to get Drupal's site directory and to fix the standalone scripts so that they bootstrap the CMS to a level just sufficient to get the config.

[seamuslee001]

@davejenx looking at it what makes you think that patch is incorrect that it identified when its adding /var onto things which makes it consistent with the path settings

[davejenx]

@seamuslee001 It incorrectly identified the URL path to CKEditor conf as /var/sites/mysite.com/files/civicrm/persist/crm-ckeditor-civimail.js , i.e. it tried (and failed) to load mysite.com/var/sites/mysite.com/files/civicrm/persist/crm-ckeditor-default.js - it shouldn't have var/ there.

[colemanw]

Thanks for the testing @davejenx - for comparison, was it working correctly without the patch?

[davejenx]

@colemanw No, it wasn't working correctly without the patch, it was then going to sites/default/ instead of sites/mysite.com/ .

[colemanw]

This has now received some manual testing which all confirms it at least isn't making things worse. To move this along, I propose we merge this change and announce it as an important item to test during the RC phase.

[davejenx]

@colemanw I'd say it arguably does make things worse in the case described in this comment: here the current behaviour is to give a valid URL path under sites/default/ (which allows the workaround of symlinking sites/mysite.com as sites/default) whereas the new behaviour is to give a non-existent URL path.

My vote is for your instinct described here:

My instinct tells me that we'd be better off using a native Drupal method instead of the functions in the last PR.

As noted in my above comment:

Another colleague had tried an alternative fix, using Drupal API function conf_path() in CRM/Utils/System/DrupalBase.php . This was successful apart from giving "undefined function" errors on standalone scripts such as /extern/open.php . His feeling is that the right way to fix is to use a Drupal API function to get Drupal's site directory and to fix the standalone scripts so that they bootstrap the CMS to a level just sufficient to get the config.

Drupal 8 doesn't have conf_path() though, so that would need further effort.

[colemanw]

One quick solution would be to keep this PR and also extend CRM_Utils_Drupal like:

public function getDefaultFileStorage() {
  if (function_exists('conf_path')) {
    // use it
  }
  else {
    return parent::getDefaultFileStorage();
  }
}

[davejenx]

@andy-walker @Nafhed Any thoughts on Coleman's suggestion above?

[colemanw]

I'm kind of on the fence. A lot of work has been put into the wordpress version of this function and so far we've only identified one flaw in it (with a tricky symlink arrangement). Maybe we should just fix that bug so all CMSs can benefit from a good working unified function.

[kcristiano]

@colemanw Correct me if I am wrong, but one of the reasons not to use CMS specific functions is to ensure we can find CiviCRM when the CMS is not bootstrapped.

If my recollection is correct, I'd rather keep your function, but provide a way to override it. Much like we are looking at for CRM-16421 and the civicrm.settings.extra.php file totten suggested the Edale sprint https://gist.github.com/totten/f080b6fa821d1b6c4d6711e750f04cd2

[colemanw]

My current thinking is that if this function works flawlessly then there is no need to override it using CMS-specific functions and deal with bootstrapping issues.

So far one edge-case bug has been identified. @davejenx could you suggest a patch to this PR that would fix it?

[roblog]

The patch worked for me after a manual install. Thanks

[andrewpthompson]

@colemanw I've tested this patch on Joomla and found some problems.
On backend pages "administrator/" is duplicated in the links to resources, e.g.:

<script type="text/javascript" src="/administrator/administrator/components/com_civicrm/civicrm/bower_components/jquery/dist/jquery.min.js?r=56hVt">

The above is with the CiviCRM Resource URL set to its default of [civicrm.root]/
And on all backend and frontend pages I get a php notice:

Notice: Undefined property: CRM_Core_Config::$imageUploadDir in /var/www/html/administrator/components/com_civicrm/civicrm/CRM/Utils/System/Joomla.php on line 716

On a Dashboard frontend page I'm also getting a datatables error:

DataTables warning: table id=DataTables_Table_0 - Invalid JSON response. 

Otherwise it does fix the problem in this stackexchange question. of the frontend resource URLs omitting administrator/ when Resource URL setting is set to [civicrm.root].

[seamuslee001]

I have just tested this on AUG multisite setup and seems to work as we need

[gboudrias]

Just tested this patch (again) on a Drupal/Aegir multisite setup with no issues.

[mark-rodgers]

Just tested on Drupal multi-site and this fixed our issue.👍

[seamuslee001]

@eileenmcnaughton @colemanw @totten Eileen, Coleman and Tim, I think this makes more sense in that out at this point, I know AUG has been running this and it fixed it for us but there does appear to be an issue for Joomla. I think we should merge this after the RC is cut and work on the Joomla issue

[totten]

Re: @davejenx and this:

Another colleague had tried an alternative fix, using Drupal API function conf_path() in CRM/Utils/System/DrupalBase.php . This was successful apart from giving "undefined function" errors on standalone scripts such as /extern/open.php . His feeling is that the right way to fix is to use a Drupal API function to get Drupal's site directory and to fix the standalone scripts so that they bootstrap the CMS to a level just sufficient to get the config.

This parallels the experience that came up when @kcristiano tried to fix some funny directory structures in WP. It would be easier to implement getCiviSourceStorage() and getDefaultFileStorage() using CMS util functions, but these functions are boot-critical for standalone scripts, so the simple fix is actually a fail.

@kcristiano and I gravitated toward an approach where you write out the boot-critical data to civicrm.settings.php. (IMHO, this is probably the original raison d'etre of civicrm.settings.php.) The effort snagged a bit because it was conceived as a WordPress issue, and it's hard to change the civicrm.settings.php template for only one CMS. But if we reframed as a general/pan-CMS issue, then it may be more doable.

Steps:

• Update the civicrm.settings.php.template to include some define()s or globals for the path+URL of civicrm.root. (Possibly do the same for civicrm.files and cms.root.)
• Change the behavior of civicrm.root so that it does a fallback search with whatever information is available -- check the CMS util functions ... or the define()s... or throw an error. (This would mean patching CRM_Utils_System_*::getCiviSourceStorage() and/or Civi\Core\Paths.)
• Update the installers to fill in the define()s (using CMS helpers).
• Update the System.check process. It should complain if the define() is missing or doesn't match the CMS's values.
• Re-test.
• Provide suitable explanations/warnings in the release-notes/docs.

This arrangement means:

• The CMS utilities generally make the decision about the path/URL.
• When you upgrade the code or migrate to a different server/URL/path, your civicrm.settings.php may not be appropriate. But the web UI can still work and display warnings/advice.
• If you have a funky environment or provisioning process, you can put custom logic into civicrm.settings.php to compute the values.

--

In the interest of full-disclosure, there are theoretically other ways to get access to the CMS utility functions. I view it as a sliding-scale of difficulty/risk:

• Add define()s to civicrm.settings.php (etal). This feels most-concrete / most-incremental / lowest-risk. (Not "low" but "lower than the others".)
• Rewrite standalone bootstrap code (civicrm.config.php, etal) to use CMS-first algorithm. This feels more risky+difficult for devs and sysadmins. (But ultimately makes the system more consistent... and possibly makes civicrm.settings.php unnecessary?)
• Convert all standalone entry-points into CMS entry-points. This feels like the highest risk and most work (for devs and sysadmins).

If someone is actually willing to put a several days dev/test time into making the second one work, then I can write more pointers about that. But otherwise I think the first approach is pretty reasonable.

[kcristiano]

@totten I agree - I think we need to define paths and URLs by CMS. I'd like to define these at install using CMS functions. You had detailed an idea at the Edale Spring in 2016 - https://gist.github.com/totten/f080b6fa821d1b6c4d6711e750f04cd2#file-settings-diff-L65

Do you still believe this is the best path forward? We'd end up with civicrm.settings.extra.php as needed? I was planning on creating this for WP (goal would be have it ready for testing by St Louis Sprint). Sounds like we can look at doing this for Drupal and Joomla as well.

[xurizaemon]

Rewrite standalone bootstrap code (civicrm.config.php, etal) to use CMS-first algorithm. This feels more risky+difficult for devs and sysadmins. (But ultimately makes the system more consistent... and possibly makes civicrm.settings.php unnecessary?)

This would be a challenge, but ultimately this (which depends on eliminating remaining standalone scripts) is the only one that will spare us these issues in future IMO.