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

NickDickinsonWilde · 2017-10-21T23:40:49Z

In Drush 8.x themes could define Drush commands as well as modules. In 9.x, this is no longer the case.
This patch re-implements that.

greg-1-anderson · 2017-10-22T16:18:48Z

src/Drupal/DrupalKernel.php

-        // 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'] : [];


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.

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 :)

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).

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

greg-1-anderson · 2017-10-22T16:19:36Z

src/Drupal/DrupalKernel.php

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


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

greg-1-anderson · 2017-10-22T16:21:00Z

This is great, thanks! I'm not 100% sure about my comments, but some of the field access here looks potentially unsafe. If it would work equivalently to use local variables in those places, that would be preferable.

To facilitat this, also have to add the theme namespaces to the autoloader.

NickDickinsonWilde · 2017-10-22T18:28:32Z

src/Drupal/DrupalKernel.php

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


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.

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

Does the active theme have its autoloader added?

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.

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).

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

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.

Few code standard fixes Drupal->Drush

ericmulder1980 · 2017-11-06T08:04:35Z

Always great to find out there is a working patch for problems you come across. We really need this patch for the ZURB Foundation theme as creating subthemes currently does not work. More info at : https://www.drupal.org/node/2920472

I can confirm that the patch works. Any possible ETA on when this could be merged? Perhaps a beta-9 release?

weitzman · 2017-11-18T19:43:13Z

The PR wont be merged as is. The next step is in #3089 (comment)

Further narrow the namespace to only Drupal/theme_name/Commands.

NickDickinsonWilde · 2017-12-17T08:16:35Z

Finally got some time today to work on this more.

I didn't end up implementing a runtime autoload function for the theme namespaces. A quick investigation revealed that was going to be more complicated than I was first suspecting.
So I'm implemented it slight differently:

Only themes with a Drush services yml will be added to the autoloader
Only the theme/src/Commands folder is added to the autoloader.

I believe this should reduce the risk of accidental collisions sufficiently as well as reducing possibility for accidental effects. Anyways, I look forward to hearing your thoughts on whether that resolves your concerns.

weitzman · 2017-12-17T12:41:19Z

src/Drupal/DrupalKernel.php

+        if ($this->themeNames) {
+            return $this->themeNames;
+        }
+        $extensions = $this->getConfigStorage()->read('core.extension');


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

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.

weitzman · 2017-12-17T12:42:16Z

src/Drupal/DrupalKernel.php

@@ -203,4 +214,77 @@ protected function addDrushServiceProvider($serviceProviderName, $serviceYmlPath
            $this->serviceYamls['app'][$serviceProviderName] = $serviceYmlPath;
        }
    }
+
+    /**
+     * populates theme data on the filesystem.


Start with upper case.

weitzman · 2017-12-17T12:46:12Z

src/Drupal/DrupalKernel.php

+     * @see Drupal\Core\DrupalKernel::moduleData().
+     */
+    protected function themeData($theme_list)
+    {


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?

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.

NickDickinsonWilde · 2018-01-22T07:55:56Z

@weitzman or @greg-1-anderson either of you have any time to give this a re-review with those changes?
thanks

weitzman · 2018-01-26T17:35:51Z

I took a quick look and still dont feel especially comfortable with this. I'm very sensitive to maintenance/complexity costs these days. Lets see what @greg-1-anderson thainks.

weitzman · 2018-02-08T20:37:30Z

Folks wanting this should work on modernizing Drupal's theme system. I'd like to highlight catch's excellent comment on this issue.

weitzman · 2018-03-04T02:45:42Z

i'm going to cll this 'wont fix'. please work in drupal core to resolve this.

NickDickinsonWilde · 2018-03-05T14:59:45Z

Keep forgetting to comment back. I totally understand wanting to minimize stuff like this diverging from Drupal loader/maintainability etc and makes sense. Thanks for all the review and discussion. When I have time I'll check out that issue on the core queue you linked, thanks

NickDickinsonWilde added 2 commits October 21, 2017 16:33

Load Drush services for themes as well as modules.

36cf829

Merge branch 'master' into NickWilde-load-services-from-themes

ab6b69d

greg-1-anderson reviewed Oct 22, 2017

View reviewed changes

Split theme & module lists to prevent possible other effects.

73cca09

To facilitat this, also have to add the theme namespaces to the autoloader.

NickDickinsonWilde commented Oct 22, 2017

View reviewed changes

NickDickinsonWilde added 4 commits October 22, 2017 11:35

fix mention of 'modules' in docstring for theme function.

4c50cf1

Merge branch 'master' into NickWilde-load-services-from-themes

c62106f

Merge branch 'master' into NickWilde-load-services-from-themes

a8bcad0

Update DrupalKernel.php

dd08985

Few code standard fixes Drupal->Drush

NickDickinsonWilde added 2 commits December 16, 2017 22:54

Merge branch 'master' into NickWilde-load-services-from-themes

9d093de

Only load theme namespaces when there are Drush commands for the theme

10ba5e0

Further narrow the namespace to only Drupal/theme_name/Commands.

weitzman reviewed Dec 17, 2017

View reviewed changes

NickDickinsonWilde added 4 commits December 17, 2017 07:46

Minor style fix

6369090

Simply DrupalKernel::themeData()

153c102

Prevent cases where second invocations of Drush would fail

0f72f3d

Standards - fix incorrect spacing

d22c9e5

weitzman mentioned this pull request Feb 8, 2018

CommandNotFoundException: Command "basic-install" is not defined. #3373

Closed

weitzman closed this Mar 4, 2018

aweingarten mentioned this pull request Apr 11, 2018

Support for Drush 8 and Drush 9 Needed in BLT 9 acquia/blt#2729

Closed

violinist-bot mentioned this pull request Oct 23, 2020

Update squizlabs/php_codesniffer from 3.5.3 to 3.5.8 SignWo/drush#186

Closed

violinist-bot mentioned this pull request Oct 11, 2021

Update squizlabs/php_codesniffer from 3.5.3 to 3.6.1 SignWo/drush#219

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

NickDickinsonWilde commented Oct 21, 2017

greg-1-anderson Oct 22, 2017

NickDickinsonWilde Oct 22, 2017

greg-1-anderson Oct 22, 2017

NickDickinsonWilde Oct 22, 2017

greg-1-anderson Oct 22, 2017

greg-1-anderson commented Oct 22, 2017

NickDickinsonWilde Oct 22, 2017

greg-1-anderson Oct 22, 2017

NickDickinsonWilde Oct 22, 2017 •

edited

Loading

greg-1-anderson Nov 6, 2017

NickDickinsonWilde Nov 7, 2017

greg-1-anderson Nov 7, 2017 •

edited

Loading

ericmulder1980 commented Nov 6, 2017

weitzman commented Nov 18, 2017

NickDickinsonWilde commented Dec 17, 2017

weitzman Dec 17, 2017

NickDickinsonWilde Dec 17, 2017

weitzman Dec 17, 2017

NickDickinsonWilde Dec 17, 2017

weitzman Dec 17, 2017

NickDickinsonWilde Dec 17, 2017

NickDickinsonWilde commented Jan 22, 2018

weitzman commented Jan 26, 2018

weitzman commented Feb 8, 2018 •

edited

Loading

weitzman commented Mar 4, 2018

NickDickinsonWilde commented Mar 5, 2018

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

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

Conversation

NickDickinsonWilde commented Oct 21, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

greg-1-anderson commented Oct 22, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

NickDickinsonWilde Oct 22, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

greg-1-anderson Nov 7, 2017 • edited Loading

Choose a reason for hiding this comment

ericmulder1980 commented Nov 6, 2017

weitzman commented Nov 18, 2017

NickDickinsonWilde commented Dec 17, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

NickDickinsonWilde commented Jan 22, 2018

weitzman commented Jan 26, 2018

weitzman commented Feb 8, 2018 • edited Loading

weitzman commented Mar 4, 2018

NickDickinsonWilde commented Mar 5, 2018

NickDickinsonWilde Oct 22, 2017 •

edited

Loading

greg-1-anderson Nov 7, 2017 •

edited

Loading

weitzman commented Feb 8, 2018 •

edited

Loading