Allow extensions to add core resources to the list

[colemanw]

Overview

Now extensions can add resources directly to the list using hook_core_resourceList

Before

Only paths relative to the civicrm directory were supported.

After

One can specify a path relative to an extension, e.g. '[org.civicrm.api4]/js/api4.js'

[colemanw]

@totten you and I discussed this change last week. FYI I've tested it out by patching the api4 extension as follows and it worked as expected:

--- a/api4.php
+++ b/api4.php
@@ -71,7 +71,7 @@ function api4_civicrm_container($container) {
  */
 function api4_civicrm_coreResourceList(&$list, $region) {
   if ($region == 'html-header') {
-    Civi::resources()->addScriptFile('org.civicrm.api4', 'js/api4.js', -9000, $region);
+    $list[] = '[org.civicrm.api4]/js/api4.js';
   }
 }

[totten]

It is a good idea to have a string notation that can resolve to files that originate in different ways. We're collecting quite a list...

CRM_Core_Config::singleton()->imageUploadDir . '/foo'`;
Civi::paths()->getPath('[civicrm.files]/foo');
Civi::resources()->getPath('org.civicrm.api4', 'foo');
Civi::resources()->getPath('civicrm.files', 'foo');

CRM_Core_Config::singleton()->imageUploadURL . '/foo'`;
Civi::paths()->getUrl('[civicrm.files]/foo');
Civi::resources()->getUrl('org.civicrm.api4', 'foo');
Civi::resources()->getUrl('civicrm.files', 'foo');

/* Within hook_civicrm_angularModules... */
'ext://org.civicrm.api4/foo'
'assetBuilder://foo.js'

/* Within hook_coreResourceList */
'[org.civicrm.api4]/foo'

Can we do this in a way that's more general?

I took a stab at making Civi::paths() support more notations:

Civi::paths()->getUrl('[civicrm.files]/foo')
Civi::paths()->getUrl('[config.imageUpload]/foo')
Civi::paths()->getUrl('ext://org.civicrm.api4/foo')
Civi::paths()->getUrl('assetBuilder://foo.js')

Civi::paths()->getPath('[civicrm.files]/foo')
Civi::paths()->getPath('[config.imageUpload]/foo')
Civi::paths()->getPath('ext://org.civicrm.api4/foo')
Civi::paths()->getPath('assetBuilder://foo.js')

https://github.com/civicrm/civicrm-core/compare/master...totten:master-path-unity?expand=1

This felt good on an aesthetic level, and it would allow one to (e.g.) set customCSSURL to ext://org.civicrm.shoreditch/css/civicrm-custom.css (which seemed neat - albeit a bit redundant with the broader theming patch+extension)... But then I circled back on how to apply it for the hook_coreResourceList use-case:

/* #1 */
$this->addScriptUrl(Civi::paths()->getUrl('[civicrm.root]/foo.js'), $jsWeight++, $region, $translate);

Hrm. Functionally, that's not really the same as what you were trying -- because addScriptFile() does more than addScriptURL() (i.e. addScriptFile() also handles JS translations).

Just spit-balling some other ways to get a general/flexible string notation... one could add more functions in CRM_Core_Resources which take a single string expression:

/* #2 */ 
Civi::resources()->addScriptExpr('[civicrm.root]/foo.js', $jsWeight++, $region, $translate);

But we'd have to do that several times -- applying the idea to several functions (addScriptExpr(), addStyleExpr(), getPathExpr(), getUrlExpr()).

Alternatively... maybe we could make the existing APIs works with a singular string. Just pass some placeholder in the first param (e.g. $ext==NULL or $ext=='EXPR') as a way to signal that it's proper to do full-on interpretation of the $file` expression.

/* #3 */
Civi::resources()->addScriptFile(NULL, '[civicrm.root]/foo.js')
Civi::resources()->addScriptFile(NULL, '[config.imageUpload]/foo.js')
Civi::resources()->addScriptFile(NULL, 'ext://org.civicrm.api4/foo.js')
Civi::resources()->addScriptFile(NULL, 'assetBuilder://foo.js')

This feels a bit more practical -- because (a) we have the same API entry-points rather than doubling them and (b) that means fewer code-paths to maintain. We'd just need extra-fiddly bits go into Resources::getPath() and Resources::getUrl() (and some doc updates)?

I have a suspicion that this works but is still somehow an odd class-design. It merits some thinking...

(EDIT) Maybe a combination makes sense in which:

• Civi::paths() is primarily about resolving paths/URLs.
• Civi::resources() is primarily about managing resources. It includes getPath() and getUrl() for backwards compat, but they're basically wrappers for Civi::paths().

[colemanw]

@totten thanks for thinking through this so deeply. I agree that option 3 is a good idea to sync up the difference between how Civi::resources() manages urls and how Civi::paths() does it, but at the end of the day it doesn't solve the problem. $coreResourceList is a flat array of strings, but your solution requires 2 params, a flag and a string.

[totten]

@colemanw That's interesting. In option 3, I thought it was amenable to a flat list -- because you can NULL-out the first param and just use the strings from the flat list, e.g.:

$coreResources = [
  '[civicrm.root]/foo.js',             // Example with explicit base (system-defined)
  '[config.imageUpload]/foo.js',       // Example with explicit base (user-configured)
  'ext://org.civicrm.api4/foo.js',     // Example provided by an extension
  'assetBuilder://foo.js',             // Example of a dynamically generated asset
];
foreach ($coreResources as $item) {
  if ($item ends with js) {
    Civi::resources()->addScriptFile(NULL, $item);
  elseif ($item ends with css) 
    Civi::resources()->addStyleFile(NULL, $item);
}

But I'm a little removed from hook_civicrm_coreResourceList per-se, and my thinking unwittingly considered a new/idealized list. The actual existing list currently going through hook_civicrm_coreResourceList looks different -- all the items are implicitly relative to [civicrm.root] or ext://civicrm. You might think of it as the implicit base.

This feels familiar. Related situations:

• A big reason why Civi::paths() was introduced with support for [civicrm.root]/foo/bar notation was that the implicit-bases for the old settings were conflicting/unintuitive. To provide backward-compatibility with old settings data, Civi::paths() still supports implicit-bases for URLs and paths (Paths::DEFAULT_URL and Paths::DEFAULT_PATH). Of course, this was deprecated ~4.7.0 -- with a warning in checkUrlVariables()+checkDirVariables() and some changed defaults.
• In using hook_civicrm_angularModules, the hook originally expected that all path expressions (for js, css, partials) were relative to the identified extension. But then we've had things like afform -- and it helps to reference assets from a data-folder or another extension or the asset-builder.

At the risk of getting increasingly wonky, here's another riff on option 3 -- but instead of using $ext==NULL, the $ext could be used to signal the implicit or default base, as in:

/* #4 */
$coreResources = [
  'foo.js',                            // Example with implicit base
  '[civicrm.root]/foo.js',             // Example with explicit base (system-defined)
  '[config.imageUpload]/foo.js',       // Example with explicit base (user-configured)
  'ext://org.civicrm.api4/foo.js',     // Example provided by an extension
  'assetBuilder://foo.js',             // Example of a dynamically generated asset
];
foreach ($coreResources as $item) {
  if ($item ends with js) {
    Civi::resources()->addScriptFile('civicrm', $item);
    // ^^ Note how $ext provides the implicit base,  but $item can ignore it and be explicit
  elseif ($item ends with css) {
    Civi::resources()->addStyleFile('civicrm', $item);
    // ^^ Note how $ext provides the implicit base,  but $item can ignore it and be explicit
  }
}

(Aside: As written, this is a subtle contract change. If you have a file literally named /var/www/sites/all/modules/civicrm/[civicrm.imageUpload]/foo.js or /var/www/sites/all/modules/civicrm/ext://org.civicrm.api4/foo.js, then it's a BC break. The BC break could be avoided... but it's such a small edge-case... I'd assume it's not worth the extra interface-complexity unless someone else people pipes-in on that being important.)

[eileenmcnaughton]

@totten @colemanw where is this at - I can see some thought has gone into it.....

[eileenmcnaughton]

@totten @colemanw ping....

[colemanw]

I'm going to close this because #13582 includes a simpler but still effective mechanism for including fully-formed urls in the list.