Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

(REF) Make it easier for extensions to define basic bundles #18660

Merged
merged 3 commits into from
Oct 2, 2020

Conversation

totten
Copy link
Member

@totten totten commented Oct 2, 2020

Overview

#18247 (same dev-cycle, 5.31.alpha1) allows one to define resource bundles. This improves developer experience, requiring a bit less boilerplate/duplication.

Before

To define a new bundle, one implements hook_container and creates a factory function.

function mymodule_civicrm_container($container) {
  $container->setDefinition('bundle.mycalendar',  new \Symfony\Component\DependencyInjection\Definition(
    'CRM_Core_Resources_Bundle', [])
    )->setFactory('_mymodule_create_mybundle')->setPublic(TRUE);
}

function _mymodule_create_mybundle() {
  $bundle = new CRM_Core_Resources_Bundle('my_bundle', ['...allowed types...']);
  $bundle->addScriptFile(...)->addStyleFile(...)->addVars(...);
  CRM_Utils_Hook::alterBundle($bundle);
  CRM_Core_Resources_Common::useRegion($bundle, CRM_Core_Resources_Common::REGION);
  return $bundle;
}

Note that the factory function _mymodule_create_mybundle has some boilerplate around alterBundle() and useRegion(). It's important to include these to ensure that the bundle is modifiable.

After

You still create a definition, but the definition can use the pre-written function createBasicBundle:

function mymodule_civicrm_container($container) {
  $container->setDefinition('bundle.mycalendar',  new \Symfony\Component\DependencyInjection\Definition(
    'CRM_Core_Resources_Bundle', ['my_bundle'])
    )->setFactory('CRM_Core_Resources_Common::createBasicBundle')->setPublic(TRUE);
}

Optionally, if you want to initialize the default content, you can still use a factory function - but it doesn't have to have the boilerplate. Note how _mymodule_init_mybundle is simpler than _mymodule_create_mybundle:

function mymodule_civicrm_container($container) {
  $container->setDefinition('bundle.mycalendar',  new \Symfony\Component\DependencyInjection\Definition(
    'CRM_Core_Resources_Bundle', ['my_bundle', '_mymodule_init_mybundle'])
    )->setFactory('CRM_Core_Resources_Common::createBasicBundle')->setPublic(TRUE);
}

function _mymodule_init_mybundle($bundle) {
  $bundle->addScriptFile(...)->addStyleFile(...)->addVars(...);
}

Comments

I marked it as (REF) because it's still backward compatible and not visible to an end-user. The simplification only applies if you use createBasicBundle. Any existing bundles (like the bootstrap3 bundle) can still work as before. Additionally, note that this is refining something recently added in the same dev-cycle (5.31-alpah1).

I'd like to get this merged before 5.31-beta1 so that we can use the cleaner notation in the docs.

@civibot
Copy link

civibot bot commented Oct 2, 2020

(Standard links)

@civibot civibot bot added the master label Oct 2, 2020
@totten totten force-pushed the master-bundle-ext branch from c7dabeb to 050e4bf Compare October 2, 2020 02:50
totten added 2 commits October 1, 2020 22:51
This mostly affects a hypothetical bundle defined outside of core's `Common.php`:

* Before: You cannot re-use `useRegion()`. You'd have to write it again.

* After: You can re-use and/or override `fillDefaults()`.
@totten totten force-pushed the master-bundle-ext branch from 050e4bf to 8eb00bd Compare October 2, 2020 05:56
@eileenmcnaughton
Copy link
Contributor

MOP - this is still being put through it's paces / documented so lots of time to flush out anything

@seamuslee001
Copy link
Contributor

Jenkins re test this please

@eileenmcnaughton
Copy link
Contributor

test this please

@eileenmcnaughton eileenmcnaughton merged commit 02bd631 into civicrm:master Oct 2, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants