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

Load Drush services for themes as well as modules. #3089

Closed
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
Split theme & module lists to prevent possible other effects.
To facilitat this, also have to add the theme namespaces to the autoloader.
NickDickinsonWilde committed Oct 22, 2017
commit 73cca09349873532cc5548bc1e603f752901573f
49 changes: 40 additions & 9 deletions src/Drupal/DrupalKernel.php
Original file line number Diff line number Diff line change
@@ -11,6 +11,9 @@ class DrupalKernel extends DrupalDrupalKernel
/** @var ServiceModifierInterface[] */
protected $serviceModifiers = [];

/** @var array */
protected $themeNames;

/**
* @inheritdoc
*/
@@ -60,6 +63,7 @@ protected function initializeContainer()
if ($this->shouldDrushInvalidateContainer()) {
$this->invalidateContainer();
}
$this->classLoaderAddMultiplePsr4($this->getModuleNamespacesPsr4($this->getThemeFileNames()));
Copy link
Author

Choose a reason for hiding this comment

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

This has to be added somewhere to add the theme folders to the autoloader. I did a bit of investigating and I think this is the best spot but it's the item I'm least sure about.

Copy link
Member

Choose a reason for hiding this comment

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

Did this happen as a side effect of adding the themes to the module list before?

Does the active theme have its autoloader added?

Copy link
Author

@NickDickinsonWilde NickDickinsonWilde Oct 22, 2017

Choose a reason for hiding this comment

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

Yeah, didn't need it before because, one of the uses of $this->moduleList was propagating the autoloader. (that line is basically copy & paste but with module changed to theme)

All enabled themes get their autoloader added by this - in Drupal core, themes don't get added to the autoloader at all.

Copy link
Member

Choose a reason for hiding this comment

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

This part is the hang up for me. I'm worried about the unpredictable consequences of Drush adding theme classes that Drupal does not add to the autoloader. Maybe this is no big deal; each theme should be uniquely namespaced, and if no one references the class, then it should not matter if it's autoloadable. I am concerned by the fact that this happens all the time when using Drush, whether or not a theme command is being executed. If this did cause a change in behavior, it could be really difficult to track down.

It might be safer to only add the autoloader when a theme command is executed. This is what the Terminus plugin mechanism does. The consequence of doing something similar for themes is that theme commands would have access to all of the classes defined in the theme, whether or not the theme was enabled, but hooks in the theme only have access to the command / hook classes in disabled themes (but should still have access to all classes in the enabled theme).

Copy link
Author

Choose a reason for hiding this comment

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

hmm... I'm 95% sure I can simply implement that with a bit of copy/paste from there and editing. Investigating, thanks :)

Copy link
Member

@greg-1-anderson greg-1-anderson Nov 7, 2017

Choose a reason for hiding this comment

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

Terminus has an autoload file that it can include, which is probably not going to be the case here. You may need to instantiate your own class loader. The Composer class loader works well in this capacity. I've done so successfully in the past, but at the moment I can't quite remember where, or I'd provide a link.

return parent::initializeContainer();
}

@@ -106,17 +110,21 @@ public function discoverServiceProviders()
// necessary that the class files in these commands are available
// in the autoloader.

// Also add Drush services from all modules & themes.
$extensions = $this->getConfigStorage()->read('core.extension');
$this->moduleList += isset($extensions['theme']) ? $extensions['theme'] : [];
$this->themeData();

// Also add Drush services from all modules.
$module_filenames = $this->getModuleFileNames();
// Load each module's serviceProvider class.
foreach ($module_filenames as $module => $filename) {
$filename = dirname($filename) . "/drush.services.yml";
$this->addDrushServiceProvider("_drush.$module", $filename);
}

// Also add Drush services from all themes.
$theme_filenames = $this->getThemeFileNames();
// Load each theme's serviceProvider class.
foreach ($theme_filenames as $theme => $filename) {
$filename = dirname($filename) . "/drush.services.yml";
$this->addDrushServiceProvider("_drush.$theme", $filename);
}
}

/**
@@ -134,15 +142,15 @@ protected function addDrushServiceProvider($serviceProviderName, $filename)
*
* @see Drupal\Core\DrupalKernel::moduleData().
*/
protected function themeData()
protected function themeData($theme_list)
{
Copy link
Member

Choose a reason for hiding this comment

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

I'm not too clear why we need this code. Why is iterating over all enabled themes sufficient? Are we wanting to catch the case where Drush integration is in the parent thee? What does simpletest have to do with anything?

Copy link
Author

Choose a reason for hiding this comment

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

I've simplified this slightly. This is necessary to populate the Drupal path cache so that drupal_get_path() and similar function work for themes. I think we can remove the simpletest stuff as we're not loading themes for testing - it is in the core Drupal\Core\DrupalKernel::moduleData() but reviewing it, we don't need it for this use case, so removed.

// First, find profiles.
$listing = new ExtensionDiscovery($this->root);
$listing->setProfileDirectories([]);
$all_profiles = $listing->scan('profile');
$profiles = array_intersect_key($all_profiles, $this->moduleList);
$profiles = array_intersect_key($all_profiles, $theme_list);

// If a module is within a profile directory but specifies another
// If a theme is within a profile directory but specifies another
// profile for testing, it needs to be found in the parent profile.
$settings = $this->getConfigStorage()->read('simpletest.settings');
$parent_profile = !empty($settings['parent_profile']) ? $settings['parent_profile'] : NULL;
@@ -158,6 +166,29 @@ protected function themeData()
$listing->setProfileDirectories($profile_directories);

// Now find themes.
$this->moduleData += $listing->scan('theme');
return $listing->scan('theme');
}

/**
* Gets the file name for each enabled module.
*
* @return array
* Array where each key is a module name, and each value is a path to the
* respective *.info.yml file.
*/
protected function getThemeFileNames() {
if ($this->themeNames) {
return $this->themeNames;
}
$extensions = $this->getConfigStorage()->read('core.extension');
Copy link
Member

Choose a reason for hiding this comment

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

Lets use a Drupal API like \Drupal::service('theme_handler')->listInfo(); or system_list('theme')

Copy link
Author

Choose a reason for hiding this comment

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

Can't do that unless we load core/includes/module.inc. At this point in the bootstrap process, it isn't yet which means system_list() is unavailable and ->listInfo() uses it.

$theme_list = isset($extensions['theme']) ? $extensions['theme'] : [];
$theme = [];
$theme_data = $this->themeData($theme_list);
foreach ($theme_list as $theme => $weight) {
if (isset($theme_data[$theme])) {
$this->themeNames[$theme] = $theme_data[$theme]->getPathname();
}
}
return $this->themeNames;
}
}