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 2 commits
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
38 changes: 37 additions & 1 deletion src/Drupal/DrupalKernel.php
Original file line number Diff line number Diff line change
@@ -106,7 +106,11 @@ public function discoverServiceProviders()
// necessary that the class files in these commands are available
// in the autoloader.

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

Choose a reason for hiding this comment

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

I don't know if this is prudent. This field is not only used by Drush, but is used throughout Drupal. Adding the themes to the module list might have unintended consequences. It would probably be much better to simply build a list of themes in local variables, and then have a separate loop over $theme_filenames to call addDrushServiceProvider.

Copy link
Author

Choose a reason for hiding this comment

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

hmmm... yes that does make sense; I wasn't considering that - I was thinking this only affects Drush. Ah well... quite easy to modify. I'll swap over to local/adjust the implementation. A bit more code/close to duplication but seems very worth it to not be altering core data without tons of testing
thanks for the review :)

Copy link
Member

Choose a reason for hiding this comment

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

You could have less duplication by merging the module list and theme list, but I don't think you should do that either. You're not supposed to ever have a module and a theme with the same name, but I've seen that violated before, so best keep the lists separated (unless Drupal is actually preventing that now -- have not tried recently).

Copy link
Author

Choose a reason for hiding this comment

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

Yeah I've seen that before too, which should have made me think about keeping it separate

$this->themeData();

$module_filenames = $this->getModuleFileNames();
// Load each module's serviceProvider class.
foreach ($module_filenames as $module => $filename) {
@@ -124,4 +128,36 @@ protected function addDrushServiceProvider($serviceProviderName, $filename)
$this->serviceYamls['app'][$serviceProviderName] = $filename;
}
}

/**
* populates theme data on the filesystem.
Copy link
Member

Choose a reason for hiding this comment

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

Start with upper case.

Copy link
Author

Choose a reason for hiding this comment

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

Fixed

*
* @see Drupal\Core\DrupalKernel::moduleData().
*/
protected function themeData()
{
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);

// If a module 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;
if ($parent_profile && !isset($profiles[$parent_profile])) {
// In case both profile directories contain the same extension, the
// actual profile always has precedence.
$profiles = array($parent_profile => $all_profiles[$parent_profile]) + $profiles;
}

$profile_directories = array_map(function ($profile) {
return $profile->getPath();
}, $profiles);
$listing->setProfileDirectories($profile_directories);

// Now find themes.
$this->moduleData += $listing->scan('theme');
Copy link
Member

Choose a reason for hiding this comment

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

Same comment - not sure that it is safe to modify $this->moduleData like this.

}
}