Add higher-level support for "bundles" and "collections" of resources #18247
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
(Standard links)
|
|
The patch is fairly long, so it may be hard to trace the refactorings point-by-point. As an alternative QA, I would suggest these as possible tasks:
|
|
@totten Would you be able to review/merge #18141 before I go too deep into this PR. That "fixes" an edge-case with addvars that causes problems for me with Stripe. I think I understand the need for something like this PR - given that I now have a fairly complex set of requirements and optional requirements for Stripe which together make for the best user experience (and in some situations make it work). I'm increasing running into issues around load-order and tracking that. Perhaps we could have a chat to discuss a bit further? |
|
@mattwire - OK, cool. I'll try to take a read on #18141 after I get up tomorrow. I hadn't seen the PR before. At first glance, it looks like the gist of it is to store settings on a per-region basis. I'm rather glad you've come to that 😌 ... that was a characteristic I needed along the way, but I wasn't sure if would make sense from other POV. If I understand correctly, it's roughly the same goal as 9a65188, with a difference in where that storage specifically lives:
But I probably need to read 99c6336 more carefully to grok the motivation, the edges (like initialization/emptiness/etc), and the tests. (FWIW, the motivation in 9a65188 is to allow logic like |
|
@colemanw I feel like you know this code the best |
totten
commented
Aug 25, 2020
5c25fd7 to
3d51660
Compare
|
I'm generally OK with this PR & at this stage |
This makes it possible for renderSnippet to work recursively. For example, the current `jquery` type can build on the `script` type, and the hypothetical `scriptFile` type could build on the `scriptUrl` type.
In the prior commits, the signatures for `addScriptFile()`, `addScriptUrl()`, etc are not strictly interoperable between `CRM_Core_Resources` and `CollectionTrait`. This is because they use key-value options instead of positional options. This makes it easier disregard positional options that don't make sense (e.g. when calling `CRM_Core_Region::addScriptFile()`, it's silly to reserve a positional argument for the `$region` option). The signatures *can* be unified by using "splats" (ie `...$options`) to accept either key-value options or backward-compatible positional options. The ultimate is hope is that: * `CRM_Core_Resources`, `CRM_Core_Region`, and `CRM_Core_Bundle` all implement the `CollectionAdderInterface`. * `CRM_Core_Resources`, `CRM_Core_Region`, and `CRM_Core_Bundle` all accept options in either format (key-value or positional). * The positional format will fade-away. The methods in CollectionTrait are newer terrain, so it's safer to change those signatures, so we do that first. Note that CollectionTrait formally builds on CollectionAdderTrait. This ensures that IDE navigation to (eg) `CRM_Core_Region::add()` and `CRM_Core_Resources_Bundle::add()` works as expected.
…gfully used If you grep universe for `addCoreResources` and `addCoreStyles`, all matches fall into these buckets: 1. They use the implicit default region. 2. They explicitly use the `html-header` region. 3. (Singular case) addCoreResources($region) delegates to addCoreStyles($region); but given 1+2, this must still be `html-header`.
…d 'coreResources'
…esources
This is a very subtle behavioral change. To understand it, we must consider
the traditional ordering in `CRM_Core_Resources::addScriptUrl()` and
`CRM_Core_Region::add()`. Compare these two examples:
```
Civi::resources()->addScriptUrl('http://example.com/foo.js', 100, 'page-footer');
Civi::resources()->addScriptUrl('http://example.com/bar.js', 100, 'page-footer');
CRM_Core_Region::instance('page-footer')->add([
'scriptUrl' => 'http://example.com/foo.js',
'weight' => 100,
]);
CRM_Core_Region::instance('page-footer')->add([
'scriptUrl' => 'http://example.com/bar.js',
'weight' => 100,
]);
```
You might expect this to output `<SCRIPT>`s in the same order. After all,
the examples use the same URLs, the same weights, and the same
procedural/natural order. Moreover, `Civi::resources()` is just a wrapper
for `CRM_Core_Region`. Surely they were the same!
They were not. The output order would differ.
The reason is that the ordering had two keys (`weight,name`), and the
secondary-key (`name`) was hidden from us:
* In `CRM_Core_Resources::addScriptUrl()`, the default `name` was the URL.
This was handy for de-duping resources.
* In `CRM_Core_Region::add()`, the default `name` was a numeric position.
This made it behave like procedural/natural-ordering.
Of course, the goal in this branch is to make various collections support
the same methods and the same behaviors. But they didn't agree before, so
something has to give. Either:
1. Standardize on the behavior of `Resources::addScriptUrl()` and change `Region::add(scriptUrl)`.
2. Standardize on the behavior of `Region::add(scriptUrl)` and change `Resources::addScriptUrl()`.
3. Embrace a split. All `CRM_Core_Resources::add*()` behave one way, and all `CRM_Core_Region::add*()` behave another.
4. Embrace a split. All rich adders (`*::addScriptUrl()`) behave one way, and all generic adders (`*::add()`) behave another.
This developmental branch has been using `civicrm#3`. The patch changes it to `civicrm#4`.
Both splits achieve backward compatibility wrt `Resources::addScriptUrl()`
and `Region::add(scriptUrl)`. The difference is that `civicrm#4` allows us to have
the same behavior in all collections (regardless of subtype), which means
that collections can be more interoperable and share more code. From my
POV, it's still quirkier than `#1` or `#2`, but it's not as quirky as `civicrm#3`.
…rInterface Before ------ `CRM_Core_Resources` directly implements a number of functions (`addFoo()`) After ----- `CRM_Core_Resources` re-uses the implementations from `CollectionAdderTrait` Comment ------- The method signatures in `CollectionAdderTrait` look different, but they are compatible. In particular, the use of splat (`...$options`) means that they can accept either key-value options or positional options. The existing test-cases in `CollectionTestTrait` assert as much.
3d51660 to
fa9a476
Compare
|
We discussed this on a CT call this morning and the consensus was now that the rc is cut we should merge this and really try to focus in on the greenwich-as-theme that it lays the groundwork for. It will have 2 full months between merge & release |
| @@ -206,6 +206,12 @@ public function createContainer() { | |||
| [] | |||
| ))->setPublic(TRUE); | |||
|
|
|||
| $container->setDefinition('bundle.coreStyles', new Definition('CRM_Core_Resources_Bundle', ['coreStyles'])) | |||
| ->setFactory('CRM_Core_Resources_Common::createStyleBundle'); | |||
|
|
|||
There was a problem hiding this comment.
Do these need setPublic(TRUE) or is there something you need to configure on a site? I'm having trouble after updating a drupal 8 site and adding setPublic makes it work.
totten
added a commit
to totten/civicrm-core
that referenced
this pull request
Sep 18, 2020
(This fixes a small bug in a new function added during this release -- part of civicrm#18247.) The signature of `addBundle()` optionally accepts an array/iterable -- if given, then it should add all the items from the array. For example: ```php Civi::resources()->addBundle(['foo', 'bar']); ``` Before ------ It adds `foo` but then bails out on `bar`. After ----- It adds both `foo` and `bar`.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Overview
Every HTML page is built on a list of "resources" (aka "assets") such as Javascript and CSS files. Civi provides a few inter-related APIs for managing these (
CRM_Core_Resources,CRM_Core_Region). This branch expands on these in a few ways:CRM_Core_Resources_Bundle). By using a bundle, an application-page may activate several resources with a single step.foobar) to the list of related resources (js/foobar.js,css/foobar.css, andCRM.vars.foobar).CRM_Core_Resources::addCoreResources(),CRM_Core_Resources::addCoreStyles()).CRM_Core_Resources,CRM_Core_Resources_Bundle, andCRM_Core_Region. To wit: the methodsadd(),addScriptFile(),addScriptUrl(), etc. are supported across the board. They are designated via PHP interfaces/traits.The resulting model looks like so:
(Note: This contributes toward https://lab.civicrm.org/dev/user-interface/-/issues/27.)
Before
(1) There is no pattern for giving a name to a "mixable list of resources".
(2) There are three places where you might add, inspect, or tap-into a list of resources. These three build on top of each other, and they are broadly similar, but there are technical differences.
CRM_Core_Resourceshook_coreResourcesListCRM_Core_RegionaddFoo('bar')$arr[] = 'bar'add(['foo' =>'bar'])weightweightSome notable differences between them:
addScriptUrl(),addScriptFile(), etc are inCRM_Core_Resourcesbut not inCRM_Core_Regionorhook_coreResourcesListadd()is inCRM_Core_Regionbut not inCRM_Core_Resourcesorhook_coreResourcesListhook_coreResourceListallows extensions to add/filter/remove resources. However, it only supports a couple resource-types, and strictness of URLs not always jive with dynamic assets.After
(1)
CRM_Core_Resources_Bundledefines a "list of resources" that is available for use -- but is not necessarily activated on every page-load.(2) The different collection-like APIs (
CRM_Core_Resources,CRM_Core_Region, etc) all implement the same PHP interfaces (CollectionInterfaceandCollectionAdderInterface) to ensure similar usage/behavior.CollectionInterface/CollectionTrait?CollectionAdderInterface/CollectionAdderTrait?get(),getAll(),find(),filter(),update()add(),addScriptFile(),addStyleFile(),addSetting(), ad nauseumCRM_Core_RegionCRM_Core_Resources_BundleCRM_Core_Resources(3) Every instance of
CRM_Core_Resources_Bundlecan be altered viahook_civicrm_alterBundle($bundle).hook_coreResourcesList. It works with more bundles, allows more content-types, and provides helper methods likefilter(),find(), andclear().hook_coreResourcesList.(4) There are two built-in bundles,
coreStylesandcoreResources. These are defined inCRM_Core_Resources_Common.