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

Default setup for Config Split #119

Merged
merged 21 commits into from
Jan 9, 2019
Merged

Default setup for Config Split #119

merged 21 commits into from
Jan 9, 2019

Conversation

becw
Copy link
Member

@becw becw commented Nov 21, 2018

Addresses #112.

This work changes the drupal-first-install target to use a custom install profile that lives within the-build. This pulls the module enabling/disabling and configuration changes out of the target, and puts them into the install profile.

The new install profile is a close sibling of the "standard" install profile, minus some modules that should be disabled on our sites, and plus config_split, devel, workbench, and workbench_tabs.

The new install profile also sets up three config_split environments: development, staging, and production; these splits are switched on in our settings.php files

Testing:

Other questions:

  • Is adding a Drupal install profile introducing a maintainability problem?
  • Should we be adding more stuff to the development split? (e.g. we could configure search_api_solr... or we could do this later -- it might make sense to wait on adding more starter config here)
  • Other thoughts?

@becw becw self-assigned this Nov 21, 2018
@becw becw requested a review from froboy November 21, 2018 16:14
@becw becw added this to the 2.0 milestone Nov 21, 2018
Copy link
Contributor

@froboy froboy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@becw this looks great. I like how you've split out the environments to their own build settings files. That definitely keeps things cleaner.

Re:

Is adding a Drupal install profile introducing a maintainability problem?

I'd say yes. I think committing the install profile to code means we'll have a lot of updates to do for almost every minor core update. I've seen this on Federated Search Demo, where I've rebuilt a few times to get Umami changes. I added some comments inline on how we might achieve what I think is a similar result. If you had other changes committed to code, I think we should be able to put all of those into the .install file you started (which I really like).

*
* @see system_install()
*/
function the_build_install() {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Moving all these to the profile is awesome.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also I totally re-used most of the stuff from the standard install profile here :)

defaults/templates/drupal.settings.php Show resolved Hide resolved
<arg value="drupal/config_split" />
<arg value="drupal/devel" />
<arg value="drupal/workbench" />
<arg value="drupal/workbench_tabs" />
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Additional recommendations:
https://www.drupal.org/project/stage_file_proxy
https://www.drupal.org/project/admin_toolbar

I also like to use https://www.drupal.org/project/environment_indicator, but I realize that might not be for everyone.

I think we can hold off on installing any of the more specific functionality like search_api, as that'll prob be on a project-by-project basis.

Copy link
Contributor

@jesconstantine jesconstantine Dec 4, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FYI I've created an issue (palantirnet/drupal-skeleton#83) which adds this list of modules into composer.json > suggest. PR: palantirnet/drupal-skeleton#84

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jesconstantine that's cool, but do we have any rationale between why some are pre-installed and some are just suggested? (prob a question for @becw too) but it seems like all of those in suggested could just move to require, as they could always be removed too.

@@ -24,6 +24,9 @@
$settings['file_public_path'] = '${drupal.site.settings.file_public_path}';
$settings['file_private_path'] = '${drupal.site.settings.file_private_path}';

// Enable/disable config_split configurations.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I might recommend changing this to the following (or adding):

To simulate other config split environments, change development to staging or production, then run drush cr && drush cim -y

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done!

$date_settings->set('timezone.default', 'America/Chicago');
$date_settings->set('timezone.user.configurable', FALSE);
$date_settings->save(TRUE);
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we remove the config, it might be good to add/remove modules with some code like this: https://github.com/palantirnet/akamai/blob/e267bbf5ebe8ea6ff59b07caecc1e9dd93c53fb4/web/modules/custom/ak_utilitiy/ak_utility.install#L11-L20

  $old = ['automated_cron', 'big_pipe', 'contact', 'history', 'search', 'tour'];
  $new = ['admin_toolbar', 'admin_toolbar_tools', 'config_split', 'workbench_tabs'];

  \Drupal::service('module_installer')->install($new);
  \Drupal::service('module_installer')->uninstall($old);

I left out devel and stage_file_proxy since they'll have to be set up split to the development environment.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think that we can extend the 'standard' install profile, given that #1356276 Allow profiles to define a base/parent profile and load them in the correct order is still open, which means that we wouldn't be uninstalling those modules like this... unless we provided the code that is currently an install profile as a module instead -- is that what you're thinking here?

The challenge I found with the standard install profile is that it is difficult to uninstall some of the modules that are enabled by default -- specifically, contact and comment, because the entity types/fields have to be explicitly deleted before they can be uninstalled. I think it's important to uninstall those modules because they present forms for user input that we don't usually use -- and that's not great, if we forget to uninstall them later.

... but if we don't use the standard install profile, we don't get things like the default block placement, the 'basic' block type, and the article/page content types... etc., etc.

So if I change this to be a utility module, then I would probably just do the extra work to fully uninstall contact and comment.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yea, I didn't fully think it out before, but without having any config then moving this into a module (like the_build_utility or the_build_installer as you suggested via Slack) I think would be the right path.

You present some good points, and I'm torn on where to start. I often think Minimal might be a better starting point since we're always overriding something in Standard, but then I look and Minimal is soooo little. I think your idea above sounds like the best compromise. With the existing install code it shouldn't (🤞) be too much work to remove those fields and disable the modules. That way instead of trying to track the changes to Standard with new Core updates we just have to make any modifications to our install steps, which should be much easier to maintain.

@froboy
Copy link
Contributor

froboy commented Nov 29, 2018

@becw one more thing I forgot to add: I wrote some pretty decent documentation for how to manage splits here https://github.com/palantirnet/umhs-acquia/blob/unified-search/docroot/config/README_CONFIG.md. Where do you think the most useful place for that to live would be?

@becw becw assigned jesconstantine and unassigned becw and froboy Dec 3, 2018
@jesconstantine
Copy link
Contributor

@becw I believe I've successfully moved all of the desired functionality and configuration out of the install profile and drupal-first-install task and into a custom module. I just have 1 outstanding question:

  1. Should the symlink to the custom module that provides the install hook and config_split config be removed after the module is uninstalled?

To test

  1. Require the-build using this branch in your project
  2. Reinstall the-build with vendor/bin/the-build-installer
  3. Say yes when it asks if you want to install Drupal
  4. Upon completion of the first site install, you should see a config/config_split/development/ directory with some configuration in it for devel
  5. When you log in to the site and visit yoursite.local/admin/config/development/configuration/config-split, the development split should be active.
  6. When you browse to the modules page yoursite.local/admin/modules you should see the following modules disabled: automated cron, big pipe, comment, contact, history, search, tour. You should see the following modules enabled (assuming you're in a local development environment): devel, workbench, workbench_tabs.
  7. When you browse through the site settings you should see:
    1. default country is US
    2. default timezone: America/Chicago
    3. users cannot configure their own timezone

@jesconstantine
Copy link
Contributor

jesconstantine commented Dec 5, 2018

@becw one more thing I forgot to add: I wrote some pretty decent documentation for how to manage splits here palantirnet/umhs-acquia:docroot/config/README_CONFIG.md@unified-search. Where do you think the most useful place for that to live would be?

@froboy how about in the drupal-skeleton /docs?

See palantirnet/drupal-skeleton#85

$date_settings->save(TRUE);

// Install modules that we want but are not included with standard profile.
$install = ['config_split', 'devel', 'workbench', 'workbench_tabs'];
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Once we add Admin Toolbar, these modules should be added:
admin_toolbar, admin_toolbar_tools

@becw
Copy link
Member Author

becw commented Dec 12, 2018

I made two small changes, and I'm happy with how this looks. Thank you so much, @jesconstantine and @froboy!

I have one remaining problem: when I visit the admin section after the first install, I get errors suggesting that the standard profile is still enabled. I see that the Drupal installer has rewritten the $settings['install_profile'] = 'config_installer'; line in the default settings.php file to specify standard, and I also see that when I re-import the config, this issue is resolved. I can think of one solution (rewrite the settings.php with phing and re-import the config, as part of the first-install target) but that might just be treating the symptoms without addressing the cause.

What do you think? Do we need to adopt the same profile-changing approach that you used on Akamai?

@froboy
Copy link
Contributor

froboy commented Dec 12, 2018

@becw @jesconstantine I'm trying to test this out, but having some trouble. I've required this branch and run the installer, but Drupal is having trouble seeing the_build_utility:

[drush] Executing '/var/www/ntc.local/vendor/bin/drush --nocolor --root="/var/www/ntc.local/web" --uri="https://ntc.local" --config="/var/www/ntc.local/drush/drushrc.php" --yes pm-enable "the_build_utility"'...
the_build_utility was not found.                                                       [warning]
No release history was found for the requested project (the_build_utility).              [error]
    [phing] /var/www/ntc.local/vendor/palantirnet/the-build/tasks/drupal.xml:275:48: Drush exited with code 1

I've checked and the module exists and is symlinked into the right place, but drush en the_build_utility fails, even after drush cc drush && drush cr. Thoughts?

@froboy
Copy link
Contributor

froboy commented Dec 17, 2018

I've tested this on a fresh skeleton and it worked fine. Not sure what was happening above.

@becw re: above:

I see that the Drupal installer has rewritten the $settings['install_profile'] = 'config_installer'; line in the default settings.php file to specify standard, and I also see that when I re-import the config, this issue is resolved.

This should be moot, as that line shouldn't be in settings.php at all. The status report complains now if that line exists at all:

INSTALL PROFILE IN SETTINGS
Drupal 8 no longer uses the $settings['install_profile'] value in settings.php and it can be removed.

@froboy
Copy link
Contributor

froboy commented Dec 17, 2018

I removed that line (see ⬆️ ) and the message goes away but install still proceeds correctly.

I think I'm seeing a similar notice on first login. No idea if this is something we should try to resolve, but it doesn't seem to be breaking anything and it doesn't appear on reinstall. Since we're always dealing with a fresh install case here I don't think we need to use the update-hook method... Maybe just advise folks to reinstall once after the initial before committing a db (which I'd assume almost everyone will do anyway).

Notice: Undefined index: name in system_requirements() (line 52 of core/modules/system/system.install).
system_requirements('runtime')
call_user_func_array('system_requirements', Array) (Line: 403)
Drupal\Core\Extension\ModuleHandler->invokeAll('requirements', Array) (Line: 112)
Drupal\system\SystemManager->listRequirements() (Line: 49)
Drupal\system\Controller\SystemInfoController->status()
call_user_func_array(Array, Array) (Line: 123)
Drupal\Core\EventSubscriber\EarlyRenderingControllerWrapperSubscriber->Drupal\Core\EventSubscriber\{closure}() (Line: 582)
Drupal\Core\Render\Renderer->executeInRenderContext(Object, Object) (Line: 124)
Drupal\Core\EventSubscriber\EarlyRenderingControllerWrapperSubscriber->wrapControllerExecutionInRenderContext(Array, Array) (Line: 97)
Drupal\Core\EventSubscriber\EarlyRenderingControllerWrapperSubscriber->Drupal\Core\EventSubscriber\{closure}() (Line: 151)
Symfony\Component\HttpKernel\HttpKernel->handleRaw(Object, 1) (Line: 68)
Symfony\Component\HttpKernel\HttpKernel->handle(Object, 1, 1) (Line: 57)
Drupal\Core\StackMiddleware\Session->handle(Object, 1, 1) (Line: 47)
Drupal\Core\StackMiddleware\KernelPreHandle->handle(Object, 1, 1) (Line: 99)
Drupal\page_cache\StackMiddleware\PageCache->pass(Object, 1, 1) (Line: 78)
Drupal\page_cache\StackMiddleware\PageCache->handle(Object, 1, 1) (Line: 47)
Drupal\Core\StackMiddleware\ReverseProxyMiddleware->handle(Object, 1, 1) (Line: 52)
Drupal\Core\StackMiddleware\NegotiationMiddleware->handle(Object, 1, 1) (Line: 23)
Stack\StackedHttpKernel->handle(Object, 1, 1) (Line: 669)
Drupal\Core\DrupalKernel->handle(Object) (Line: 19)

@becw becw removed the Open Question label Jan 9, 2019
@becw becw merged commit 92a093a into release-2.0 Jan 9, 2019
@becw becw deleted the config-split branch January 9, 2019 00:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants