Problem/Motivation

The critical need

As documented in #3051352: [Plan] Remove unused jQuery UI components and replace with a suite of contrib packages for the continuous upgrade path, jQueryUI is deprecated in Drupal9 core. In Drupal 9, contrib themes dependent on jQueryUI should be satisfying their jQueryUI requirements via the contrib jQueryUI modules. It needs to be possible for these themes to declare these contrib modules as dependencies.

Additional motivation

Parsing .info files for dependency information is already implemented on the modules administration page. Adding support for the same dependencies in theme.info files, and implementing the same behavior on the admin/build/themes page, would allow designers building heavily customized themes to make safer assumptions about their target sites.

A theme might require the existence of imagecache to auto-generate variations of a header image. This would be a nice compromise between systems like Wordpress and Joomla!, which give themes much greater control over the functionality of the site, and Drupal's module-centric approach.

Proposed resolution

  • Allow themes to add the dependencies to .info.yml files
  • Show these dependencies on /admin/appearance and make it impossible to install without the requirements via UI or command line
  • Install dependencies automatically on API level.Moved to followup #3100374: Make it possible to install dependent modules when installing theme
  • Validation is added to prevent installing themes that depend on uninstalled modules, and prevent uninstalling modules that are depended upon by installed themes. This is implemented at a level that prevents these installs/uninstalls from occurring programatically or via cli (Drush/Drupal Console).

Follow-ups

  1. #3100374: Make it possible to install dependent modules when installing theme
  2. Update #3005229: Provide optional support for using composer.json for dependency metadata to read composer.json theme's also

User interface changes

Themes listed in admin/appearance will list the modules they depend on and the ability to install that theme will not be available unless those module dependencies are met.
The module descriptions found at admin/modules and admin/module/uninstall will include themes in their "Required By" description.

API changes

Themes will be able to define modules dependencies in their info.yml via the dependencies key.

Release notes snippet

Pending.

Original report by [eaton]

Issue Summary
Parsing .info files for dependency information is already implemented on the modules administration page. Adding support for the same dependencies in theme.info files, and implementing the same behavior on the admin/build/themes page, would allow designers building heavily customized themes to make safer assumptions about their target sites.

A theme might require the existence of imagecache to auto-generate variations of a header image. This would be a nice compromise between systems like Wordpress and Joomla!, which give themes much greater control over the functionality of the site, and Drupal's module-centric approach.

CommentFileSizeAuthor
#308 474684-308-9.0.x.patch76.17 KBtedbow
#308 interdiff-9.0.x-307-308.txt2.66 KBtedbow
#308 474684-308-8.9.x.patch76.19 KBtedbow
#308 interdiff-303-308-8.9.x.txt2.52 KBtedbow
#307 474684-306-9.0.patch76.29 KBtedbow
#303 474684-303.patch76.25 KBtedbow
#303 interdiff-301-303.txt1016 bytestedbow
#302 474684-301.patch76.2 KBtedbow
#302 interdiff-297-301.txt8.96 KBtedbow
#297 interdiff-288-297.txt8.19 KBjungle
#297 474684-297.patch75.67 KBjungle
#288 474684-289.patch75.73 KBtedbow
#288 interdiff-286-289.txt6.9 KBtedbow
#286 474684-287.patch75.82 KBtedbow
#286 interdiff-285-287.txt6.51 KBtedbow
#285 474684-286.patch75.79 KBtedbow
#285 interdiff-282-286.txt17.01 KBtedbow
#282 474684-283.patch80.1 KBtedbow
#279 474684-279.patch80.2 KBbnjmnm
#279 interdiff_272-279.txt1.88 KBbnjmnm
#272 474684-272.patch80.28 KBbnjmnm
#272 interdiff_269-272.txt2.63 KBbnjmnm
#269 interdiff_267-269.txt8.09 KBbnjmnm
#269 474684-269.patch80.02 KBbnjmnm
#267 474684--267.patch76.63 KBbnjmnm
#267 interdiff__265-267.txt4.29 KBbnjmnm
#265 474684-265.patch72.35 KBbnjmnm
#265 interdiff_260-265.txt22.36 KBbnjmnm
#262 Screenshot 2020-01-21 at 15.30.12.png151.3 KBalexpott
#260 474684-260.patch67.01 KBbnjmnm
#260 interdiff_258-260.txt5.73 KBbnjmnm
#258 474684-258.patch67.24 KBbnjmnm
#258 interdiff_256-258.txt827 bytesbnjmnm
#256 interdiff_254-256.txt5.92 KBbnjmnm
#256 474684-256.patch67.27 KBbnjmnm
#254 474684-254.patch63.24 KBbnjmnm
#254 interdiff_252-254.txt5.99 KBbnjmnm
#252 474684-252.patch60.01 KBbnjmnm
#252 interdiff_250-252.txt915 bytesbnjmnm
#250 474684-250.patch60.01 KBbnjmnm
#250 interdiff_246-250.txt6.92 KBbnjmnm
#246 474684--246.patch57.24 KBbnjmnm
#246 interdiff__243-246.txt16.23 KBbnjmnm
#243 474684-243.patch57.25 KBbnjmnm
#243 interdiff_240-243.txt20.98 KBbnjmnm
#240 474684-240.patch55.41 KBbnjmnm
#240 interdiff_238-240.txt18.28 KBbnjmnm
#238 474684-238.patch49.43 KBbnjmnm
#238 interdiff_236-238.txt1.27 KBbnjmnm
#236 474684-236.patch49.68 KBbnjmnm
#236 interdiff_234-236.txt4.67 KBbnjmnm
#234 474684-234.patch49.68 KBbnjmnm
#234 interdiff_232-234.txt5.97 KBbnjmnm
#232 474684-232.patch48.92 KBbnjmnm
#232 interdiff_230-232.txt1.05 KBbnjmnm
#230 474684-230.patch48.97 KBbnjmnm
#230 interdiff_228-230.txt19.85 KBbnjmnm
#228 474684-228.patch50.5 KBbnjmnm
#228 interdiff_226-228.txt15.45 KBbnjmnm
#226 474684-226.patch51.13 KBbnjmnm
#226 interdiff_224-226.txt1.02 KBbnjmnm
#224 474684-224.patch51.13 KBbnjmnm
#224 interdiff_222-224.txt21.52 KBbnjmnm
#222 474684-222-reroll.patch51.85 KBtedbow
#220 474684-220.patch51.84 KBrensingh99
#220 interdiff.txt607 bytesrensingh99
#220 after_module_install.png13.64 KBrensingh99
#220 required_message.png14.9 KBrensingh99
#220 module_name.png7.15 KBrensingh99
#219 474684-219.patch51.84 KBbnjmnm
#219 interdiff_217-219.txt15.14 KBbnjmnm
#217 474684-217.patch50.17 KBbnjmnm
#217 interdiff_215_217.txt4.12 KBbnjmnm
#215 474684-215.patch46.59 KBbnjmnm
#215 interdiff_208-215.txt60.97 KBbnjmnm
#208 474684-208.patch66.89 KBbnjmnm
#208 interdiff_206-208.txt13.92 KBbnjmnm
#206 474684-206.patch66.16 KBbnjmnm
#206 interdiff_205-206.txt34.15 KBbnjmnm
#205 interdiff_204-205.txt11.58 KBbnjmnm
#205 474684-205.patch37.18 KBbnjmnm
#204 474684-204-REROLL-REPAIR.patch32.8 KBbnjmnm
#203 reroll_diff_194-203.txt22.16 KBshaal
#203 allow-themes-to-declare-dependencies-474684-203.patch26.27 KBshaal
#194 allow-themes-to-declare-dependencies-1-474684-194.patch28.1 KBkamkejj
#189 474684-188.patch28.03 KBpfrenssen
#186 474684-186.patch28.04 KBvdacosta@voidtek.com
#186 474684-186.patch28.04 KBvdacosta@voidtek.com
#184 474684-183.patch35.25 KBademarco
#184 interdiff-474684-181-183.txt2.87 KBademarco
#184 interdiff-474684-178-183.txt8.18 KBademarco
#181 474684-181.patch35.13 KBademarco
#181 interdiff-474684-180-181.txt2.24 KBademarco
#180 474684-180-failing-test.patch34.26 KBademarco
#180 interdiff-474684-178-180.txt4.24 KBademarco
#8 build_dependencies_for_theme-474684-8.patch4.01 KBsreynen
#30 screenshot10.png64.46 KBRobLoach
#30 thememoduledependency.patch6.27 KBRobLoach
#36 modtheme.patch10.42 KBSnugug
#47 theme_dependencies-474684-46.patch21.92 KBthedavidmeister
#64 474684-64.patch2.73 KBdawehner
#65 474684-65.patch2.73 KBMiguel.kode
#67 474684-67.patch6.68 KBdawehner
#67 interdiff.txt4.96 KBdawehner
#82 474684-82.patch6.71 KBdawehner
#82 interdiff.txt623 bytesdawehner
#90 474684-90.patch8.73 KBdawehner
#90 interdiff.txt2.02 KBdawehner
#93 Screen Shot 2016-05-01 at 08.50.01.png43.12 KBJohnAlbin
#95 interdiff.txt756 bytesalmaudoh
#95 474684-95.patch8.73 KBalmaudoh
#104 theme-dependencies-474684-104.patch8.07 KBJohnAlbin
#107 interdiff.txt1.2 KBjhedstrom
#107 474684-107.patch9.27 KBjhedstrom
#109 theme-dependencies-474684-109.patch19.41 KBJohnAlbin
#109 interdiff.txt13.84 KBJohnAlbin
#112 interdiff.txt13.23 KBJohnAlbin
#112 theme-dependencies-474684-112.patch22.84 KBJohnAlbin
#112 interdiff.txt13.23 KBJohnAlbin
#114 theme-dependencies-474684-114.patch22.88 KBJohnAlbin
#114 interdiff.txt719 bytesJohnAlbin
#126 474684-126.patch23.58 KBdawehner
#126 interdiff-474684.txt10.34 KBdawehner
#127 474684-127.patch25.86 KBdawehner
#127 interdiff-474684.txt2.28 KBdawehner
#132 474684-132.patch27.5 KBdawehner
#132 interdiff-474684.txt5.02 KBdawehner
#134 474684-134.patch27.51 KBdawehner
#134 interdiff-474684.txt1.25 KBdawehner
#137 474684-137.patch22.45 KBdpagini
#137 interdiff-474684-134-137.txt925 bytesdpagini
#138 474684-138.patch27.51 KBkevineinarsson
#138 interdiff-474684-137-138.txt4.18 KBkevineinarsson
#142 474684-142-theme-dependencies.patch27.71 KBkalpaitch
#142 interdiff-474684-138-142.txt1.23 KBkalpaitch
#144 474684-144-theme-dependencies.patch27.34 KBkalpaitch
#144 interdiff-142-144.txt3.23 KBkalpaitch
#145 474684-145-theme-dependencies.patch27.35 KBkalpaitch
#145 interdiff-474684-142-145.txt3.22 KBkalpaitch
#148 474684-148.patch30.06 KBdawehner
#148 interdiff-474684.txt3.52 KBdawehner
#151 474684-151.patch31.1 KBdawehner
#151 interdiff-474684.txt2.41 KBdawehner
#154 screenshot-d8x.localhost-2018.02.07-13-54-05.png185.24 KBPol
#161 474684-161.patch30.82 KBdawehner
#161 interdiff-474684.txt1.46 KBdawehner
#164 474684-164.patch30.67 KBdawehner
#164 interdiff-474684.txt5.31 KBdawehner
#165 474684-165.patch30.71 KBpfrenssen
#165 interdiff.patch3.72 KBpfrenssen
#167 474684-167.patch30.81 KBpfrenssen
#167 interdiff.patch3.66 KBpfrenssen
#178 interdiff-474684-167-178.txt7.22 KBmarkhalliwell
#178 474684-178.patch35.98 KBmarkhalliwell
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

philbar’s picture

+1

See: #340967: Remove Dependency on Extra Voting Forms (Test Alternative Voting Modules)

I am not keen on adding a dependency, because Drigg itself doesn't depend on it. It's "default theme" does.

I'm in the process of moving the default theme to a theme project page. It will depend on Drigg and Extra Voting Form.

stephthegeek’s picture

+1, this would be an exciting option potentially encouraging some interesting contrib themes for various site recipes/configurations

laura s’s picture

+1: the bee's knees.

Jeff Burnz’s picture

Yes, this would be fantastic - right now I can't tell you often we have to deal with support/bug issues because our base theme didn't get installed, or we have add a silly long description in the hope the user will read it AND actually click the links (that our theme needs this or that module to be feature complete).

I actually think this darn near borders on being a bug - the user can install a sub-theme, site blows up, wtf?

sreynen’s picture

This preliminary patch simply collects the dependency data from theme .info files, without actually using it anywhere. The first change is simply renaming _module_build_dependencies() as _system_build_dependencies() and moving it to system.inc to indicate it can be used for more than just modules. After that, a second parameter is necessary because the current implementation only builds a dependency graph among the files being checked, meaning themes could only depend on themes. Finally, this adds a call to _system_build_dependencies() from system_rebuild_theme_data() to match the call in system_rebuild_module_data().

This builds themes dependencies on other themes as well as modules, though modules still can not depend on themes. If that sounds right, next step is to actually apply the dependencies for themes similar to how they're applied for modules.

rickvug’s picture

beefzilla’s picture

Yes, this would be nice to have, as I just created a theme that depends on the conditional styles module, http://drupal.org/project/conditional_styles

I'd be surprised if drupal 7 doesn't support this already

dvessel’s picture

@Jeff Burnz, that's definitely a bug. Defining a base theme should implicitly act as a dependency even if we can't explicitly define dependencies.

Setting a module as a dependency will pose some UI challenges. It's mildly annoying managing dependent and disabled checkboxes in the module config page. What would you do when themes come into the picture?

RobLoach’s picture

Having themes depend on certain modules would be great. xCSS or SASS are good examples where themes can benefit from required module dependencies.

webchick’s picture

Status: Active » Needs work

Marking this as having a patch. I like this approach. dvessel's point about UX challenges though is not to be ignored.

sreynen’s picture

I'm interested in working on the UX challenges, but it would be nice to first get some consensus on whether or not it would also be useful to have module dependencies on themes, as that would suggest a significantly different, much more complicated approach to this. The only example I can think of where that might be useful is admin module having a dependency on Rubik. Any thoughts on whether that's a use case worth allowing?

dvessel’s picture

sreynen, I think that's going too far. Modules provide general functionality that themes may require but themes tend to be very specific. It might be useful for one-off custom modules on a private site but allowing the two way dependency in other cases will be a big mess.

KrisBulman’s picture

I definitely consider this a bug in d6 & d7.. subthemes should not break a site if the parent theme isn't present... there should be a warning, or the theme simply should just not show up in theme settings. It almost makes a case for storing sub themes inside their parent themes directory... at least when you are the maintainer of the base and sub theme.

Jeff Burnz’s picture

Category: feature » bug

@11, 13, 14, we started working on a redesign of the themes page: #1167444: Not clear which themes are enabled and disabled, if we can agree this needs to happen we can work on some of the UI stuff over there maybe.

Seems like there is consensus that this is a bug.

sreynen’s picture

Merging the two issues seems like a good idea. The UI changes here are dependent on the UI changes in #1167444: Not clear which themes are enabled and disabled, but those UI changes can't happen without the dependency checking code here. Should we just mark this as duplicate and move everything over to #1167444, expanding the scope a bit? Or would it be better to postpone this until #1167444 is fixed?

Jeff Burnz’s picture

OK, I postponed #1167444: Not clear which themes are enabled and disabled, we can still work on our ideas over there (UI changes).

catch’s picture

laura s’s picture

Re UI changes in #17, I would rather see a theme grayed out with a message why (e.g., "requires foobar theme to be installed" or "requires Skinr module to be enabled").

Re #14 (theme dependencies for modules) I can't say I've encountered that, but as Drupal gets more presentation-agnostic and the web gets more platform-agnostic, I can't say this use case would not arise. I wonder if the easy "fix" there would be simply to add the themes available somewhere in the modules management area, just as themes and modules both appear on update status.

Todd Nienkerk’s picture

sun’s picture

Title: Themes should support dependencies[] » dependencies[] for themes, so they can depend on modules
Category: bug » feature
Priority: Normal » Major
Issue tags: +API clean-up

Better title. Regardless of annoyance, this is a feature request.

I'd highly recommend to defer the implicit base theme as dependencies[] handling to a separate issue. The idea is sound, but it leads to having a theme name in a list of module names - turning the list into a list of extension names. All of that is sound and I really support it, but the merge of modules and themes into extensions needs to be discussed in a larger scope first, and we don't want this issue to get blocked on that.

+++ b/includes/module.inc
@@ -202,39 +202,6 @@ function system_list_reset() {
-function _module_build_dependencies($files) {

+++ b/modules/system/system.module
@@ -2415,13 +2415,51 @@ function system_rebuild_module_data() {
+function _system_build_dependencies($files, $dependency_files = array()) {

@@ -2563,6 +2601,8 @@ function system_rebuild_theme_data() {
+  $modules = system_rebuild_module_data();
+  $themes = _system_build_dependencies($themes, $modules);

Unfortunately, this change hugely contradicts a major D8 effort that attempts to move all the low-level bootstrap functionality out of System module: #679112: Time for system.module and most of includes to commit seppuku

The function needs to stay in module.inc.

That said, the renaming idea itself looks bogus to me -- you're still building and checking for module dependencies.

20 days to next Drupal core point release.

KrisBulman’s picture

Module dependencies aside, what about sub themes having base themes as dependencies? Should a separate issue be created?

sun’s picture

@KrisBulman: A sub-theme can only have one "parent" base theme, and that's specified via the base theme .info file property already. Also, as mentioned in #24, I still recommend to leave fiddling with base themes to a separate follow-up issue.

Jeff Burnz’s picture

RobLoach’s picture

RobLoach’s picture

Status: Needs work » Needs review
FileSize
64.46 KB
6.27 KB

Doesn't take sun's notes from above into considering. Just wanted to throw something up there to get things rolling again....

Snugug’s picture

Is there any movement on this?

loominade’s picture

#1435406: allow themes to require modules has been marked as a duplicate

rcross’s picture

Cross posting a comment here.

While by no means a perfect solution, my suggestion here is for anyone who found this entry due to searching for 'theme module dependency' (or the like).

Drupal 8 may incorporate a fancy mechanism for this, Drupal 7 and 6 do not, but in D6 and D7 you can do this, in your template.php in your mytheme_preprocess_page() function add the following:

function mytheme_preprocess_page(&$vars)
{
  ...
  if (!module_exists('needed_module'))
  {
    // No needed_module means we need to add it, let everyone know.
    //
    print "<h1>"  . t( 'mytheme requires that module needed_module is installed')
        . "</h1>"
        . "<h6>"  . t( 'warning generated in file: %f', array ( '%f'=>__FILE__ ))
        . "</h6>";
  }
  ...
}

Yes, this is a bit kludgy, because it just prints out the error. But if your theme might be broken anyway, best to get the message out. And, if your user installed the theme without the module, they'll know instantly what's wrong. If something breaks, like your user uninstalls the dependency, they'll know right away.

So, you've got all you need to solve it right there. You could get more elaborate, perhaps making your warning include where to download the missing module, but meh.

rcross’s picture

that aside, what else is needed to move this forward?

sreynen’s picture

Status: Needs review » Needs work

Looks like the next step is to apply sun's comments in #24 to the patch in #30. Specifically, move _system_build_dependencies() back to _module_build_dependencies().

Snugug’s picture

FileSize
10.42 KB

Did a bunch of work on this at DrupalCon Munich core sprint. Still needs work, but on its way.

yannickoo’s picture

Snugug, you should remove the dsm right? And I would recommend to use dpm instead of dsm ;)

Snugug’s picture

Like I said, this needs more work and is not finished. This patch was how far I got in sprints today and I put it up here so I could get it once I got back to my computer at the suggestion of John Albin. Once I've got a working computer again I will be able to finish this and get it to a point to really be considered.

thedavidmeister’s picture

Been working on this today using Snugug's work as a base. Decided I have some issues with what is there beyond the dsm() and that all of the implications of what Sun was saying is not immediately clear until you're looking directly at the code and trying to refactor it.

So here's my interpretation of what Sun was saying:

  • _module_build_dependencies is an API function that "figure out the modules that these files depend on, using the module system's definition of dependency" and it should stay that way
  • Don't put this functionality in system.module
  • When determining what a theme relies on, don't lump base themes and modules into a big list of "dependencies", keep the distinction clear all the way from the underlying logic through to the UI presented to the end-user.

Here's my review of what Snugug posted - well aware that it is "unfinished" but I don't know what the intentions of this patch were/are as there aren't any comments or clues in this queue so I can only discuss what is in front of me.

  • The function _theme_build_dependencies written by Snugug should mirror _module_build_dependencies and "figure out all the themes that these files depend on, using the theme system's definition of dependency" - opposed to "figure out all modules and themes that these theme files depend on, using the theme system's definition of module/theme dependencies"
  • The logic that prevents a theme with missing dependencies from being enabled should not start in the theming of the system form as this won't help with drush or direct calls to theme_enable().
  • Shouldn't be querying the system table directly to get a list of modules that are missing/disabled/enabled, we can use system_rebuild_module_data()

Also, kind of related post - #1067408: Themes do not have an installation status

So, I ran out of time today, I've been building on the patch for Snugug but don't have anything to post yet. I'll try to keep working on it this weekend.

Here's the approach that I'm taking:

  • Turn Snugug's _theme_build_dependencies into a way for the theme system to define theme dependencies for files, based on base themes defined in .info files
  • Change the documentation for _module_build_dependencies slightly to say that it checks what modules are dependent on the list of *files* rather than a list of *modules*
  • Write a separate function that checks both the theme and module dependencies for a given theme for use in the theme enabling process and the general UI
  • Make sure that theme_enable() can't enable theme's that have missing dependencies
  • Update the UI to represent what's going on now - Snugug did most of the work here
anandkp’s picture

Just piping in my own two cents...

Wanted to mention that this would be awesome for advanced themers that use all sorts of tweaky-modules like Browser Class, CSS3Pie, Entity View Mode, Prepro w/ Sassy and tonnes of others.

I see many, many benefits to this and just wanna say, keep the awesomeness rolling! Really think this will help make packaging Drupal as a product that much more end-user-friendly.

Additional thoughts and ideas... ;o)

On the side, here's a question - if a User decides to enable a theme that has disabled dependencies, would we get that warning page that asks if we want the said dependencies to be enabled?

I can think of one other user friendly perk that could be clubbed into this patch/update. Would it be possible to offer to download and install those modules (a la Drush)? If not that, then it would be wonderful if we could at least show the user links to the missing modules if they are not found in the site install.

Once again, keep up the awesome work and thank you very, very much for your time and efforts!

PS... Search terms were "theme module dependency" and the reason I searched was cause of an admin-theme that I've customised off of Rubik where I'm now using Sassy to handle the .scss files...

Cheers :o)

thedavidmeister’s picture

@anandps - Yeah, I use prepro and related modules for a lot of my stuff, which is my main motivation for looking into this too. I definitely agree that whatever comes out of this thread needs to play nice with methods of enabling extensions outside of the UI (like drush).

As for how the UI should work... having themes give you all the same warnings, etc.. as modules across the board might need to be implemented at least partially by other modules, like Features or even drush.

It makes sense to provide a convenience screen to enable dependent modules, but we need to get the nuts-and-bolts right in the API first before we can polish up the UI.

davidneedham’s picture

Totally. I was giving snugug some moral support at DC Munich, so I know we were building this to match the user experience from module dependencies. This patch should already incorporate your suggestions around warning, prompts and enabling dependencies. :-)

thedavidmeister’s picture

Looking at this some more, I think the right place to call _theme_build_dependencies is in system_rebuild_theme_data() as the only place _module_build_dependencies is called is within system_rebuild_module_data(). (Both of which should be moved to theme_rebuild_theme_data() and module_rebuild_module_data() at some point, according to the post that Sun referenced).

To keep requires and required_by split by extension type, we kind of have to introduce extension-type specific keys at this point. Ie. where previously you would have called system_rebuild_module_data() or system_rebuild_theme_data to get a list of all available modules and themes then done something like $modules['modulename']->requires to get an array of required modules you'd now have to do this $modules['modulename']->requires['modules'] for modules and $modules['modulename']->requires['themes'] for themes, which is useless because modules don't depend on themes but it opens up the ability to do the same with $themes with a consistent interface.

Also, system_rebuild_theme_data() is not currently statically cached currently, whereas system_rebuild_module_data is so if we were to call system_rebuild_theme_data with module dependencies in there we rebuild all module dependencies every single time, bypassing that static cache, which is a bit nasty.

I'd like to suggest that because of this we can't really commit this feature without some static caching on system_rebuild_theme_data() either because we now have to invoke the module dependency system that function becomes much heavier.

sun’s picture

Most of that is going to be resolved by #1067408: Themes do not have an installation status

thedavidmeister’s picture

Hmmm. I got this to the point where the dependencies stuff was working and I just had to update Snugug's UI work to use the new data structure I set up but it looks like something that was committed to system_rebuild_theme_data() in the last week or so is now causing fatal errors in my installation when I checked out the latest version from git :(

As far as I can tell it's because that function was converted to use a new config('system.module')->get('enabled') style format. I might just have to re-install D8 and give it another once-over.

Anyway, here's a patch against the lastest version of D8 so others can see what approach I've been taking.

thedavidmeister’s picture

Ah, before I do any more work here I'm going to apply that patch you referenced in the installation status issue first. There looks to be a lot of good stuff in there :)

thedavidmeister’s picture

I just wanted to mention that I haven't forgotten this thread, there's just a new patch going up on the issue Sun referenced in #46 every couple of days at the moment. I don't think that anything here will be able to be committed cleanly/stably until that thread is closed so I'm waiting for that thread to die down a bit before attempting to re-roll #47 here. Hopefully that happens in the next couple of weeks or we'll very likely miss the feature freeze deadline for this :/

KrisBulman’s picture

The rabbit hole on this issue seems pretty deep at first glance: #1067408: Themes do not have an installation status -> #1798732: Convert install_task, install_time and install_current_batch to use the state system -> #1530756: [meta] Use a proper kernel for the installer

I'm really worried this may not make it in, however sun seems to be quite involved in all issues that this relies on so that's something!

thedavidmeister’s picture

Yeah, me too. But really, the outcome of #1067408: Themes do not have an installation status could easily mean this functionality should be implemented in a completely different way to how the current patches work, and there's no point in duplicating efforts with Sun when he's rolling patches new patches so consistently each time the test bot fails. I think we'll just have to wait and see what happens.

thedavidmeister’s picture

considering how the other threads are going it's highly likely this feature has missed the boat. I'd move it to 9.x-dev right now if the tag existed :/

sun’s picture

Issue tags: -API clean-up +API addition

At this point, I'd recommend to try to move forward independently here.

I'd rather object to the _module_build_dependencies() changes here though - themes are currently using the 'base theme' property only and implement their own dependency checking for that. Thus, the $theme->requires and $theme->required_by properties are not occupied, still available, and can be used to denote module dependencies in the same way as modules are doing.

That should happen in _system_rebuild_theme_data(). You should be able to copy/paste the necessary lines from _system_rebuild_module_data().

thedavidmeister’s picture

But it isn't $theme, it's $files in the _build_dependency functions.

Originally I was going to keep it is $theme->requires and $theme->required_by but then you run into this problem:

I'd highly recommend to defer the implicit base theme as dependencies[] handling to a separate issue. The idea is sound, but it leads to having a theme name in a list of module names - turning the list into a list of extension names.

Allowing themes to declare other themes as dependencies has been requested multiple times in this issue, even if nobody has updated the title of the thread to match. See #7, #8, #11, #17, #19, #25.

It only leads to having a theme name in a list of module names if you don't ask systems that can implement dependencies to namespace themselves somehow. Previously there was only one system that had a _build_dependencies function so it didn't matter. Now there would be a _theme_build_dependencies, which is more or less copied and pasted from _module_build_dependencies but for the theme system.

I didn't think that it was ok for _theme_build_dependencies to do anything at all related to deciding what a "module dependency" meant, only what a "theme dependency" meant. Themes could call _module_build_dependencies and have their info files parsed to discover anything that looks like a "module dependency" later/elsewhere.

I'm kind of just re-hashing what I said in #41 and #45 now.

tl;dr - based on what I found while debugging I don't see how $theme->requires and $theme->required_by would be sufficient to handle the requests coming in from everyone in this thread. Even if we take the "do that in a separate issue approach" we should still be polite enough not to screw whoever works on that thread by taking the good property names in advance. At least, we can't do that AND keep modules and theme dependencies in separate lists as per #24.

Also... considering my personal life commitments this week I don't think I'll get a chance to re-hash that patch before 1st Dec anyway, although I can still join in the discussion here. A re-roll of #47, with the required extra UI functionality and cleanup would be amazing if anyone gets the chance.

thedavidmeister’s picture

jhedstrom’s picture

Version: 8.0.x-dev » 8.1.x-dev

Bumping to 8.1.x.

davidwbarratt’s picture

Version: 8.1.x-dev » 8.0.x-dev
Status: Needs work » Needs review

Moving to Needs Review and 8.0.x to kick off the test bot of the patch submitted in #47.

mgifford’s picture

@davidwbarratt do you want to re-roll the patch or should this be bounced up to 8.1 again?

Fabianx’s picture

Version: 8.0.x-dev » 8.1.x-dev

As feature request this is definitely 8.1.x material.

markhalliwell’s picture

Issue tags: +Needs reroll

I was really giddy when I created and enabled a sub-module that was located inside the bootstrap base theme only to find out that I couldn't actually add said sub-module as a dependency. Doing so causes the theme to become non-installable due to it thinking that it's looking for a theme and not a module.

A little sad that this feature is still out in the ether. I thought it had been taken care of with all the theme services/manager refactoring that took place :-/

dawehner’s picture

FileSize
2.73 KB

Just a quick start so far, nothing elaborated like the last patch. We cannot change the API anymore so i'm not sure adding 'modules' dependencies to modules itself would make sense.
IMHO adding theme dependencies for modules should not be added due to the potential circular dependency issues involved with it.

Miguel.kode’s picture

Status: Needs work » Needs review
FileSize
2.73 KB

This is the re-roll for this patch.

dawehner’s picture

Status: Needs work » Needs review
FileSize
6.68 KB
4.96 KB

There we go.

This uses the implicit assumption that you cannot have themes with the same name as modules, which IMHO is a valid restriction.

star-szr’s picture

Issue tags: -Needs reroll

@dawehner thanks, can you please elaborate on the potential for circular dependencies, maybe give an example?

catch’s picture

I'd want to see the use case for adding dependencies on themes to modules before adding that, seems considerably more obscure than the other way 'round, and hook_requirements() can be used for obscure cases if they exist.

dcrocks’s picture

Is an attempt to install module dependencies the right response to missing dependencies for themes or would an error message be better, giving a themer a chance to install modules thru existing interfaces? Given the potential for specification mistakes, it seems it would be easier to edit a theme info file rather than uninstalling an unwanted module. And given module dependency needs it might be confusing for a themer to get error messages from the module system when messages fro the theme system are expected.

davidwbarratt’s picture

I would assume that themes would always depend on modules, not the other way around. I can't see a scenario in which a module would need to depend on a theme. Is there anything in a theme you can't do with a module? I can't think of anything.

markhalliwell’s picture

Re: module's depending on themes

There is precedence when a module should depend on a theme.

Take for instance https://www.drupal.org/project/bootstrap_core which provides a Bootstrap Carousel Formatter in the sub-module bootstrap_field. The field doesn't work if the Drupal Bootstrap base theme isn't enabled (which provides the bootstrap_carousel theme hook).

Also, my goal in 8.x would be to package bootstrap_core with the base theme itself (instead of having an entirely separate project).

Re: theme's depending on modules
The Drupal Bootstrap base theme does not currently require bootstrap_core, however given that a module has more power (e.g. services) to alter things like properly replacing/extending the theme registry, it would make sense to move that stuff to bootstrap_core and have the Drupal Bootstrap base theme depend on the module as well.

---

I can see the dilemma with how the current dependencies logic makes this a problem. Currently and historically, a dependency means that it's a "hard/required" dependency and should be installed first. Thus, if both the base theme and packaged module depended on each other, this would thus create "circular dependencies".

IMO though, that's really the problem: we're assuming that there's just "one type" of dependency.

The reality is far from this case though. If anything, I would say that we should also have a way to declare "soft" or rather "peer dependencies".

Would the "circular dependencies" issue still technically exist then, sure. However, that would be "works as designed" IMO, especially if we add "peer dependencies" to account for this use case.

When a module/theme combo runs into this issue, they can use "peer dependencies" instead and core would just keep track of them in a "to be installed list" (install after, not before) and just skip when already installed depending on which is installed first.

markhalliwell’s picture

seems considerably more obscure than the other way 'round, and hook_requirements() can be used for obscure cases if they exist.

The problem with this is that this (currently) only applies to modules. Theme's cannot implement hook_requirements() or any install/update hooks for that matter.

Besides, this feels like a very hackish way to do something that core should take into account from the get go IMO. I think the "peer dependencies" solution is a much better approach.

markhalliwell’s picture

Issue tags: +Needs backport to D7

Also, considering that this is purely an addition. I'm tagging in hopes to see this backported (in some fashion, I know it won't be 1:1), but hopeful. This feature can alleviate a lot of issues (that are created by people who don't read project documentation) with themes and modules (e.g. a theme requiring jquery_update).

markhalliwell’s picture

Title: dependencies[] for themes, so they can depend on modules » Allow themes to declare dependencies on modules
Related issues: +#2659938: Allow extensions to declare "peer dependencies"

Just created a similar ticket for the "peer dependencies" concept so it doesn't highjack this issue, which is really about theme's being able to declare dependencies (of any kind) in the first place.

dawehner’s picture

So this issue clearly is about adding module dependencies for themes, not the other way round.

markhalliwell’s picture

Yes, which is why created the other issue.......

I was simply explaining (for @catch above) what the "circular dependency" issue you brought up is and how to "fix it". Yes, even though this issue would technically "create the circular dependency" conundrum, that's really just a byproduct and can be avoided with common sense of how dependencies work. Also, it's not like the "circular dependency" issue wouldn't be very self-evident when a page/drush command fails due to recursive/nested calls.

This is certainly fine with me as long as we plan to address it with "peer dependencies" (or whatever)... thus making the "circular dependencies" a "known/works as designed issue".

RainbowArray’s picture

One note: challenge with modules depending on themes is that a sub theme that extends a parent theme would fail that test even if generally filled the requirements the module needed. Checking the parent tree for a theme could alleviate that I suppose. That could be useful.

Having themes declare module dependencies could be very useful particularly because there are many things a theme just can't do but an associated module could provide.

catch’s picture

The problem with this is that this (currently) only applies to modules. Theme's cannot implement hook_requirements() or any install/update hooks for that matter.

Besides, this feels like a very hackish way to do something that core should take into account from the get go IMO. I think the "peer dependencies" solution is a much better approach.

Yes, I was talking about the obscure case of modules depending on themes. Since that's something that only modules would have a problem with, hook_requirements() is available in that case.

Yes, even though this issue would technically "create the circular dependency" conundrum, that's really just a byproduct and can be avoided with common sense of how dependencies work.

Not if we only add support for themes to depend on modules here.

andypost’s picture

Summary is outdated, not descriptive and there's not enough arguments.
To bundle config and module requirements there's install profiles.

The only dependency for themes is libraries everything other is a distribution/profile.
Knowing an external library that theme require to render "cool slider" is more needed then module deps.

Also enhances[] is good key to point which modules the theme supports

dawehner’s picture

Status: Needs work » Needs review
FileSize
6.71 KB
623 bytes

@markcarver
Oh sorry yeah, partially caused by a browser tab being open for a day or two.

/me sighs about the failure.

RainbowArray’s picture

It is absolutely possible for a theme to need a dependency on a module and not just a library.

A theme geared towards panels-based sites would not make much sense if panels is not installed for example. Lots of possible reasons a theme could need a module.

hass’s picture

A good example is a theme that implements HTML5 stuff and requires https://www.drupal.org/project/elements module.

markhalliwell’s picture

The only dependency for themes is libraries everything other is a distribution/profile.

Assuming that a theme can supplement the need for modules with "libraries" is simply not true and horribly misleading.

As stated above, a very common use case (in 7.x anyway) would be to specify that a theme requires jquery_update (which, in this case is a module not a library). I know jquery_update is touted as "not being needed in 8.x because we promise to update it", but we really don't have any real world data for this yet. Besides, this is just one example. To assume that this is just "one example/special use case" would be rather ignorant.

Just because a theme has the ability to provide libraries (just for itself btw), does not negate the fact that there are often better and more abstract solutions that already exist in existing modules.

However the real, primary, reason a theme would require dependencies on modules is for the fact that themes are continuing to be placed on the chopping block (as evidence by your statement above) for what PHP hook's they're "allowed" to participate in.

If this "idea" continues (as I'm sure it will given that themes are essentially becoming 3rd class citizens), then there's will literally be nothing more a theme can do other than to provide templates (which is horrible mistake IMO, but what do I know).

At the very least, this issue would alleviate a lot of my concerns with this process happening. It would allow themes to be shipped with a companion module that does the "PHP side of things" (see related issue I'm attaching now).

markhalliwell’s picture

Also enhances[] is good key to point which modules the theme supports

The key "enhances" !== "theme supports". Naming things is hard, sure, but I see very little relation between either of those, let alone what this issue is attempting to accomplish.

This issue is about declaring a hard requirement for a module in a theme (e.g. installing it when the theme is installed or preventing the installation of a theme until the module is at least present).

markhalliwell’s picture

Re: @catch in #80:

Not if we only add support for themes to depend on modules here.

What I was also trying to get at is that creating special cases around extension dependencies is pointless (profiles/modules/themes, etc.). They're all extensions and how we handle dependencies should work the same for all of them.

dawehner’s picture

What I was also trying to get at is that creating special cases around extension dependencies is pointless (profiles/modules/themes, etc.). They're all extensions and how we handle dependencies should work the same for all of them.

I can see where you are coming from, but when we do that, we need a way better approach, because we really want to avoid potential circular dependencies, or at least detect them. So in order to do that we would need like a global dependency tree of all the extensions. As you could imagine, this is not just a small change like this issue and needs more throughout thoughts.

For me there are other reasons why we don't want to support a module depending on a profile. Some people consider it as a good idea, to vendor login users of their distribution to their distribution, by trying to put in dependencies from their custom modules to their install profile, without a technical reason underneath it. Being able to make that impossible from the system itself, is IMHO an excellent feature, given our promised openness.

Version: 8.1.x-dev » 8.2.x-dev

Drupal 8.1.0-beta1 was released on March 2, 2016, which means new developments and disruptive changes should now be targeted against the 8.2.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

dawehner’s picture

Let's now also display the dependencies ...

SKAUGHT’s picture

JohnAlbin’s picture

Real world example: setting Zen 8.x-7.x's STARTERKIT theme to be the default theme will now WSOD your site if you don't install https://www.drupal.org/project/components first. All I'm able to do in drupal 8.1.x is to hack the .info description to put a big red WARNING on it.

Screenshot of Appearance page showing a warning above the "set as default" link on Zen's starter kit.

almaudoh’s picture

+++ b/core/modules/system/templates/system-themes-page.html.twig
@@ -66,6 +66,9 @@
+              {% if theme.requires  %}
+                <span>Module dependencies: </span><span>{{ theme.requires | |keys | safe_join(', ')}}</span>
+              {% endif %}

This looks like a typo: ...| |keys...

almaudoh’s picture

Fixed #94

almaudoh’s picture

Status: Needs work » Needs review
dawehner’s picture

So yeah IMHO we need to figure out the admin UI, as well as validating the dependencies during install time.

SKAUGHT’s picture

to note: this issue and Add .install file abilities for Themes are really tied together.

@dawehner
With the addition to a theme having a .install file, then that install would have a hook_requirements() function to allow for all the rest of that.

dawehner’s picture

@SKAUGHT
I totally get the angle you are coming from.

Having a semantic relationship between theme and its dependencies is IMHO a big advantage as for example tools on d.o. could leverage that information etc. Using install and requirement is just a workaround for me.

SKAUGHT’s picture

(: we skip across the issue that themes and modules aren't so far apart to begin with. 'The Themer' conundrum.

to add to the mix: Unify & simplify render & theme system: component-based rendering maybe things will be changing in a much different way sooner than later.

SKAUGHT’s picture

@JohnAlbin :: #93

LOL.. html inline in the info message. i've don't that myself in the past. yep, hacky.

SKAUGHT’s picture

JohnAlbin’s picture

Status: Needs work » Needs review
FileSize
8.07 KB

Re-rolled #95 by applying it to latset 8.1.x and then cherry-picking onto 8.2.x. No other changes.

jhedstrom’s picture

Assigned: Unassigned » jhedstrom

Going to see if I can fix the tests. The failures appear to be related to the conversion of kernel tests to the new phpunit-based version.

jhedstrom’s picture

Indeed it was. Any kernel test requiring the system module now needs to explicitly enable that module after the test base change (#2687897: Convert system module's kernel tests to NG).

JohnAlbin’s picture

FileSize
19.41 KB
13.84 KB

Following the previous patches, this patch will automatically install any disabled modules when clicking the "Install" link under a disabled theme. Alternatively, we could tell users they need to ensure all required modules are installed before giving them the "install" link.

This patch has a @TODO where we need to check for theme dependencies on the modules page. It still needs to show which themes depend on modules and needs to prevent uninstall of modules that have theme dependencies.

Wim Leers’s picture

  1. +++ b/core/lib/Drupal/Core/Extension/ThemeHandler.php
    @@ -272,6 +272,7 @@ public function rebuildThemeData() {
    +      'dependencies' => array(),
         );
     
         $sub_themes = array();
    @@ -358,6 +359,11 @@ public function rebuildThemeData() {
    
    @@ -358,6 +359,11 @@ public function rebuildThemeData() {
           }
         }
     
    +    // Store the list of module dependencies separately from the full requires list.
    +    foreach ($themes as $key => $theme) {
    +      $themes[$key]->module_dependencies = isset($theme->base_themes) ? array_diff_key($theme->requires, $theme->base_themes) : $theme->requires;
    +    }
    

    I get the addition of 'dependencies' => array(). This ensures the new dependencies key in theme *.info.yml files has a defautl value.

    But I don't understand why I don't see dependencies in that second hunk. Presumably because the code in between reuses a lot of the generic info file parsing, which maps dependencies to requires?

    (I do see that base theme is mapped to dependencies already in HEAD: $themes[$key]->info['dependencies'][] = $themes[$key]->info['base theme'];.)

    I do suspect that this is why the patch is failing.

  2. +++ b/core/modules/system/src/Form/ModulesListForm.php
    @@ -187,10 +187,45 @@ public function buildForm(array $form, FormStateInterface $form_state) {
       /**
    +   * @param array $modules
    

    Missing description.

  3. +++ b/core/modules/system/src/Form/ModulesListForm.php
    @@ -187,10 +187,45 @@ public function buildForm(array $form, FormStateInterface $form_state) {
    +   * @param $dependency
    ...
    +   * @param $version
    

    Nit: Typehint to string?

  4. +++ b/core/modules/system/src/Form/ModulesListForm.php
    @@ -187,10 +187,45 @@ public function buildForm(array $form, FormStateInterface $form_state) {
    +   *   Version information from the module whose dependency we are checking.
    

    Nit: s/from/of/

  5. +++ b/core/modules/system/src/Form/ModulesListForm.php
    @@ -187,10 +187,45 @@ public function buildForm(array $form, FormStateInterface $form_state) {
    +   * @return
    

    @return string|null

  6. +++ b/core/modules/system/src/Form/ModulesListForm.php
    @@ -187,10 +187,45 @@ public function buildForm(array $form, FormStateInterface $form_state) {
    +    // Only display visible modules.
    +    elseif (empty($modules[$dependency]->hidden)) {
    
    @@ -297,31 +332,14 @@ protected function buildRow(array $modules, Extension $module, $distribution) {
           // Only display visible modules.
           elseif (empty($modules[$dependency]->hidden)) {
    

    This logic is now repeated. Which indicates that the moving of part of this logic into a helper function has made this even more difficult to understand. This needs further attention.

  7. +++ b/core/modules/system/system.admin.inc
    @@ -225,6 +225,7 @@ function template_preprocess_system_modules_details(&$variables) {
    +    // @TODO: Add theme dependencies.
    

    This TODO must be resolved before this can e committed.

    In other words: we need to make sure that $module['#required_by'] can also list themes that require a module.

    #required_by is set in \Drupal\system\Form\ModulesListForm::buildRow()

  8. +++ b/core/modules/system/tests/themes/test_theme_module_dependency/test_theme_dependency/src/Service.php
    @@ -0,0 +1,12 @@
    +/**
    + * @file
    + * Contains \Drupal\test_theme_dependency\Service.
    + */
    

    No longer necessary.

  9. +++ b/core/tests/Drupal/KernelTests/Core/Theme/ThemeInstallerTest.php
    @@ -350,6 +351,15 @@ function testThemeInfoAlter() {
    +    $this->themeInstaller()->install(['test_theme_module_dependency']);
    

    test_theme_depending_on_module would be clearer I think.

JohnAlbin’s picture

Status: Needs work » Needs review
FileSize
13.23 KB
22.84 KB
13.23 KB

Fixed 1-6, 8-9. Uploading new patch to see if I fixed the broken test.

I'll circle back to #7 later if someone else (you, dear reader?) doesn't grab it sooner.

JohnAlbin’s picture

Status: Needs work » Needs review
FileSize
22.88 KB
719 bytes

Fixing test failure.

Incidentally, whenever I try to test this locally, all tests fail with "PDOException: SQLSTATE[HY000] [2002] No such file or directory ". What setup step have I missed?

Version: 8.2.x-dev » 8.3.x-dev

Drupal 8.2.0-beta1 was released on August 3, 2016, which means new developments and disruptive changes should now be targeted against the 8.3.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Fabianx’s picture

Status: Needs review » Needs work
+++ b/core/modules/system/system.admin.inc
@@ -225,6 +225,7 @@ function template_preprocess_system_modules_details(&$variables) {
+    // @TODO: Add theme dependencies.
     if (!empty($module['#required_by'])) {

This is the todo that still needs to be resolved.

Therefore setting to CNW.

Version: 8.3.x-dev » 8.4.x-dev

Drupal 8.3.0-alpha1 will be released the week of January 30, 2017, which means new developments and disruptive changes should now be targeted against the 8.4.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

keithm’s picture

@JohnAlbin #474684-114: Allow themes to declare dependencies on modules

Incidentally, whenever I try to test this locally, all tests fail with "PDOException: SQLSTATE[HY000] [2002] No such file or directory ". What setup step have I missed?

Try changing 'host' => '127.0.0.1' in your settings.php file. Without that change Drupal runs for me but tests don't. This change fixed that. Seems to be related to #2610858: Add informative error message for 'Connection refused' errors in MySQL.

Version: 8.4.x-dev » 8.5.x-dev

Drupal 8.4.0-alpha1 will be released the week of July 31, 2017, which means new developments and disruptive changes should now be targeted against the 8.5.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

sun’s picture

The latest patch only contains a test to assert that a required module gets enabled together with a theme upon enabling the theme.

However, as a theme developer, my first and foremost expectations for this feature would be:

  1. A theme cannot be enabled if the dependencies are not met; i.e., no operation links to install/enable.
  2. A corresponding status info/notice appears in the user interface / command line interface.
  3. An appropriate error message informs me about which dependencies are missing, if I try and find a way to request an installation regardless of that info.

In short, the application should not allow someone to install my theme if the necessary modules do not exist.

Point in case being: My theme can actively depend on modules today already. But there's no mechanism anywhere that protects users from weird results / fatal errors / WSODs right now. That's the "feature" I'm looking for as a theme developer/creator/maintainer, both regarding publicly and privately shared themes.

A prime example for required dependencies of a theme would be https://www.drupal.org/project/components

The functional code of this patch seems to cover all or most of this already. But these basic expectations should also be covered by tests.

Wim Leers’s picture

@sun++

chr.fritsch’s picture

Sorry for the noise, but welcome back in the issue queue @sun 😊

dbazuin’s picture

Subscribing

camoa’s picture

I like @sun++ approach.

That way we can name the dependencies on a theme info file and make sure they are met before the theme can be enabled.

This way we avoid the issue of throwing themes into the mix of uninstalling a module but we can ensure a module is on when the theme is enabled.

yay for https://www.drupal.org/project/components as well.

kclarkson’s picture

hilarious that I was just thinking about using components and went to the info.yml file of the theme looking for place to add the module dependency.

I will stay tuned to this great feature request!

I love Drupal :)

dawehner’s picture

Status: Needs work » Needs review
FileSize
23.58 KB
10.34 KB

I've expanded the test coverage with a case of a theme with missing dependencies.

dawehner’s picture

I actually forgot to add the new test in the latest patch.

markhalliwell’s picture

What's needed to get this in? Bootstrap really, really, needs this.

Version: 8.5.x-dev » 8.6.x-dev

Drupal 8.5.0-alpha1 will be released the week of January 17, 2018, which means new developments and disruptive changes should now be targeted against the 8.6.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

dawehner’s picture

Issue tags: +Needs change record

@markcarver
I pinged @joelpittet for a review on this issue, but yeah that's basically it I guess :)

Wim Leers’s picture

Trying to find any problems that a committer might point out, including nits.

  1. +++ b/core/lib/Drupal/Core/Extension/ThemeInstaller.php
    @@ -61,6 +62,14 @@ class ThemeInstaller implements ThemeInstallerInterface {
    +   * The module installer used to install module dependent by theme.
    

    "used to install module dependent by theme" is wrong.

    What about "used to install modules depended on by themes"?

  2. +++ b/core/lib/Drupal/Core/Extension/ThemeInstaller.php
    @@ -61,6 +62,14 @@ class ThemeInstaller implements ThemeInstallerInterface {
    +  protected $moduleInstaller;
    +
    +
       /**
    

    Nit: one blank line too many.

  3. +++ b/core/modules/system/src/Controller/ThemeController.php
    @@ -106,9 +106,24 @@ public function install(Request $request) {
    +            drupal_set_message($this->formatPlural(count($newly_installed_modules_names), 'The %theme theme and its module dependency, %name, have been installed.', 'The %theme theme and its @count module dependencies have been installed: %names', [
    

    Missing trailing period in the last UI string.

  4. +++ b/core/modules/system/src/Controller/ThemeController.php
    @@ -106,9 +106,24 @@ public function install(Request $request) {
    +            drupal_set_message($this->t('The %theme theme has been installed.', ['%theme' => $theme_data[$theme]->info['name']]));
    

    Should use the messenger service per https://www.drupal.org/node/2774931.

  5. +++ b/core/modules/system/src/Form/ModulesListForm.php
    @@ -190,11 +190,49 @@ public function buildForm(array $form, FormStateInterface $form_state) {
    +   * Checks a single module dependency of a module or theme that has the given
    +   * module version requirements.
    

    Should fit on a single line per our cs.

  6. +++ b/core/modules/system/src/Form/ModulesListForm.php
    @@ -190,11 +190,49 @@ public function buildForm(array $form, FormStateInterface $form_state) {
    +   * @param string $dependency
    +   *   The dependency to check.
    

    This only supports module dependencies. The parameter name and description suggest it also supports other types of extensions.

  7. +++ b/core/modules/system/src/Form/ModulesListForm.php
    @@ -190,11 +190,49 @@ public function buildForm(array $form, FormStateInterface $form_state) {
    +      return t('@module (<span class="admin-missing">missing</span>)', ['@module' => Unicode::ucfirst($dependency)]);
    ...
    +        return t('@module (<span class="admin-missing">incompatible with</span> version @version)', [
    

    I was surprised to see the admin-missing class for both the "missing" and "incompatible" cases.

    But \Drupal\system\Form\ModulesListForm::buildRow() already does this in HEAD, so this is fine (in fact, this code was extracted from there, so no change at all).

  8. +++ b/core/modules/system/system.admin.inc
    @@ -165,6 +165,7 @@ function template_preprocess_system_modules_details(&$variables) {
    +    // @TODO: Add theme dependencies.
    

    Has this been addressed? If not, can we create an issue to link to?

  9. +++ b/core/modules/system/system.admin.inc
    @@ -290,6 +291,15 @@ function template_preprocess_system_themes_page(&$variables) {
    +      $current_theme['module_dependencies'] = [];
    

    This is not used anywhere AFAICT?

dawehner’s picture

Issue summary: View changes
Issue tags: -Needs issue summary update, -Needs change record
FileSize
27.5 KB
5.02 KB

Thank you wim!

"used to install module dependent by theme" is wrong.

What about "used to install modules depended on by themes"?

Ha, I thought this read odd when I wrote it, but well, I couldn't motivate myself to ask anyone for an alternative. Thank you wim!

Missing trailing period in the last UI string.

Done.

was surprised to see the admin-missing class for both the "missing" and "incompatible" cases.

But \Drupal\system\Form\ModulesListForm::buildRow() already does this in HEAD, so this is fine (in fact, this code was extracted from there, so no change at all).

Thank you for figuring this out!

Has this been addressed? If not, can we create an issue to link to?

Good point. I added a followup to address this.

This is not used anywhere AFAICT?

It is totally used in the template ... core/modules/system/templates/system-themes-page.html.twig

dawehner’s picture

Status: Needs work » Needs review
FileSize
27.51 KB
1.25 KB

oops

dillix’s picture

Is there a chance to get this committed into 8.5.0?

dpagini’s picture

kevineinarsson’s picture

The patch in #137 is missing the new/untracked files. Just adding them back to queue it for testing, as well as fixing a trailing whitespace error Git was pointing out.

dawehner’s picture

Assigned: Unassigned » dawehner

Looking at the remaining failures

kalpaitch’s picture

There's an assumption that any theme dependencies will be newly installed:

diff -u b/core/modules/system/src/Controller/ThemeController.php b/core/modules/system/src/Controller/ThemeController.php
--- b/core/modules/system/src/Controller/ThemeController.php
+++ b/core/modules/system/src/Controller/ThemeController.php
@@ -127,6 +127,8 @@
             $newly_installed_modules_names = array_map(function ($module_name) use ($module_data) {
               return $module_data[$module_name]->info['name'];
             }, array_keys($newly_installed_modules));
+          }
+          if (!empty($newly_installed_modules)) {
             $this->messenger->addStatus($this->formatPlural(count($newly_installed_modules_names), 'The %theme theme and its module dependency, %name, have been installed.', 'The %theme theme and its @count module dependencies have been installed: %names.', [
               '%theme' => $theme_data[$theme]->info['name'],
               '%name' => $newly_installed_modules_names[0],

But the failing tests are down to the message above being dropped before the next page load.

kevineinarsson’s picture

The failing tests are down to the kernel rebuild that happens when installing a module. This breaks the reference between from messenger service to the flashbag ($_SESSION['_symfony_flashes']). For the time being and to keep this issue moving, requesting the messaging service anew after a module install happens "solves" the issue. So instead of using $this->messenger after a module install, using \Drupal::service('messenger').

kalpaitch’s picture

For sure, seems to me the simplest option is to continue with the deprecated `drupal_set_message()` until this issue is resolved.

dawehner’s picture

Nice research @kalpaitch!

+++ b/core/modules/system/src/Controller/ThemeController.php
@@ -24,16 +25,26 @@ class ThemeController extends ControllerBase {
+   * The messenger.
+   *
+   * @var \Drupal\Core\Messenger\MessengerInterface
+   */
+  protected $messenger;
+
...
+   * @param \Drupal\Core\Messenger\MessengerInterface $messenger
+   *   The messenger.
    */
-  public function __construct(ThemeHandlerInterface $theme_handler, ConfigFactoryInterface $config_factory) {
+  public function __construct(ThemeHandlerInterface $theme_handler, ConfigFactoryInterface $config_factory, MessengerInterface $messenger) {
     $this->themeHandler = $theme_handler;
     $this->configFactory = $config_factory;
+    $this->messenger = $messenger;

@@ -42,7 +53,8 @@ public function __construct(ThemeHandlerInterface $theme_handler, ConfigFactoryI
+      $container->get('messenger')

Do you mind opening up an issue so someone has a deeper link into it? This would be amazing! We can remove this change from the file.

kalpaitch’s picture

Issue raised #2940148: Messenger service can't set messages super early and loses reference to flashbag reference on rebuild

Injected messenger service removed for the time being.

Tests updated to check for themes with already installed module dependencies as per comment #140

kalpaitch’s picture

Oopsie woopsie, let's just ignore that last one.

markhalliwell’s picture

+++ b/core/lib/Drupal/Core/Extension/ThemeInstaller.php
@@ -96,6 +104,7 @@ public function __construct(ThemeHandlerInterface $theme_handler, ConfigFactoryI
+    $this->moduleInstaller = $module_installer ?: \Drupal::service('module_installer');

This shouldn't invoke the static service get in the constructor.

Instead, there should be a getModuleInstaller helper method added that invokes it there if it wasn't injected. Thus, all subsequent $this->moduleInstaller->* code should be $this->getModuleInstaller()->*.

dawehner’s picture

@kalpaitch Nice research! It felt right to nerd snipe you with this issue.

+++ b/core/lib/Drupal/Core/Extension/ThemeInstaller.php
@@ -96,6 +104,7 @@ public function __construct(ThemeHandlerInterface $theme_handler, ConfigFactoryI
+ $this->moduleInstaller = $module_installer ?: \Drupal::service('module_installer');
This shouldn't invoke the static service get in the constructor.

Do you mind elaborating why you think this change is needed? I know we are doing the same pattern all over the place in core, and I haven't seen an issue with it, yet. Feel free to use ag "this->" | grep " = " | grep "Drupal::" | grep -v "Test"

+++ b/core/lib/Drupal/Core/Extension/ThemeHandler.php
@@ -364,6 +365,17 @@ public function rebuildThemeData() {
+    foreach ($themes as $key => $theme) {
+      // buildModuleDependencies() adds a theme->requires array that contains
+      // both module and base theme dependencies, if they are specified. Ensure
+      // that every theme stores the list of module dependencies separately
+      // from the full requires list.
+      if (!isset($theme->requires)) {
+        $theme->requires = [];
+      }
+      $themes[$key]->module_dependencies = isset($theme->base_themes) ? array_diff_key($theme->requires, $theme->base_themes) : $theme->requires;
+    }
+

I'm curious, should we add a test to \Drupal\Tests\Core\Extension\ThemeHandlerTest for this addition?

dawehner’s picture

Assigned: dawehner » Unassigned
FileSize
30.06 KB
3.52 KB

The alternative way to fix it would be something like this. This mirrors what is done in the module installer. Does that make sense to you?
I think though for this issue it is fine to not start injecting the messenger service.

markhalliwell’s picture

Status: Needs work » Needs review

Let's see what #148 comes back with.

Do you mind elaborating why you think this change is needed?

From my experience, using the static \Drupal::service() helper method inside constructors can still sometimes lead to race conditions or other unforeseen issues down the road.

It's usually best to either have it as a complete hard dependency injected service or if it's not a hard dependency, creating a getter method which calls the static helper method only at that point to set the service if it wasn't injected... well after the class instance has been created.

That being said, it's still not quite clear to me why the ModuleInstaller service isn't a hard dependency injected service and allowing NULL as a default value.

While we certainly don't really risk any race conditions here since ModuleInstaller doesn't require ThemeInstaller, it's still an anti-pattern as \Drupal::service() was not really intended to be used in this fashion and kind of defeats the whole point of dependency injection.

dawehner’s picture

Status: Needs work » Needs review
FileSize
31.1 KB
2.41 KB

From my experience, using the static \Drupal::service() helper method inside constructors can still sometimes lead to race conditions or other unforeseen issues down the road.

It is a problem in the case of a low level services, I'd argue the theme installer isn't, but sure, let's be rather save than sorry. @markcarver Do you mind expanding the issue summary explaining for the committer, why we are doing that?

markhalliwell’s picture

Status: Needs review » Needs work

Do you mind expanding the issue summary explaining for the committer, why we are doing that?

I'm not sure I can. Why can't we just properly inject the service in the first place? Why is a helper method needed at all? This reason hasn't been documented in the code and I'm beginning to think that maybe it was just an accident and should be corrected.

dawehner’s picture

I'm not sure I can. Why can't we just properly inject the service in the first place? Why is a helper method needed at all? This reason hasn't been documented in the code and I'm beginning to think that maybe it was just an accident and should be corrected.

Well, we want to prevent the case that someone has subclasses the file and adapted the services.yml file for that. By falling back to \Drupal::service() we would prevent that. This is absolutely not a new idea and is done all over the place, as said before. I am a bit confused why we cannot just accept this common practise.

Pol’s picture

I just tested the patch against 8.5.x and it's working pretty fine.

screenshot

SKAUGHT’s picture

--edit.

#154 Pol: do you have a sandbox of the "openEuropa Theme". a common tool for manual testing would be good.

dawehner’s picture

I'm not sure I can. Why can't we just properly inject the service in the first place?

The problem is with just injecting: Someone extended the class and calls the parent constructor without the additional argument. By falling back to the \Drupal::service() call, you avoid this problem. On most sites, aka. the sites which don't have the module installed, the value would be passed along properly and life is totally fine.

markhalliwell’s picture

Hm. I guess. I thought that's why __construct is never added to interfaces (in case of service dependency changes). If you're saying this is a wide-spread practice (you would know more than I), then I defer to your judgment. I just thought I'd bring it up as it's something I've noticed from time to time is all.

dawehner’s picture

Yeah, no worries :) I've totally been down in the darkness of loops in service creation.
Are you okay with dropping the additional helper method again?

markhalliwell’s picture

Sure, I just want to get this in ASAP (and even 8.5.x if possible)

Pol’s picture

@SKAUGHT: We are early in the development of it, but we have a nice working solution based on Docker. Find all the doc in the README of the project: https://github.com/ec-europa/oe_theme

dawehner’s picture

Status: Needs work » Needs review
FileSize
30.82 KB
1.46 KB

@markcarver
Sure, let's give it a try.

larowlan’s picture

Thanks for working on this, its really needed. Some observations

  1. +++ b/core/modules/system/src/Controller/SystemController.php
    @@ -232,9 +234,37 @@ public function themesPage() {
    +            $theme->incompatible_module = TRUE;
    ...
    +          elseif (!empty($modules[$dependency]->hidden)) {
    

    We could continue here and avoid the elseif, it could be just a standard if.

  2. +++ b/core/modules/system/src/Controller/SystemController.php
    @@ -232,9 +234,37 @@ public function themesPage() {
    +              $theme->module_dependencies[$dependency] = $this->t('@module', array('@module' => $name));
    ...
    +              $theme->module_dependencies[$dependency] = $this->t('@module (<span class="admin-disabled">disabled</span>)', array('@module' => $name));
    

    we need short array syntax here

  3. +++ b/core/modules/system/src/Controller/ThemeController.php
    @@ -106,9 +131,27 @@ public function install(Request $request) {
    +        $previously_installed_modules = \Drupal::moduleHandler()->getModuleList();
    

    Given we inject the kernel, can we inject this too?

  4. +++ b/core/modules/system/src/Controller/ThemeController.php
    @@ -106,9 +131,27 @@ public function install(Request $request) {
    +            drupal_set_message($this->formatPlural(count($newly_installed_modules_names), 'The %theme theme and its module dependency, %name, have been installed.', 'The %theme theme and its @count module dependencies have been installed: %names.', [
    ...
    +            drupal_set_message($this->t('The %theme theme has been installed.', ['%theme' => $theme_data[$theme]->info['name']]));
    

    we inject the messenger but don't use it?

  5. +++ b/core/modules/system/src/Form/ModulesListForm.php
    @@ -190,11 +190,48 @@ public function buildForm(array $form, FormStateInterface $form_state) {
    +      // Disable the checkbox if the dependency is incompatible with this
    +      // version of Drupal core.
    

    we're not really disabling a checkbox anymore

  6. +++ b/core/modules/system/src/Form/ModulesListForm.php
    @@ -190,11 +190,48 @@ public function buildForm(array $form, FormStateInterface $form_state) {
    +      elseif ($modules[$dependency]->info['core'] != \Drupal::CORE_COMPATIBILITY) {
    

    the previous if returned, so we can use just plain if here, no need for elseif

  7. +++ b/core/modules/system/src/Form/ModulesListForm.php
    @@ -301,35 +338,20 @@ protected function buildRow(array $modules, Extension $module, $distribution) {
    +        if ($incompatible = $this->checkDependencyMessage($modules, $dependency, $version)) {
    

    should we call this statically to make it clear there is no mutation of $this?

  8. +++ b/core/modules/system/system.admin.inc
    @@ -310,6 +321,9 @@ function template_preprocess_system_themes_page(&$variables) {
           elseif (!empty($theme->incompatible_engine)) {
    ...
    +      elseif (!empty($theme->incompatible_module)) {
    

    What if the theme fails to satisfy more than one of these conditions. In theory it could have all three (incompatible core, incompatible engine and missing dependencies). Are we sure if/elseif/elseif is right?

  9. +++ b/core/modules/system/tests/src/Functional/Theme/ThemeUiTest.php
    @@ -0,0 +1,74 @@
    +    $this->getSession()->getDriver()->click('//h3[contains(text(), "test theme depending on module")]/../ul/li[1]/a');
    ...
    +    $this->getSession()->getDriver()->click('//h3[contains(text(), "test theme depending on already installed module")]/../ul/li[1]/a');
    

    Might be easier to maintain in the long-run if we used the CssSelector component here? Xpath tends to scare people off

dillix’s picture

@larowlan 9. in #162 may not work on languages other then English, so I think we should avoid it.

dawehner’s picture

Thank you for your review @larowlan!

We could continue here and avoid the elseif, it could be just a standard if.

Nice suggestion

Given we inject the kernel, can we inject this too?

All we really need to ensure is that the refresh logic works. Nice catch

we inject the messenger but don't use it?

Let's remove it for now, see https://www.drupal.org/project/drupal/issues/2940148

we're not really disabling a checkbox anymore

Fair

What if the theme fails to satisfy more than one of these conditions. In theory it could have all three (incompatible core, incompatible engine and missing dependencies). Are we sure if/elseif/elseif is right?

Sure this could be the case beforehand too. There are 5 other conditions already. Would it be a BC break by passing down a render array here instead of a simple string?

Might be easier to maintain in the long-run if we used the CssSelector component here? Xpath tends to scare people off

I tried hard to get it working with pure CSS selectors. I have no idea how to reliable target these links without hardcoding the position of the theme in the list.

@larowlan 9. in #162 may not work on languages other then English, so I think we should avoid it.

I don't think test strings for translations matter in tests :)

pfrenssen’s picture

FileSize
30.71 KB
3.72 KB

This needed a reroll since #2935256: Remove all usages of drupal_get_message and drupal_set_message in modules went in. I also updated two lines of documentation with missing periods. I solved the merge conflict using a rebase so I couldn't generate a normal interdiff, but I provided a diff between the current patch and the previous one, this also shows the lines that were changed.

borisson_’s picture

Status: Needs review » Needs work
  1. +++ b/core/modules/system/src/Controller/SystemController.php
    @@ -232,9 +234,39 @@ public function themesPage() {
    +          } else {
    

    Coding standards: else should be on the next line.

  2. +++ b/core/modules/system/src/Controller/ThemeController.php
    @@ -30,10 +40,13 @@ class ThemeController extends ControllerBase {
    -  public function __construct(ThemeHandlerInterface $theme_handler, ConfigFactoryInterface $config_factory) {
    +  public function __construct(ThemeHandlerInterface $theme_handler, ConfigFactoryInterface $config_factory, DrupalKernelInterface $kernel) {
    
    @@ -42,7 +55,9 @@ public function __construct(ThemeHandlerInterface $theme_handler, ConfigFactoryI
       public static function create(ContainerInterface $container) {
         return new static(
           $container->get('theme_handler'),
    -      $container->get('config.factory')
    +      $container->get('config.factory'),
    +      $container->get('kernel'),
    +      $container->get('messenger')
    

    This doesn't seem to match anymore.

  3. +++ b/core/modules/system/src/Controller/ThemeController.php
    @@ -106,9 +122,28 @@ public function install(Request $request) {
    +        // Ensure we always have the latest.
    +        $previously_installed_modules = $this->moduleHandler()->getModuleList();
    

    I think there are words missing in this comment.

  4. +++ b/core/tests/Drupal/KernelTests/Core/Theme/ThemeInstallerTest.php
    @@ -350,6 +351,15 @@ public function testThemeInfoAlter() {
    +  public function testThemeWithModuleDependency() {
    

    Needs docblock

pfrenssen’s picture

Status: Needs work » Needs review
FileSize
30.81 KB
3.66 KB

Addressed remarks from @borisson_.

borisson_’s picture

Status: Needs review » Reviewed & tested by the community

Thanks @pfrenssen! This looks good now, I can't find any other nits to pick and the patch has sufficient test-coverage.

fgm’s picture

+++ b/core/lib/Drupal/Core/Extension/ThemeInstaller.php
@@ -301,4 +319,11 @@ protected function systemListReset() {
+    $this->themeHandler = \Drupal::service('theme_handler');

Any reason why this method uses this global accessor instead of the injected theme_handler service ?

dawehner’s picture

@fgm
Sure, it is documented 3 lines above in the code:

+++ b/core/lib/Drupal/Core/Extension/ThemeInstaller.php
@@ -301,4 +319,11 @@ protected function systemListReset() {
+  /**
+   * Refresh services after container rebuild.
+   */
+  protected function refreshServices() {
+    $this->themeHandler = \Drupal::service('theme_handler');
+  }
+
 }

This is the full context. We want to reload the service after the installation of the theme.

markhalliwell’s picture

Status: Reviewed & tested by the community » Needs work
+++ b/core/modules/system/src/Form/ModulesListForm.php
@@ -190,11 +190,47 @@ public function buildForm(array $form, FormStateInterface $form_state) {
+      return t('@module (<span class="admin-missing">missing</span>)', ['@module' => Unicode::ucfirst($dependency)]);
...
+        return t('@module (<span class="admin-missing">incompatible with</span> version @version)', [
...
+        return t('@module (<span class="admin-missing">incompatible with</span> this version of Drupal core)', [

@@ -301,35 +337,20 @@ protected function buildRow(array $modules, Extension $module, $distribution) {
+            $row['#requires'][$dependency] = $this->t('@module', ['@module' => $name]);
...
+            $row['#requires'][$dependency] = $this->t('@module (<span class="admin-disabled">disabled</span>)', ['@module' => $name]);

These shouldn't be putting static markup into translations. It should really be turned into proper render arrays.

markhalliwell’s picture

Assigned: Unassigned » markhalliwell

I'll take a quick stab at it.

markhalliwell’s picture

Assigned: markhalliwell » Unassigned
Status: Needs work » Reviewed & tested by the community
Related issues: +#2937952: Show theme dependencies on the module page

Bleh, it's too tightly coupled with modules ATM. The entire way we convey status messages like that needs to be refactored, which should really be a follow-up issue of its own, maybe perhaps a part of #2937952: Show theme dependencies on the module page.

Setting back to RTBC since I didn't really find anything else with the patch wrong.

dawehner’s picture

Thank you @markcarver. I totally agree with you. The way how some of these admin pages are build, is less than ideal :)

fgm’s picture

@dawehner: thanks, I had read the phpdoc before asking, but hadn't realized what made this actually refresh the service.

So I looked throughout core, and found zero occurrence of this pattern (looking for similar phpdocs, so I may have missed some). The closest practice I found is in EntityKernelTestBase and EntityUnitTestBase, both of which do not directly refresh a single service, but fetch the new container (and store it), then take their service from there.

What makes me a bit uneasy at this point is the fact that after this call, the installer instance carries both services from the old container, from the constructor, and the theme handler from the new container, from this call. Shouldn't we be refreshing all services ?

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
  1. It's really good to see all the test coverage. I haven't quite worked out yet whether all the new conditions are tested though.
  2. +++ b/core/lib/Drupal/Core/Extension/ThemeInstaller.php
    @@ -96,6 +106,7 @@ public function __construct(ThemeHandlerInterface $theme_handler, ConfigFactoryI
    +    $this->moduleInstaller = $module_installer ?: \Drupal::service('module_installer');
    

    We should trigger a silenced deprecation if the module handler is not passed in. See https://www.drupal.org/core/deprecation. We should also update core so that this deprecation is not triggered.

  3. +++ b/core/lib/Drupal/Core/Extension/ThemeInstaller.php
    @@ -301,4 +319,11 @@ protected function systemListReset() {
    +  /**
    +   * Refresh services after container rebuild.
    +   */
    +  protected function refreshServices() {
    +    $this->themeHandler = \Drupal::service('theme_handler');
    +  }
    

    This should copy the code from \Drupal\Core\Config\ConfigImporter::reInjectMe() - that should probably be a trait (i think there is an issue somewhere). But it is possible that the module install has resulted in different services so we need to refresh all of them. This will help us avoid super hard to track down bugs.

  4. +++ b/core/modules/system/src/Controller/ThemeController.php
    @@ -190,4 +224,15 @@ public function setDefaultTheme(Request $request) {
    +  /**
    +   * Refresh services after container rebuild.
    +   */
    +  protected function refreshServices() {
    +    $container = $this->kernel->getContainer();
    +    $this->messenger = $container->get('messenger');
    +    $this->themeHandler = $container->get('theme_handler');
    +    $this->configFactory = $container->get('config.factory');
    +    $this->moduleHandler = $container->get('module_handler');
    +  }
    

    Similar to the other comment.

  5. Manual testing has shown that I can uninstall a module that is depended on by an installed theme. The dependency needs to be enforced or you might break your site.
  6. If a theme is going to enable a module there ought to be a confirm form that informs the user this is going to occur. This would match what happens when you install a module and there are extra dependencies to install that the user has not checked.
markhalliwell’s picture

This should copy the code from \Drupal\Core\Config\ConfigImporter::reInjectMe() - that should probably be a trait (i think there is an issue somewhere).

I think it's a part of #2380293: Properly inject services into ModuleInstaller, which was reverted. The latest patch contains UpdateDependenciesTrait.

I'll see what I can do to address #176 though.

markhalliwell’s picture

Ok, I was able to address #176.2-4.

#176.5 & 6 still needs to be done, but will require a little more time than I have right now.

Maybe someone else can tackle it.

markhalliwell’s picture

Status: Needs work » Needs review

Setting to CNR to trigger testing, will set it back to CNW to address #176.5 & 6.

ademarco’s picture

Status: Needs review » Needs work
FileSize
4.24 KB
34.26 KB

We have found that, when a theme depends on more than one module, it would enable all its dependencies in $this->moduleInstaller->install(array_keys($module_dependencies)); but then it would only save the first dependency in "core.extension.module".

Attached a patch with a failing test to better highlight the problem. I'll post the final, fixing patch later.

ademarco’s picture

Attached a patch that fixed the problem highlighted in #180.

ademarco’s picture

Status: Needs work » Needs review
ademarco’s picture

Status: Needs work » Needs review
FileSize
8.18 KB
2.87 KB
35.25 KB

Fix broken tests.

markhalliwell’s picture

vdacosta@voidtek.com’s picture

Test and improve the readability of the code.

markhalliwell’s picture

Status: Needs review » Needs work

#176.5 and #176.6 still need to be addressed.

Version: 8.6.x-dev » 8.7.x-dev

Drupal 8.6.0-alpha1 will be released the week of July 16, 2018, which means new developments and disruptive changes should now be targeted against the 8.7.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

pfrenssen’s picture

Patch didn't apply any more on 8.6.0. Straight reroll.

We should have a look at the test failures.

jasonawant’s picture

I'm experiencing an error when installing from configuration. I'm listing the dependencies as follows.

dependencies:
  - module_a
  - module_b
  - module_c

When installing from a single exported configuration directory, see change record Installing Drupal from configuration (only certain profiles), I see the following errors.

Unable to install the [THEME NAME] theme since it requires the theme.

Digging into this, I see Drupal\Core\EventSubscriber\ConfigImportSubscriber::validateThemes() checks to see if a theme's theme dependencies have been met.

    // Ensure that all themes being installed have their dependencies met.
    foreach ($installs as $theme) {
      foreach (array_keys($theme_data[$theme]->requires) as $required_theme) {
        if (!isset($core_extension['theme'][$required_theme])) {
          $theme_name = $theme_data[$theme]->info['name'];
          $required_theme_name = $theme_data[$required_theme]->info['name'];
          $config_importer->logError($this->t('Unable to install the %theme theme since it requires the %required_theme theme.', ['%theme' => $theme_name, '%required_theme' => $required_theme_name]));
        }
      }
    }

I think this the issue occurs when it checks requires at array_keys($theme_data[$theme]->requires). This returns a list of dependencies, including modules prepared by this patch in Drupal\Core\Extension\ThemeHandler::rebuildThemeData(), specially the following line.

$themes[$key]->module_dependencies = isset($theme->base_themes) ? array_diff_key($theme->requires, $theme->base_themes) : $theme->requires;

I don't know if buildModuleDependencies() needs to be updated or we just shuffle modules dependences into module_dependences and then restore requires to only themes, not modules too. I've worked around the error above by adding the following. Not sure if that's the best path forward. I have to resolve other config install issues before I can review whether or not theme and its dependences were installed correctly.

$themes[$key]->requires = array_diff_key($themes[$key]->requires, $themes[$key]->module_dependencies);
markhalliwell’s picture

I'm wondering if maybe we should postpone this issue on #3023131: [PP-1] Extension System, Part IV: ExtensionHandler and ExtensionHandlerInterface?

It will be a lot easier to implement this once there's a clearer path between ExtensionLists and ExtensionHandlers and there will be a lot of shared code that this issue could benefit from.

Version: 8.7.x-dev » 8.8.x-dev

Drupal 8.7.0-alpha1 will be released the week of March 11, 2019, which means new developments and disruptive changes should now be targeted against the 8.8.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

TLWatson’s picture

+1. I am experiencing the same issues as in #190.

kamkejj’s picture

8.7 patch re-work.

Re-work 8.6 patch from #178 to work with 8.7 since the patch from #189 didn't apply because a straight re-roll can not be done with the 8.7 changes to /core/lib/Drupal/Core/Extension/ThemeHandler.php and the addition of /core/lib/Drupal/Core/Extension/ThemeExtensionList.php

SKAUGHT’s picture

Status: Needs work » Needs review
lauriii’s picture

Priority: Major » Critical

I'm going to bump this issue to critical because we are planning to deprecate some libraries and move them to contrib modules. Without this feature, contrib themes might have include duplicate versions of libraries since they cannot depend on the modules providing these libraries. Not having this feature is a significant regression to developer experience.

Wim Leers’s picture

Status: Needs review » Needs work
  • #194 contains 50 coding standards violations.
  • Those coding standards violations mask 58 test failures, which also need to be fixed.
  • Finally, @markcarver's feedback from more than a year ago in #187 still applies: #176.5 and #176.6 still need to be addressed.
effulgentsia’s picture

Issue tags: +Drupal 9

Tagging this with "Drupal 9" (meaning, Drupal 9 could highly benefit from this getting done in Drupal 8) due to #196.

Version: 8.8.x-dev » 8.9.x-dev

Drupal 8.8.0-alpha1 will be released the week of October 14th, 2019, which means new developments and disruptive changes should now be targeted against the 8.9.x-dev branch. (Any changes to 8.9.x will also be committed to 9.0.x in preparation for Drupal 9’s release, but some changes like significant feature additions will be deferred to 9.1.x.). For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

edmilsonefs’s picture

Hello! I will have some free time... I might be able to help here(can be my first contribution). I may need some guidance to start.

Using the patch at #194 as a base and work to fix the coding standards violations is a good plan to start?

If so, I should use version 8.7.x, apply the patch #194 and then start trying to fix them all?

DamienMcKenna’s picture

@edmilsonefs: Thanks for stepping up to help! If you have time to deal with this, you should:

  • Update the "assigned" field to your name.
  • Reroll the patch so it can apply cleanly against 8.9.x.
  • Work through the feedback in #197, which also refers to previous comments.
  • When you're done, set the "assigned" field back to "unassigned".

You should also upload a patch after you accomplish each item, especially if you're not going to be able to follow up on it for a few days, that way someone else could take the baton and run on a bit further with it (think: relay race).

edmilsonefs’s picture

Assigned: Unassigned » edmilsonefs
shaal’s picture

I took a stab at rerolling #194 for Drupal 8.8.x

There were parts that seemed to already been done in core itself, I tried figuring out what's the correct parts to keep/discard/update.

Since there is no way to interdiff, I created a diff between the 2 patches.

bnjmnm’s picture

Apologies to @edmilsonefs if you were actively working on this I reached out on Slack to see if this was the case and didn't hear back, and I made the call to get this critical issue moving again. You mentioned this was your first issue please, feel free to contact me on Slack if you'd like some help finding another good first-issue and I'd be happy to help you through the contrib process which can be confusing at first.

This rerolls against 8.9, and brings back several necessary test themes that were removed in #186 (I'm guessing unintentionally). Also fixed coding standards on anything added in the patch.

#176.5 and #176.6 still need to be addressed.

bnjmnm’s picture

Addressing the test failures and deprecated code.
Still working on #176.5-6

bnjmnm’s picture

Status: Needs work » Needs review
FileSize
34.15 KB
66.16 KB

This ,

  1. Addresses 5-6 from #176
  2. As part of #176.5, wound up addressing a separate issue: #2937952: Show theme dependencies on the module page
  3. Brings back ThemeUiTest which I assume was unintentionally removed from an earlier iteration of this patch
  4. Added access checks so themes that would enable modules can only be installed by users with the administer modules permission.
  5. Expanded ThemeUiTest to cover the functionality added here

Two things in particular to note in this +30k of changes:
A) This patch includes saving theme info to State in order to prevent recursion problems with 'extension.list.module' calling 'extension.list.theme'. Happy to hear more effective ways to accomplish this.

B) I made a change to Drupal/Core/Extension/ModuleHandler::buildModuleDependencies to prevent using Drush to uninstall modules that an enabled theme depends on. This works, but the output of Drush isn't particularly informative. I looked into this and it seemed like it would require modifications to Drush, but I'd love to find out there's a way to do it in core.

#190 probably needs further investigation, and hopefully it's something that can be reproduced in tests.

Wim Leers’s picture

#206.2: so does that mean this issue is blocked on #2937952: Show theme dependencies on the module page? If so, we should bump that to critical priority since this issue is critical.

RE: A + B that you call out: wow! 🤯 Thanks for calling those out specifically, that's really helpful. 🙏

RE: A (breaking recursion by using State). Very interesting. I'm not principally opposed to it, but we need to be careful. It'd be helpful if you can point to other places in Drupal core that do this too. If you can, the discussions that led to those getting committed would be useful guides in assessing whether it's appropriate here too. Are there such places?

Superficial patch review

  1. +++ b/core/lib/Drupal/Core/Extension/ModuleHandler.php
    @@ -236,6 +247,13 @@ public function buildModuleDependencies(array $modules) {
    +      // This prevents uninstalling a module required by a theme via drush,
    +      // but the output of the command is not great at the moment.
    

    This reads like a temporary comment that is not intended for commit? 😅

  2. +++ b/core/modules/system/src/Form/ModulesListForm.php
    @@ -74,6 +75,13 @@ class ModulesListForm extends FormBase {
    +   * A array keyed by modules that required by themes.
    

    🤓 Missing are.

  3. +++ b/core/modules/system/src/Form/ModulesListForm.php
    @@ -166,6 +174,19 @@ public function buildForm(array $form, FormStateInterface $form_state) {
    +    $theme_list = \Drupal::service('extension.list.theme');
    

    🤔 Either we should inject this, or we should document why this can't be injected.

  4. +++ b/core/modules/system/src/Form/ModulesUninstallForm.php
    @@ -68,12 +77,19 @@ public static function create(ContainerInterface $container) {
    +      @trigger_error('The extension.list.theme service must be passed to \Drupal\system\Form\ModulesUninstallForm::__construct(). It was added in Drupal 8.9.0 and will be required before Drupal 9.0.0.', E_USER_DEPRECATED);
    

    🤓 This should use __NAMESPACE__.

  5. +++ b/core/modules/system/src/Form/ModulesUninstallForm.php
    @@ -159,6 +188,15 @@ public function buildForm(array $form, FormStateInterface $form_state) {
    +      // Modules required by an active theme should not be allowed to be
    +      // uninstalled.
    

    🤓 s/should not be/are not/

    (And I think that that change will make it fit on a single line too!)

  6. +++ b/core/modules/system/src/Form/ThemeInstallConfirmForm.php
    @@ -0,0 +1,168 @@
    + * @internal
    

    👍 If \Drupal\system\Form\ModulesUninstallConfirmForm is allowed to be @internal in HEAD, then I think this one can be too.

  7. +++ b/core/modules/system/src/Form/ThemeInstallConfirmForm.php
    @@ -0,0 +1,168 @@
    +  /**
    +   * The machine name of the theme being enabled.
    +   *
    +   * @var string
    +   */
    +  protected $theme;
    +
    +  /**
    +   * The name of the theme being enabled.
    +   *
    +   * @var string
    +   */
    +  protected $themeName;
    

    👍 I was gonna say "why explicitly say machine name" — but then it became clear :)

  8. +++ b/core/modules/system/src/Form/ThemeInstallConfirmForm.php
    @@ -0,0 +1,168 @@
    +    $query = $this->getRequest()->query;
    +    $this->modulesToBeInstalled = $query->get('modules');
    

    🤔 Woah. I've never seen the request object being retrieved in a constructor. Do we do the same thing somewhere else?

  9. +++ b/core/modules/system/src/Form/ThemeInstallConfirmForm.php
    @@ -0,0 +1,168 @@
    +  public function submitForm(array &$form, FormStateInterface $form_state) {
    +  }
    

    😅 Ehm … how does this work exactly? 😀

  10. +++ b/core/modules/system/src/Form/ThemeInstallConfirmForm.php
    @@ -0,0 +1,168 @@
    +    // The route is different if the theme is also to be set to default.
    

    🤔🤓 I think "set as the default" is better?

  11. +++ b/core/modules/system/src/Form/ThemeInstallConfirmForm.php
    @@ -0,0 +1,168 @@
    +    // Change to a link with the necessary CSRF token.
    

    🤔🤓 Why mention the CSRF token in the comment if none of the logic deals with that? Yes, this URL must contain a CSRF token, but the URL generator takes care of that for us.

bnjmnm’s picture

All the numbered items #207 are addressed. Quick pass reviews are very welcome and appreciated! This is a gigantic patch and I think embracing the concept of manageable chunks will help get this to the finish line faster.

Re #207

so does that mean this issue is blocked on #2937952: Show theme dependencies on the module page? If so, we should bump that to critical priority since this issue is critical.

. In this case, that issue can probably be closed as a duplicate. The requirements of that issue were covered by the the work I did to address #174.5 (preventing uninstallation of modules required by active themes). Once this gets a little more reviewing to confirm the approach is sound I'll close that as a duplicate.

Also #207

I'm not principally opposed to it, but we need to be careful. It'd be helpful if you can point to other places in Drupal core that do this too. If you can, the discussions that led to those getting committed would be useful guides in assessing whether it's appropriate here too. Are there such places?

The closest I could find was

$this->state->set('views.view_route_names', $this->viewRouteNames);

in \Drupal\views\EventSubscriber\RouteSubscriber::routes and \Drupal\views\EventSubscriber\RouteSubscriber::routeRebuildFinished. It's not a 1:1 relationship, but it seems to be storing information in State that could technically be generated dynamically.

I'm definitely still curious if there's another solution. The issue I'm currently running into is building the list in extension.list.theme includes getting module info from extension.list.module this works fine, but once I try to add something where extension.list.module gets info a list extension.list.theme the recursion starts putting up a fuss -- the focal point happening at drupal_get_filename(). If there were a way to get dependency info from theme yml files without having to call drupal_get_filename() it may be possible to avoid using State.

I also created an issue in the Drush project regarding the changes that this functionality would present https://github.com/drush-ops/drush/issues/4248 Once this issue gets committed I'll look into taking care of those Drush needs if they're not already being attended to.

Wim Leers’s picture

Another round of review. Still fairly superficial. I would need at least half a day to review this as thoroughly as needed for an RTBC.

  1. +++ b/core/modules/system/src/Controller/ThemeController.php
    @@ -40,6 +42,20 @@ class ThemeController extends ControllerBase {
    +  protected $kernel;
    
    @@ -51,12 +67,22 @@ class ThemeController extends ControllerBase {
    +   * @param \Drupal\Core\DrupalKernelInterface $kernel
    +   *   The Drupal kernel.
    ...
    +    $this->kernel = $kernel;
    

    🤔 AFAICT this is not being used?

  2. +++ b/core/modules/system/src/Controller/ThemeController.php
    @@ -249,4 +297,34 @@ public function setDefaultTheme(Request $request) {
    +  /**
    +   * Updates an object's external dependencies from the container.
    +   *
    +   * This method depends on \Drupal\Core\DependencyInjection\Container::get()
    +   * adding the _serviceId property to all services.
    +   *
    +   * @param \Symfony\Component\DependencyInjection\ContainerInterface $container
    +   *   The container.
    +   *
    +   * @see \Drupal\Core\DependencyInjection\Container
    +   *
    +   * @todo https://www.drupal.org/node/2380293 Remove this method and instead
    +   *   add the trait that supplies this method from that issue.
    +   */
    +  protected function updateDependencies(ContainerInterface $container) {
    +    $vars = get_object_vars($this);
    +    foreach ($vars as $key => $value) {
    +      if (is_object($value) && isset($value->_serviceId)) {
    +        $this->$key = $container->get($value->_serviceId);
    +        continue;
    +      }
    +
    +      // Special case the container, which might not have a service ID.
    +      if ($value instanceof ContainerInterface) {
    +        $this->$key = $container;
    +        continue;
    +      }
    +    }
    +  }
    

    🤯😨

  3. +++ b/core/modules/system/src/Form/ModulesListForm.php
    @@ -210,11 +211,53 @@ public function buildForm(array $form, FormStateInterface $form_state) {
    +      return t('@module (<span class="admin-missing">missing</span>)', ['@module' => Unicode::ucfirst($dependency)]);
    ...
    +        return t('@module (<span class="admin-missing">incompatible with</span> version @version)', [
    ...
    +        return t('@module (<span class="admin-missing">incompatible with</span> this version of Drupal core)', [
    

    👎 These should use $this->t()

    🤔 Is Unicode::ucfirst() truly appropriate here?

  4. +++ b/core/modules/system/src/Form/ModulesListForm.php
    @@ -210,11 +211,53 @@ public function buildForm(array $form, FormStateInterface $form_state) {
    +      if ($dependency === 'common_test') {
    +        $stop = 'here';
    +
    +      }
    

    😅 This looks like a debug leftover?

  5. +++ b/core/modules/system/src/Form/ModulesUninstallForm.php
    @@ -68,12 +77,19 @@ public static function create(ContainerInterface $container) {
    +      @trigger_error('The extension.list.theme service must be passed to ' . __NAMESPACE__ . 'ModulesUninstallForm::__construct(). It was added in Drupal 8.9.0 and will be required before Drupal 9.0.0.', E_USER_DEPRECATED);
    

    🤓 Missing a \ before ModulesUninstallForm

  6. +++ b/core/modules/system/src/Form/ThemeInstallConfirmForm.php
    @@ -0,0 +1,165 @@
    +    // This method is present as it is required by ConfirmFormBase but is
    +    // unused because the submission button is changed to a link in buildForm().
    

    👍

  7. +++ b/core/modules/system/src/Form/ThemeInstallConfirmForm.php
    @@ -0,0 +1,165 @@
    +    // to the theme install links in admin/appearance that don't require a
    

    🤓 to can be moved to the preceding line.

    🤓 admin/appearance/admin/appearance

  8. +++ b/core/modules/system/tests/src/Functional/Theme/ThemeUiTest.php
    @@ -0,0 +1,166 @@
    +  /**
    +   * Modules to enable.
    +   *
    +   * @var array
    +   */
    

    🤓 Übernit: {inheritdoc}.

  9. +++ b/core/modules/system/tests/src/Functional/Theme/ThemeUiTest.php
    @@ -0,0 +1,166 @@
    +    $user = $this->drupalCreateUser([
    

    🤔 $user is not used anywhere. Why not do $this->drupalLogin($this->drupalCreateUser(…?

  10. +++ b/core/modules/system/tests/src/Functional/Theme/ThemeUiTest.php
    @@ -0,0 +1,166 @@
    +  /**
    +   * Tests installing a theme with existing module dependencies.
    +   */
    +  public function testInstallModuleWithNotInstalledDependencies() {
    ...
    +  /**
    +   * Tests installing a theme with existing module dependencies.
    +   */
    +  public function testInstallModuleWithAlreadyInstalledDependencies() {
    

    🤓 Comments don't match what's being tested.

  11. +++ b/core/modules/system/tests/themes/test_theme_depending_nonexisting_module/test_theme_depending_on_nonexisting_module.info.yml
    @@ -0,0 +1,6 @@
    +base theme: stable
    
    +++ b/core/modules/system/tests/themes/test_theme_depending_on_already_installed_module/test_theme_depending_on_already_installed_module.info.yml
    @@ -0,0 +1,6 @@
    +base theme: stable
    
    +++ b/core/modules/system/tests/themes/test_theme_depending_on_modules/test_theme_depending_on_modules.info.yml
    @@ -0,0 +1,7 @@
    +base theme: stable
    

    🤔 Why stable and not stark?

    It seems there's no value in this case to use stable as the base theme? The fewer things in core that depend on it, the better, I think.

  12. +++ b/core/tests/Drupal/KernelTests/Core/Asset/LibraryDiscoveryIntegrationTest.php
    @@ -13,6 +13,12 @@
    +  public static $modules = ['system'];
    
    +++ b/core/tests/Drupal/KernelTests/Core/Render/ElementInfoIntegrationTest.php
    @@ -11,6 +11,11 @@
    +  public static $modules = ['system'];
    

    🤔 These seem bugfixes that could land independently?

  13. +++ b/core/tests/Drupal/KernelTests/Core/Asset/LibraryDiscoveryIntegrationTest.php
    @@ -13,6 +13,12 @@
    +
    +
       /**
    

    🤓Extraneous whitespace.

  14. +++ b/core/tests/Drupal/Tests/Core/Extension/ThemeHandlerTest.php
    @@ -58,7 +59,8 @@ protected function setUp() {
    +    $this->state = $this->createMock('Drupal\Core\State\StateInterface');
    

    🤔 Let's use StateInterface::class.

  15. +++ b/core/tests/Drupal/Tests/Core/Extension/ThemeHandlerTest.php
    @@ -74,15 +76,14 @@ protected function setUp() {
           ]));
    -
         $theme_data = $this->themeHandler->rebuildThemeData();
    

    🤓 Übernit: unnecessary whitespace change.

phenaproxima’s picture

Didn't get all the way through, but...

  1. +++ b/core/lib/Drupal/Core/Extension/ModuleHandler.php
    @@ -221,6 +221,17 @@ protected function add($type, $name, $path) {
    +    $themes = \Drupal::state()->get('theme.list', []);
    

    Should the state service be injected?

  2. +++ b/core/lib/Drupal/Core/Extension/ModuleInstaller.php
    @@ -372,7 +373,7 @@ public function uninstall(array $module_list, $uninstall_dependents = TRUE) {
    +          if (!isset($module_data[$dependent]) && !isset($theme_list[$dependent])) {
    

    Nitpick (or pro-tip, depending on how your look at it): isset() is variadic and it only returns TRUE if all of the parameters are set. So this could be !isset($module_data[$dependent], $theme_list[$dependent]).

    Quoth PHP's documentation:

    If multiple parameters are supplied then isset() will return TRUE only if all of the parameters are considered set. Evaluation goes from left to right and stops as soon as an unset variable is encountered.

  3. +++ b/core/lib/Drupal/Core/Extension/ThemeExtensionList.php
    @@ -140,6 +141,16 @@ protected function doList() {
    +      // buildModuleDependencies() adds a theme->requires array that contains
    +      // both module and base theme dependencies, if they are specified. Ensure
    +      // that every theme stores the list of module dependencies separately
    +      // from the full requires list.
    

    This comment doesn't seem to be completely accurate...? I'm not seeing any reference to $theme->requires in buildModuleDependencies(), although I might be looking in the wrong place.

  4. +++ b/core/lib/Drupal/Core/Extension/ThemeHandler.php
    @@ -48,11 +56,14 @@ class ThemeHandler implements ThemeHandlerInterface {
    +  public function __construct($root, ConfigFactoryInterface $config_factory, ThemeExtensionList $theme_list, StateInterface $state) {
         $this->root = $root;
         $this->configFactory = $config_factory;
         $this->themeList = $theme_list;
    +    $this->state = $state;
    

    Does this require (eugh) a BC layer?

  5. +++ b/core/lib/Drupal/Core/Extension/ThemeHandler.php
    @@ -161,7 +172,11 @@ public function reset() {
    +    // Store a list of themes in state so other classes in Drupal\Core\Extension
    +    // can access this information without recursion problems.
    +    $this->state->set('theme.list', $theme_list);
    

    🤔 This decision feels fragile to me, to be very honest. It requires other classes to have internal knowledge of how the theme list is stored, rather than doing something in ThemeHandler itself, or maybe ThemeExtensionList, to prevent recursion problems. Is there no way we could "hide" this?

  6. +++ b/core/lib/Drupal/Core/Extension/ThemeInstaller.php
    @@ -97,15 +107,22 @@ public function __construct(ThemeHandlerInterface $theme_handler, ConfigFactoryI
    +      @trigger_error('Not supplying the $module_installer parameter is deprecated since version 8.6.0 and will be a required parameter in Drupal 9.0.0. Supply the $module_installer parameter. See https://www.drupal.org/node/2971389.', E_USER_DEPRECATED);
    

    This deprecation notice has the wrong version of Drupal core. :)

alexpott’s picture

+++ b/core/lib/Drupal/Core/Extension/ThemeHandler.php
@@ -161,7 +172,11 @@ public function reset() {
+    // Store a list of themes in state so other classes in Drupal\Core\Extension
+    // can access this information without recursion problems.
+    $this->state->set('theme.list', $theme_list);

It'd be great to avoid this. Storing lists of things in state that can be determined from the file system (and be changed by people editing things) has not worked out well for us in the past. And specifically extension object stored in state caused issues in the 8.x.x cycle. Put another way we've actually only just got rid of this. Adding this back feels like a step backwards and not forwards. I'm trying to think of alternatives - I've got nothing yet but I wanted to post this because it's presence is setting off warning bells for me.

alexpott’s picture

  1. So I think you don't need the state thing at all. We don't need to affect the module dependency graph with this change as far as I can see. We can implement a module_install.uninstall_validator tagged service. See
      required_module_uninstall_validator:
        class: Drupal\Core\Extension\RequiredModuleUninstallValidator
        tags:
          - { name: module_install.uninstall_validator }
        arguments: ['@string_translation', '@extension.list.module']
        lazy: true
    

    as an example. We can then prevent modules from being uninstalled that are required by installed themes and then, I think, we don't need to make any changes to the module installer or module handler.

  2. +++ b/core/modules/system/src/Controller/ThemeController.php
    @@ -249,4 +297,34 @@ public function setDefaultTheme(Request $request) {
    +      // Special case the container, which might not have a service ID.
    +      if ($value instanceof ContainerInterface) {
    +        $this->$key = $container;
    +        continue;
    +      }
    

    I hope this is not needed. The container shouldn't be stored on this object.

  3. +++ b/core/modules/system/src/Controller/ThemeController.php
    index f32dad0909..34c36c7aac 100644
    --- a/core/modules/system/src/Form/ModulesListForm.php
    
    --- a/core/modules/system/src/Form/ModulesListForm.php
    +++ b/core/modules/system/src/Form/ModulesListForm.php
    

    I hope the changes tot his file are not necessary. Ah I see you're trying to re-use functionality from this is the theme form. I think this points out that there is far too much business logic which determines whether a module can or cannot be install on the ModulesListForm - in my mind we should refactor that first. For example, if the module declared it was not compatible with a specific version of PHP and this module was a dependency of a theme - installing via a theme would allow you to install on an unsupported PHP version.

  4. +++ b/core/modules/system/src/Form/ThemeInstallConfirmForm.php
    @@ -0,0 +1,165 @@
    +    return $this->formatPlural(count($modules_to_be_installed),
    +      'Enabling the %theme_name theme will also enable the module: %module_string.',
    +      'Enabling the %theme_name theme will also enable these modules: %module_string.',
    +      [
    +        '%theme_name' => $theme_name,
    +        '%module_string' => $module_string,
    +      ]
    +    );
    

    If any of the modules are experimental we're going to need the experimental disclaimer.

  5. +++ b/core/modules/system/src/Form/ThemeInstallConfirmForm.php
    @@ -0,0 +1,165 @@
    +    // Change "Confirm" from a button to a link, so it functions identically
    +    // to the theme install links in admin/appearance that don't require a
    +    // confirmation form.
    

    I think this should be a form submission and we need to do the same validation and checks as ModulesListConfirmForm::submitForm and \Drupal\system\Form\ModulesListForm::submitForm

  6. +++ b/core/tests/Drupal/KernelTests/Core/Asset/LibraryDiscoveryIntegrationTest.php
    @@ -13,6 +13,12 @@
    +  /**
    +   * {@inheritdoc}
    +   */
    +  public static $modules = ['system'];
    

    This looks like a sign of something not being quite right with the approach. Ideally system module should be becoming less required not more.

bnjmnm’s picture

Re: #212.5

I think this should be a form submission and we need to do the same validation and checks as ModulesListConfirmForm::submitForm and \Drupal\system\Form\ModulesListForm::submitForm

This is a very good point. It should not be possible to bypass these validations by enabling a module via the Theme UI, and this needs to be addressed. I'm not yet sure how to best take care of this.

Two approaches come to mind:

  1. Limit just how much module-enabling can happen via the theme UI. If a theme's dependencies doesn't pass one of the checks in ModulesListConfirmForm::submitForm or \Drupal\system\Form\ModulesListForm::submitForm (or perhaps even stricter critera), it will not be possible to install the theme without first going to admin/modules to enable the dependencies. The Install/Install and set as default links would not be available in this case, and replaced by a message explaining what needs to be enabled in admin/modules. This option doesn't have ideal UX, but enabling a theme is not something a user needs to do frequently, and streamlining the experience could be done in a followup.
  2. Duplicating or abstracting the module installation validation logic so it can be used when enabling themes, and figuring out how to consolidate confirmation forms as simply re-using the logic \Drupal\system\Form\ModulesListForm::submitForm would result in confirmation-of-confirmation forms and having to persist data across them. I'm not fully sure at the moment how this could be accomplished, but if there's consensus that it's a must-have for this issue then it'll get figured out.

The other approach that comes to mind seems fairly complex and potentially fragile as it would mean either

It's quite possible there's a middleground between the two extremes mentioned above, hence my sharing it here while I work on addressing other feedback on the patch (I'm VERY pleased to be made aware of module_install.uninstall_validator, btw). In the absence of feedback move forward with approach #1.

bnjmnm’s picture

There's also the use case of Experimental Themes redirecting to their own confirmation form, so Experimental themes with module dependencies would require confirmation of:

  • It being an experimental theme
  • The modules being enabled
  • If the module dependencies are experimental there's needs to be a confirmation for that as well.
  • The criteria checked in ModulesListConfirmForm::submitForm and \Drupal\system\Form\ModulesListForm::submitForm, some of which redirects to other confirmation forms

After identifying Experimental themes as yet another piece of confirmation/validation logic to consider, I'm strongly leaning towards not permitting modules to be enabled via the Theme UI at all. The user would have to visit admin/modules and enable the dependencies before the theme is installable. Installing modules in the Theme UI could be done in followups while still allowing the critical needs of this issue to be met here.

bnjmnm’s picture

This should address everything in #209, #210, #211, and #212 - some less direct that others.

The biggest change is that I removed the ability to enable modules via the Theme UI. The ability to enable modules in the Theme UI adds a complex layer of validation and form confirmations that can be done in a followup issue. The immediate need is for themes to depend on modules, and this is still satisfied. Enabling themes is an infrequent enough activity that having to switch to admin/modules on occasion shouldn't have much negative impact on the overall experience. This also removes 20k from the patch.

A few other things to note:

  • No longer using State to contain theme information (thank goodness)
  • Could not implement #210.2 as it needs both issets to return false/ If they're merged into one isset() it will return false if either condition is untrue.
  • Had to add a core verion to the path_alias module's info.yml in order for tests to pass.
  • An earlier version of the patch added checkDependencyMessage as a static method. It wasn't clear why and based on feedback it looks like other folks are also unsure. I moved this to a trait as that seems a more appropriate way to share common functionality across multiple classes.
alexpott’s picture

  1. +++ b/core/core.services.yml
    @@ -550,7 +550,7 @@ services:
         class: Drupal\Core\Extension\RequiredModuleUninstallValidator
         tags:
           - { name: module_install.uninstall_validator }
    -    arguments: ['@string_translation', '@extension.list.module']
    +    arguments: ['@string_translation', '@extension.list.module', '@extension.list.theme', '@config.factory']
         lazy: true
    

    We can add another module_install.uninstall_validator rather than extending this one. No deprecations and we can get the list of dependenty modules from installed modules upfront. And then check each module... less processing that way.

  2. +++ b/core/lib/Drupal/Core/Extension/RequiredModuleUninstallValidator.php
    @@ -60,4 +95,20 @@ protected function getModuleInfoByModule($module) {
    +    $installed_themes = $this->configFactory->get('core.extension')->get('theme');
    +    return array_filter(array_keys($installed_themes), function ($theme_name) use ($module) {
    +      return isset($this->themeExtensionList->get($theme_name)->module_dependencies[$module]);
    +    });
    

    The theme extension list knows about what's installed - see \Drupal\Core\Extension\ExtensionList::getAllInstalledInfo().

bnjmnm’s picture

Implemented the recommendations in #216

bnjmnm’s picture

Cancelled the test runs in #217 -- it occurred to me the changes I made there also require updating tests.

bnjmnm’s picture

Updated with the additional steps that didn't make it into #217

rensingh99’s picture

Hi,

I have checked the patch #219.

There was some issue with the name of the module. I have corrected that and uploaded the patch.

After that, I have tested the patch and it worked as designed.

Below are my updates.

1 I have added the module in dependencies in theme.info.Yml

- node
  - if_then_else
  - if_then_else_examples

2 When I visited the appearance page then it showing the required module information by theme. And I was not able to install the module.

3 I was able to install the theme when I installed the required module by theme.

The patch is working great.

Thanks,
Ren

tedbow’s picture

Status: Needs review » Needs work

Great this issue is getting some traction. First pass at reviewing mainly test coverage.

  1. +++ b/core/lib/Drupal/Core/Extension/ModuleInstaller.php
    @@ -355,6 +355,7 @@ public function uninstall(array $module_list, $uninstall_dependents = TRUE) {
    +    $theme_list = \Drupal::service('extension.list.theme')->getList();
    

    Technically we don't the theme list unless $uninstall_dependents is TRUE. Since getting the theme list requires scanning directories we should probably move this within the
    if ($uninstall_dependents) {
    block further down the method.

  2. +++ b/core/lib/Drupal/Core/Extension/ModuleRequiredByThemesUninstallValidator.php
    @@ -0,0 +1,94 @@
    +    $installed_themes = $this->themeExtensionList->getAllInstalledInfo();
    +    foreach ($this->themeExtensionList->getAllInstalledInfo() as $theme) {
    

    $this->themeExtensionList->getAllInstalledInfo() is called 2x in these 2 lines. We can just use $installed_themes for the foreach loop

  3. +++ b/core/modules/system/tests/src/Functional/Module/InstallUninstallTest.php
    @@ -133,6 +133,7 @@ public function testInstallUninstall() {
    +
    

    This file only has 1 blank line change. Maybe a left over?

  4. +++ b/core/modules/system/tests/src/Functional/Theme/ThemeUiTest.php
    @@ -0,0 +1,183 @@
    +  /**
    +   * Tests installing a theme with depending on modules that are not enabled.
    +   */
    +  public function testInstallModuleWithNotInstalledDependencies() {
    ...
    +  /**
    +   * Tests installing a theme depending on an already enabled module.
    +   */
    +  public function testInstallModuleWithAlreadyInstalledDependencies() {
    

    These 2 test methods each have their own test theme. One that requires node and another that requires 2 new test modules.

    Is there are need to actually have 2 themes here? Couldn't we just use the same theme and just install the required modules at the beginning of testInstallModuleWithAlreadyInstalledDependencies()

    Of just have 1 test method and first try the theme without the dependencies enable and then try after they are enabled.

  5. +++ b/core/modules/system/tests/src/Functional/Theme/ThemeUiTest.php
    @@ -0,0 +1,183 @@
    +    $themeXpath = '//h3[contains(text(), "test theme depending on modules")]';
    

    I don't think we have functional test coverage for a theme that is not dependent itself on modules it's base theme requires modules.

    We could probably have the same test methods just with dataProvider that tests the theme directly dependent and the theme that is dependent via it's base theme.

  6. +++ b/core/modules/system/tests/src/Functional/Theme/ThemeUiTest.php
    @@ -0,0 +1,183 @@
    +    $expected_incompatible_text = 'This theme requires the listed modules to operate correctly. They must first be enabled via the Extend page.';
    

    So one difference between modules depending on modules and themes depending on module is that for a module you can just check the box and when you submit the form it will prompt to enable the other modules.

    For theme you have to go to the modules and then find the modules that were just listed as a required for the theme.

    This seems fine as a first implementation of this feature but I think we should create a follow up see if can get similar experience where you don't have to go to a seperate from but are just shown a confirmation form to enable the required modules.

  7. +++ b/core/modules/system/tests/src/Functional/Theme/ThemeUiTest.php
    @@ -0,0 +1,183 @@
    +    $assert_session->elementTextContains('css', '[data-drupal-selector="edit-modules-node"] .requirements', 'test theme depending on already installed module (Theme) (disabled)');
    +    $assert_session->elementTextContains('css', '[data-drupal-selector="edit-modules-test-another-module-required-by-theme"] .requirements', 'test theme depending on modules (Theme) (disabled)');
    +    $assert_session->elementTextContains('css', '[data-drupal-selector="edit-modules-test-module-required-by-theme"] .requirements', 'test theme depending on modules (Theme) (disabled)');
    

    is the functionality that was made for this follow-up #2937952: Show theme dependencies on the module page?

    If so should be close the follow up. If not I think we should update the description on that issue to be more clear.

  8. +++ b/core/modules/system/tests/src/Functional/Theme/ThemeUiTest.php
    @@ -0,0 +1,183 @@
    +    $this->drupalGet('admin/modules/uninstall');
    +    $assert_session->elementExists('css', '[name="uninstall[test_module_required_by_theme]"][disabled]');
    +    $assert_session->elementExists('css', '[name="uninstall[test_another_module_required_by_theme]"][disabled]');
    +    $assert_session->elementTextContains('css', '[data-drupal-selector="edit-test-another-module-required-by-theme"] .item-list', 'Required by: test_theme_depending_on_modules (Theme)');
    +    $assert_session->elementTextContains('css', '[data-drupal-selector="edit-test-module-required-by-theme"] .item-list', 'Required by: test_theme_depending_on_modules (Theme)');
    

    We don't actually have test coverage for uninstalling a module which is required by a theme.

    I think the should have to uninstall the theme first before you have are uninstall the module but we should have explicit test coverage for this.

  9. +++ b/core/modules/system/tests/themes/test_theme_depending_on_modules/test_module_required_by_theme/test_module_required_by_theme.services.yml
    @@ -0,0 +1,3 @@
    +  test_module_required_by_theme.service:
    +    class: Drupal\test_module_required_by_theme\Service
    

    This service actually used anywhere in a test?

  10. +++ b/core/tests/Drupal/Tests/Core/Extension/ModuleRequiredByThemesUninstallValidatorTest.php
    @@ -0,0 +1,96 @@
    +    $this->moduleRequiredByThemeUninstallValidator->expects($this->once())
    +      ->method('getModulesThemesDependOn')
    +      ->willReturn([]);
    ...
    +    $this->moduleRequiredByThemeUninstallValidator->expects($this->once())
    +      ->method('getModulesThemesDependOn')
    +      ->willReturn([
    +        $module => [$theme_name],
    +      ]);
    ...
    +    $this->moduleRequiredByThemeUninstallValidator->expects($this->once())
    +      ->method('getModulesThemesDependOn')
    +      ->willReturn([
    +        $module => [$theme_name_1, $theme_name_2],
    +      ]);
    

    I think instead of mocking this protected method we should be calling the original constructor and passing in mock objects for the parameters. So we can test that this class can responds properly to calls to \Drupal\Core\Extension\ExtensionList::getAllInstalledInfo() and \Drupal\Core\Extension\ExtensionList::get()(less important to get the module name).

    It looks to me that getModulesThemesDependOn returns a different list depending on whether a themes own theme dependencies are installed or not.

tedbow’s picture

Status: Needs work » Needs review
FileSize
51.85 KB

forgot I rerolled in order to review. Here is the reroll

tedbow’s picture

Status: Needs review » Needs work
bnjmnm’s picture

Status: Needs work » Needs review
Issue tags: -Needs backport to D7
FileSize
21.52 KB
51.13 KB

This addresses everything in #221
1. ✅Moved the creation of $theme_list into the conditional.
2. ✅Removed the unnecessary call to $this->themeExtensionList->getAllInstalledInfo().
3. ✅Was leftover -- no longer changed in patch.
4. ✅That exposed how the tests were also still geared towards an approach that is no longer happening. Rewrote one big test that enables/disables themes and modules and added a dataProvider that tries a module-depending theme and a theme that subthemes a module-depending theme.
5. ✅see #4
6. ✅ created followup #3100374: Make it possible to install dependent modules when installing theme.
7. ✅ closed #2937952: Show theme dependencies on the module page as that functionality is added here (and really should be).
8. ✅ The changes to ThemeUiTest now include uninstalling a module.
9. ✅ Service unused -- so it's removed.
10. ✅ Good catch, that's a much better way to test. Rewrote the test to do that.

bnjmnm’s picture

Status: Needs work » Needs review
FileSize
1.02 KB
51.13 KB

Had to change a deprecated method for tests to pass.

tedbow’s picture

Status: Needs review » Needs work

@bnjmnm thanks for the updates. Didn't have the bandwidth for full review but here is some more. Generally looking good though!

  1. +++ b/core/lib/Drupal/Core/Extension/ModuleRequiredByThemesUninstallValidator.php
    @@ -0,0 +1,94 @@
    +  /**
    +   * Populate $this->modulesThemesDependOn with modules themes depend on.
    +   */
    

    This comment seems outdated. There is no $this->modulesThemesDependOn any more.

  2. +++ b/core/lib/Drupal/Core/Extension/ModuleRequiredByThemesUninstallValidator.php
    @@ -0,0 +1,94 @@
    +  protected function getModulesThemesDependOn() {
    +    $modules_themes_depend_on = [];
    +    $installed_themes = $this->themeExtensionList->getAllInstalledInfo();
    +    foreach ($installed_themes as $theme) {
    +      foreach ($theme['dependencies'] as $dependency) {
    +        if (!isset($installed_themes[$dependency])) {
    +          $modules_themes_depend_on[$dependency][] = $theme['name'];
    +        }
    +      }
    

    I was little confused about what this method. It is really getting just installed themes are installed not all dependencies. Which make sense for this class but could be clearer.

    also this method is getting dependencies for all modules that any theme dependencies.

    But since this is only called 1 place we really only care about 1 module.

    if we passed in $module we could just return a list of themes. and the array for other modules.

    This method doesn't have @return also

  3. +++ b/core/lib/Drupal/Core/Extension/ThemeInstaller.php
    @@ -103,9 +103,8 @@ public function __construct(ThemeHandlerInterface $theme_handler, ConfigFactoryI
    -    $extension_config = $this->configFactory->getEditable('core.extension');
    -
    
    @@ -147,11 +148,9 @@ public function install(array $theme_list, $install_dependencies = TRUE) {
    +    $extension_config = $this->configFactory->getEditable('core.extension');
    

    While it seems reasonable to move this down in the method is this actually necessary? I don't see where this would be updated in between the old and new location.

  4. +++ b/core/lib/Drupal/Core/ProxyClass/Extension/ModuleRequiredByThemesUninstallValidator.php
    @@ -0,0 +1,88 @@
    diff --git a/core/modules/path_alias/path_alias.info.yml b/core/modules/path_alias/path_alias.info.yml
    
    diff --git a/core/modules/path_alias/path_alias.info.yml b/core/modules/path_alias/path_alias.info.yml
    index 76bdd35a48..2a3ded6abc 100644
    
    index 76bdd35a48..2a3ded6abc 100644
    --- a/core/modules/path_alias/path_alias.info.yml
    
    --- a/core/modules/path_alias/path_alias.info.yml
    +++ b/core/modules/path_alias/path_alias.info.yml
    

    Not sure why this is here or why the 8.9.x version of this doesn't have this key.

    But I think this is unrelated change

  5. +++ b/core/modules/system/system.admin.inc
    @@ -131,6 +132,19 @@ function template_preprocess_system_admin_index(&$variables) {
    +          $form[$dependency]['#required_by'][] = $theme->info['name'] . ' (' . t('Theme') . ')' . (!empty($theme->experimental) ? ' (' . t('Experimental') . ')' : '') . (!$theme->status ? ' (' . t('Disabled') . ')' : '');
    

    Do we need to show "Experimental" here. We don't show this in "required by" for experimental modules.

    Also the docblock @param $variables needs to be updated to say this can be themes too.

  6. +++ b/core/modules/system/tests/src/Functional/Theme/ThemeUiTest.php
    @@ -0,0 +1,200 @@
    +  /**
    +   * {@inheritdoc}
    +   */
    +  public static $modules = ['node', 'block', 'file'];
    

    I don't any of these modules are need for the test. If you remove node and block the test will fail but that is just because the permissions are assigned in the test.

    but permission aren't needed I think. You can remove this and the permissions.

  7. +++ b/core/modules/system/tests/src/Functional/Theme/ThemeUiTest.php
    @@ -0,0 +1,200 @@
    +      'access administration pages',
    +      'view the administration theme',
    

    These 2 permissions aren't actually needed. They can removed from all instances in the test.

  8. +++ b/core/modules/system/tests/src/Functional/Theme/ThemeUiTest.php
    @@ -0,0 +1,200 @@
    +    // The confirmation page should not be reachable.
    +    $this->drupalGet('admin/appearance/install/confirm?theme=test_theme_depending_on_modules&modules%5B0%5D=test_module_required_by_theme&modules%5B1%5D=test_another_module_required_by_theme');
    +    $this->assertSession()->statusCodeEquals(404);
    

    We don't actually test if this page is reachable if you do have permissions. it seems where you would get a 404 and not a 403.

    I couldn't find this path anywhere else in core. When is this page shown.

  9. +++ b/core/modules/system/tests/src/Functional/Theme/ThemeUiTest.php
    @@ -0,0 +1,200 @@
    +    $page_text = $this->getSession()->getPage()->getText();
    +    $this->assertTrue(strpos($page_text, $confirmation_message));
    

    Can simply use $assert_session->pageTextContains($confirmation_message);

  10. +++ b/core/modules/system/tests/src/Functional/Theme/ThemeUiTest.php
    @@ -0,0 +1,200 @@
    +    $button_xpath = '//input[@type="submit"][@value="Uninstall"]';
    +    $button = $this->xpath($button_xpath);
    +    $this->assertCount(1, $button);
    +    $button[0]->click();
    

    I don't think we need to test here there is only 1 'Uninstall' button. There should be a modules page test for this.

    Should be able to just use $page->pressButton('Uninstall');

  11. +++ b/core/modules/system/tests/src/Functional/Theme/ThemeUiTest.php
    @@ -0,0 +1,200 @@
    +  public function providerTestThemeInstallWithModuleDependencies() {
    +    return [
    +      ['test theme depending on modules'],
    +      ['test theme with a base theme depending on modules'],
    

    We should be put keys in the dataprovider array so we get better errors messages.

  12. +++ b/core/modules/system/tests/src/Functional/Theme/ThemeUiTest.php
    @@ -0,0 +1,200 @@
    +  protected function verifyUninstallableTheme(array $expected_requires_list_items, $theme_name) {
    

    Core more often uses method names that start with "assert" here. So like assertUninstallableTheme

  13. +++ b/core/modules/system/tests/src/Functional/Theme/ThemeUiTest.php
    @@ -0,0 +1,200 @@
    +    $themeXpath = "//h3[contains(text(), \"$theme_name\")]";
    

    This method uses "css" to find elements 3 times and then xpath. I not sure if there is core prefered way(I prefer CSS) but I think we should switch to CSS to keep it the same in this method.

    Can use
    $elements = $this->getSession()->getPage()->findAll('css', "h3:contains(\"$theme_name\")");

    "contains" isn't standard CSS but it works in Mink and it is used in core a lot.

  14. +++ b/core/modules/system/tests/src/Functional/Theme/ThemeUiTest.php
    @@ -0,0 +1,200 @@
    +    $element = $elements[0];
    +    $container = $element->getParent();
    

    Personal preference but we could just call parent() directly on $element[0]

  15. +++ b/core/modules/system/tests/src/Functional/Theme/ThemeUiTest.php
    @@ -0,0 +1,200 @@
    +    $requires = $container->find('css', '.theme-info__requires');
    +    $requires_list_items = $requires->find('css', 'li');
    

    Also here if li was just added to the selector above you won't need $requires

  16. +++ b/core/modules/system/tests/themes/test_theme_depending_on_modules/test_another_module_required_by_theme/test_another_module_required_by_theme.info.yml
    @@ -0,0 +1,5 @@
    diff --git a/core/modules/system/tests/themes/test_theme_depending_on_modules/test_module_required_by_theme/src/Service.php b/core/modules/system/tests/themes/test_theme_depending_on_modules/test_module_required_by_theme/src/Service.php
    
    diff --git a/core/modules/system/tests/themes/test_theme_depending_on_modules/test_module_required_by_theme/src/Service.php b/core/modules/system/tests/themes/test_theme_depending_on_modules/test_module_required_by_theme/src/Service.php
    new file mode 100644
    
    new file mode 100644
    index 0000000000..3e7d7eedfa
    
    index 0000000000..3e7d7eedfa
    --- /dev/null
    
    --- /dev/null
    +++ b/core/modules/system/tests/themes/test_theme_depending_on_modules/test_module_required_by_theme/src/Service.php
    

    This file is left over. The services.yml was removed in your last patch

  17. +++ b/core/tests/Drupal/Tests/Core/Extension/ModuleRequiredByThemesUninstallValidatorTest.php
    @@ -0,0 +1,161 @@
    +if (!defined('DRUPAL_MINIMUM_PHP')) {
    +  define('DRUPAL_MINIMUM_PHP', '7.0.8');
    +}
    

    There is probably a reason for this but not sure. We don't necessarily need a comment but just thought would ask incase it is not needed.

bnjmnm’s picture

Status: Needs work » Needs review
FileSize
15.45 KB
50.5 KB

Re: #227
1-2 ✅refactored+renamed the method.
3. ✅that line doesn't need to be moved.
4. ✅ the lack of a 'core' was resulting in test failures, but I discovered this is because the message capabilities added in the patch (ModuleDependencyMessageTrait) were changed to check against ->info['core'] instead of ->info['core_incompatible']. Changed back to use 'core_incompatible' like it should be
5. ✅Right, experimental isn't shown for modules listed so themes should do the same. Updated dockblock param too
6-15. ✅ That test was a big ol mess.😐
16. ✅ Removed the file
17. Both the theme and module extension lists use DRUPAL_MINIMUM_PHP so the constant needs to be defined if they're ->reveal()d.

Also updated the Claro template to show the module dependency info in admin/appearance -- that theme didn't exist for most of the lifetime of this issue. It's possible this will need to be done in a different ticket, but reviewing this patch seems like a good way to determine that.

tedbow’s picture

Status: Needs review » Needs work

re #228

  1. looks good
  2. looks good. 1 point
    +++ b/core/lib/Drupal/Core/Extension/ModuleRequiredByThemesUninstallValidator.php
    @@ -0,0 +1,97 @@
    +/**
    + * Ensures modules required by themes cannot be uninstalled.
    + */
    +class ModuleRequiredByThemesUninstallValidator implements ModuleUninstallValidatorInterface {
    

    I think we just to update this comment to be clear it is only modules that are required by themes that are installed.

Everything else looks good.

Ok this got all the way through. I probably missed stuff. First full pass.

  1. +++ b/core/lib/Drupal/Core/Extension/ThemeInstaller.php
    @@ -106,6 +106,7 @@ public function install(array $theme_list, $install_dependencies = TRUE) {
    +    $installed_themes = $this->configFactory->get('core.extension')->get('theme') ?: [];
    
    @@ -116,16 +117,18 @@ public function install(array $theme_list, $install_dependencies = TRUE) {
    -      $installed_themes = $extension_config->get('theme') ?: [];
    

    I not sure why these lines to change. I changed them back and \Drupal\Tests\system\Functional\Theme\ThemeUiTest() all passed.

    \Drupal\KernelTests\Core\Theme\ThemeInstallerTest which isn't change here but is the kernel test for this class also didn't have any files with these lines reversed.

    If there is a reason these need to be change we should have test coverage for what fails if they are not.

    Also \Drupal\KernelTests\Core\Theme\ThemeInstallerTest should have test coverage for required modules too.

  2. +++ b/core/modules/system/src/Controller/SystemController.php
    @@ -66,13 +77,21 @@ class SystemController extends ControllerBase {
    +    $this->moduleExtensionList = $module_extension_list;
    +    if ($module_extension_list === NULL) {
    +      @trigger_error('The extension.list.module service must be passed to \Drupal\system\Controller\SystemController::__construct. It was added in Drupal 8.9.0 and will be required before Drupal 9.0.0.', E_USER_DEPRECATED);
    +      $module_extension_list = \Drupal::service('extension.list.module');
    +    }
    +    $this->moduleExtensionList = $module_extension_list;
    

    The first
    $this->moduleExtensionList = $module_extension_list;

    should be removed.

  3. +++ b/core/modules/system/src/Controller/SystemController.php
    @@ -190,6 +210,7 @@ public function themesPage() {
    +    $modules = [];
    
    @@ -231,9 +252,47 @@ public function themesPage() {
    +        if (empty($modules)) {
    +          $modules = $this->moduleExtensionList->getList();
    +        }
    

    $modules is assigned to an empty array and then the next time it is referenced we check if it is empty.

    Also I don't think $modules is access outside of the

    if ($theme->module_dependencies) {
    

    block. So we can probably just define it at the top of this block like

    $modules = $this->moduleExtensionList->getList();
    
  4. +++ b/core/modules/system/src/Form/ModulesListForm.php
    @@ -214,7 +217,7 @@ public function buildForm(array $form, FormStateInterface $form_state) {
    -   *   The list existing modules.
    +   *   The list of existing modules.
    

    Unrelated change.

  5. +++ b/core/modules/system/src/ModuleDependencyMessageTrait.php
    @@ -0,0 +1,53 @@
    +      // Check if the module is incompatible with the dependency constraints.
    +      $version = str_replace(\Drupal::CORE_COMPATIBILITY . '-', '', $modules[$dependency]->info['version']);
    +      if (!$dependency_object->isCompatible($version)) {
    +        $constraint_string = $dependency_object->getConstraintString();
    +        return $this->t('@module_name (<span class="admin-missing">incompatible with</span> version @version)', [
    +          '@module_name' => "$module_name ($constraint_string)",
    +          '@version' => $modules[$dependency]->info['version'],
    +        ]);
    +      }
    +
    +      // Check if the module is compatible with the installed version of core.
    +      if ($modules[$dependency]->info['core_incompatible']) {
    +        return $this->t('@module_name (<span class="admin-missing">incompatible with</span> this version of Drupal core)', [
    +          '@module_name' => $module_name,
    +        ]);
    +      }
    

    I think we should switch the order of these checks. Since return back the first message and don't show both messages.

    have a module is not compatible with this version of core seems more important also check $modules[$dependency]->info['core_incompatible'] is a less costly operation.

  6. +++ b/core/modules/system/tests/themes/test_theme_depending_nonexisting_module/test_theme_depending_on_nonexisting_module.info.yml
    @@ -0,0 +1,6 @@
    +name: test theme depending on nonexisting module
    +type: theme
    +core: 8.x
    +base theme: stark
    +dependencies:
    +  - test_module_non_existing
    

    I think all the test themes need
    version: VERSION

    it seems other test themes have this.

  7. +++ b/core/modules/system/tests/themes/test_theme_depending_on_modules/test_another_module_required_by_theme/test_another_module_required_by_theme.info.yml
    @@ -0,0 +1,5 @@
    +name: test another module required by theme
    +type: module
    +core: 8.x
    +package: Testing
    +version: VERSION
    

    Super-nit

    Most test themes and modules seem to use sentence case title and have description

bnjmnm’s picture

Status: Needs work » Needs review
FileSize
19.85 KB
48.97 KB

#229
1. ✅This indirectly exposed the fact that a module-depending theme could be enabled via drush without the dependee modules installed. Everything brought up here was addressed and received test coverage, including functionality that truly prevents installs, not just hiding it in the UI. This expanded test coverage also includes verifying a module that an active theme depends on can't be uninstalled.
2. ✅
3. ✅
4. ✅
5. ✅
6. ✅
7. ✅

Also updated a few comments and line breaks within them.
The other big change was reverting all changes to ModulesUninstallForm.php. This isn't necessary with the uninstall validator working properly.

bnjmnm’s picture

Status: Needs work » Needs review
FileSize
1.05 KB
48.92 KB

Fixed the test failure from #230. ModuleRequiredByThemesUninstallValidatorTest needed to update some of the strings it checked as the "Required by" language for themes better matched the language for modules.

tedbow’s picture

Status: Needs review » Needs work
  1. +++ b/core/modules/system/tests/themes/test_theme_depending_on_modules/test_module_required_by_theme/test_module_required_by_theme.info.yml
    @@ -0,0 +1,5 @@
    +name: test module required by theme
    
    +++ b/core/modules/system/tests/themes/test_theme_depending_on_modules/test_theme_depending_on_modules.info.yml
    @@ -0,0 +1,7 @@
    +name: test theme depending on modules
    
    +++ b/core/modules/system/tests/themes/test_theme_with_a_base_theme_depending_on_modules/test_theme_with_a_base_theme_depending_on_modules.info.yml
    @@ -0,0 +1,4 @@
    +name: test theme with a base theme depending on modules
    

    Sorry have mention before multiple info.yml should switch to title case

  2. +++ b/core/tests/Drupal/KernelTests/Core/Theme/ThemeInstallerTest.php
    @@ -137,6 +139,45 @@ public function testInstallNameTooLong() {
    +    try {
    +      $message = 'ThemeInstaller::install() throws MissingDependencyException upon installing a theme with unmet module dependencies.';
    +      $this->themeInstaller()->install([$name]);
    +      $this->fail($message);
    +    }
    +    catch (MissingDependencyException $e) {
    +      $this->pass(get_class($e) . ': ' . $e->getMessage());
    +    }
    

    Same here

  3. +++ b/core/tests/Drupal/KernelTests/Core/Theme/ThemeInstallerTest.php
    @@ -137,6 +139,45 @@ public function testInstallNameTooLong() {
    +    try {
    +      $message = 'Attempting to uninstall a module that an active theme depends on results in a ModuleUninstallValidatorException';
    +      $this->container->get('module_installer')->uninstall(['test_module_required_by_theme']);
    +      $this->fail($message);
    +    }
    +    catch (ModuleUninstallValidatorException $e) {
    +      $this->pass(get_class($e) . ': ' . $e->getMessage());
    +    }
    

    Instead of the try/catch we should be using \PHPUnit\Framework\TestCase::expectException() and \PHPUnit\Framework\TestCase::expectExceptionMessage(). This will also mean we can test for the exact message.

    I think because this not being backported we don't need to worry about compatiblity with earlier PHPUnit versions.

bnjmnm’s picture

Status: Needs work » Needs review
FileSize
5.97 KB
49.68 KB

Addresses #233 by title casing all the other info.yml files and refactoring the try catch checks for exceptions in ThemeInstallerTest

bnjmnm’s picture

Status: Needs work » Needs review
FileSize
4.67 KB
49.68 KB

ThemeUiTest needed updating to reflect the title case changes in #234

tedbow’s picture

Got to move on to another issue for now but had started a review so just noting this 1 issue. Will come back and do another full review

+++ b/core/lib/Drupal/Core/Extension/ModuleRequiredByThemesUninstallValidator.php
@@ -0,0 +1,97 @@
+      $module_name = $this->getModuleName($module);
...
+  protected function getModuleName($module) {
+    return $this->moduleExtensionList->get($module)->info['name'];
+  }

Does this actually need to be a function? It is only called once and it is just 1 line. Seem like we could use this 1 line directly

bnjmnm’s picture

#237

Does this actually need to be a function? It is only called once and it is just 1 line. Seem like we could use this 1 line directly

Nope - had it like that for an earlier iteration that needed it, but it's not necessary anymore.

tedbow’s picture

Status: Needs review » Needs work
  1. +++ b/core/core.services.yml
    @@ -547,6 +547,12 @@ services:
    +    arguments: ['@string_translation', '@extension.list.module', '@extension.list.theme', '@config.factory']
    

    I don't think the @config.factory is needed here as it is not in the constructor of ModuleRequiredByThemesUninstallValidator I guess no error is thrown for an extra argument.

  2. +++ b/core/modules/system/src/Controller/SystemController.php
    @@ -66,13 +77,20 @@ class SystemController extends ControllerBase {
    +    if ($module_extension_list === NULL) {
    +      @trigger_error('The extension.list.module service must be passed to ' . __NAMESPACE__ . '\SystemController::__construct. It was added in Drupal 8.9.0 and will be required before Drupal 9.0.0.', E_USER_DEPRECATED);
    +      $module_extension_list = \Drupal::service('extension.list.module');
    +    }
    

    Can we add a new deprecation in 8.9.0? If I search core for the regex @trigger_error.*8\.9 this the only 1 I see. But of course 8.9.x was not opened long ago. Just double checking

  3. +++ b/core/modules/system/src/ModuleDependencyMessageTrait.php
    @@ -0,0 +1,53 @@
    + * Messages for missing or incompatible dependencies on modules.
    ...
    +   * Provides messages for missing or incompatible dependencies on modules.
    

    Since this also handles incompatible core dependency should we change the wording to something like "missing modules or incompatible dependencies."

  4. +++ b/core/tests/Drupal/KernelTests/Core/Theme/ThemeInstallerTest.php
    @@ -104,21 +106,11 @@ public function testInstallSubTheme() {
    -
    -    $themes = $this->themeHandler()->listInfo();
    -    $this->assertEmpty(array_keys($themes));
    -
    -    try {
    -      $message = 'ThemeInstaller::install() throws UnknownExtensionException upon installing a non-existing theme.';
    -      $this->themeInstaller()->install([$name]);
    -      $this->fail($message);
    -    }
    -    catch (UnknownExtensionException $e) {
    -      $this->pass(get_class($e) . ': ' . $e->getMessage());
    -    }
    -
    ...
         $this->assertEmpty(array_keys($themes));
    +    $this->expectException(UnknownExtensionException::class);
    +    $this->expectExceptionMessage('Unknown themes: non_existing_theme.');
    +    $this->themeInstaller()->install([$name]);
    

    I think no changes are actually needed to this test. While I like the change to use expectException() I think we will get push back for an unrelated change.

  5. +++ b/core/tests/Drupal/KernelTests/Core/Theme/ThemeInstallerTest.php
    @@ -137,6 +129,34 @@ public function testInstallNameTooLong() {
    +  /**
    +   * Tests installing a theme with unmet module dependencies.
    +   */
    +  public function testInstallThemeWithUnmetModuleDependencies() {
    +    $name = 'test_theme_depending_on_modules';
    

    I think we should add a dataProvider here and test test_theme_with_a_base_theme_depending_on_modules also.

    We could also add a third test for theme that has it's own dependencies in addition to a base theme dependencies. That way we could be sure the combination works.

    If the dataProvider also had installed_modules(which would be installed by the test before calling install() and expected_missing_dependencies then we could test a theme with

    • Its own dependencies installed but not it's base themes
    • Its base theme's but not its own
    • both not installed

    We might also want to expand \Drupal\Tests\system\Functional\Theme\ThemeUiTest::testThemeInstallWithModuleDependencies() to also a test theme with both it's own dependencies and in its base theme. To make sure the UI message is correct. The kernel and functional test could use the same test theme.

bnjmnm’s picture

Status: Needs work » Needs review
FileSize
18.28 KB
55.41 KB

Takes care of #239, the biggest change is in \Drupal\Tests\system\Functional\Theme\ThemeUiTest::testThemeInstallWithModuleDependencies() which received quite a bit of refactoring to support the additional test cases.

tedbow’s picture

  1. re #239.2
    This be removed in 10.0.0 not 9.0.0
  2. +++ b/core/tests/Drupal/KernelTests/Core/Theme/ThemeInstallerTest.php
    @@ -137,6 +139,69 @@ public function testInstallNameTooLong() {
    +      'test_theme_mixed_module_dependencies' => [
    ...
    +      'test_theme_mixed_module_dependencies - node enabled' => [
    ...
    +      'test_theme_mixed_module_dependencies - test modules enabled' => [
    

    I think the keys here for the test cases don't make sense.
    Maybe node module was used before.

    Instead of test_theme_mixed_module_dependencies maybe, theme with neither own or base theme's required modules installed
    instead of test_theme_mixed_module_dependencies - node enabled maybe theme with own module dependencies installed, not base theme's

    instead of test_theme_mixed_module_dependencies - test modules enabled maybe theme with base theme module dependencies installed, not its own

    Or something like that so the key explicitly tells what is being tested. I don't think we need the theme name in those keys.

  3. +++ b/core/modules/system/tests/src/Functional/Theme/ThemeUiTest.php
    @@ -0,0 +1,311 @@
    +  public function testThemeInstallWithModuleDependencies(
    +    $theme_name,
    +    $first_expected_required_list_items,
    +    $first_module_enable,
    +    $first_confirm_checked,
    +    $second_expected_required_list_items,
    +    $second_module_enable,
    +    $second_confirm_checked,
    +    $disabled_attributes,
    +    $required_by_messages,
    +    $module_uninstall_message
    +  ) {
    

    I think the method arguments are suppose to be on 1 line regardless of how many but could be wrong.

    This test failed. It is kinda of hard to follow with all the arguments but I couldn't figure out a suggestion to make it simpler.

bnjmnm’s picture

Status: Needs work » Needs review
FileSize
20.98 KB
57.25 KB

#242.1 - Changed to 10.0.0
#242.2 - Revised keys, that makes sense
#242.3 - was already in the process of refactoring the test/provider to make it more coherent and address the failed test, which fortunately addresses your comments here.

tedbow’s picture

Status: Needs review » Needs work
+++ b/core/modules/system/tests/src/Functional/Theme/ThemeUiTest.php
@@ -0,0 +1,325 @@
+  public function testThemeInstallWithModuleDependencies($data) {

I couldn't find another case where we pass all the test case arguments in one $data argument. I found some test methods like

public function testSetData($data) {

But in that case $data is being used on a set method and it is not array for test case arguments. Even though it is a lot of arguments I still think we should pass them individually. I was commenting that I think they should all be in 1 line.

One advantage we get by passing them as separate arguments is that phpunit will fail the test if the number of arguments is incorrect.

Otherwise the interdiff looks good. I will give the whole patch another look later.

bnjmnm’s picture

I actually made the switch to $data before I'd seen the feedback regarding the number of function arguments - my intent was to make it easier to identify the provider data within the test. This approach made troubleshooting the tests much easier and I think it would be beneficial to keep it structured like that. I could do an assertCount($data) at the beginning of the test to get the benefits of separate arguments. Any thoughts on that?

bnjmnm’s picture

Status: Needs work » Needs review
FileSize
16.23 KB
57.24 KB

@tedbow reminded me of a much easier way to structure the data provider. Here's that change.

tedbow’s picture

Status: Needs review » Needs work
  1. I like the test changes in #246
  2. +++ b/core/lib/Drupal/Core/Extension/ThemeInstaller.php
    @@ -116,16 +118,25 @@ public function install(array $theme_list, $install_dependencies = TRUE) {
    +          $missing_module_dependencies_list = implode(', ', array_keys($missing_module_dependencies));
    +          throw new MissingDependencyException("Unable to install theme: '$theme' due to missing module dependencies: '$missing_module_dependencies_list.'");
    

    This very similar to the exception we throw in \Drupal\Core\Extension\ModuleInstaller::install()

    throw new MissingDependencyException(sprintf('Unable to install modules %s due to missing modules %s.', implode(', ', $module_list), implode(', ', $missing_modules)));
    

    But in the other case "missing" means they don't exist not just they aren't install(I think).

    So I think the wording should a little bit different to denote they just aren't installed.

  3. +++ b/core/modules/system/src/Controller/SystemController.php
    @@ -231,9 +250,44 @@ public function themesPage() {
    +        foreach ($theme->module_dependencies as $dependency => $dependency_object) {
    +          if ($incompatible = $this->checkDependencyMessage($modules, $dependency, $dependency_object)) {
    +            $theme->module_dependencies[$dependency] = $incompatible;
    ...
    +          $theme->module_dependencies[$dependency] = $modules[$dependency]->status ? $this->t('@module_name', ['@module_name' => $module_name]) : $this->t('@module_name (<span class="admin-disabled">disabled</span>)', ['@module_name' => $module_name]);
    

    $theme->module_dependencies seems to start as an array of objects but then we re-assign the values as strings. I think we should avoid this.

  4. +++ b/core/modules/system/system.admin.inc
    @@ -131,6 +132,18 @@ function template_preprocess_system_admin_index(&$variables) {
    +        // Add themes to the module's required by list.
    +        $form[$dependency]['#required_by'][] = $theme->info['name'] . ' (' . t('Theme') . ')' . (!$theme->status ? ' (' . t('Disabled') . ')' : '');
    

    Do we have functional tests for this message? I couldn't find it.
    If not I think we should. Maybe it should be in \Drupal\Tests\system\Functional\Form\ModulesListFormWebTest(). Maybe there is no test for the existing "Required by" for modules.

HongPong’s picture

Would just like to suggest that it may be a good idea to have 'recommended' modules as well as required.

tedbow’s picture

@HongPong this issue is required to ship Drupal 9 I think so I don't think we should add anything else to it. If people want the extra feature it should be created in a new issue that would be a follow up. Of course the other reason not to add anything else is that this issue is over 10 years old already 🙀

For anyone interested there is a related issue for "recommends" for modules, #328932: Modules and partial dependencies - enhances[] and enhancedby[] field in modules' .info.yml files (though changed to different wording this is similar) and 1 for profiles #820054: Add support for recommends[] and fix install profile dependency special casing

Tagging with "Needs Issue Summary Update" so it's connection to Drupal 9 can be documented.

Also the summary says "Install dependencies automatically on API level." which this issue decided to do in a follow-up #3100374: Make it possible to install dependent modules when installing theme, so that should be updated in the summary. Not sure about the rest right now

bnjmnm’s picture

Issue summary: View changes
Status: Needs work » Needs review
Issue tags: -Needs issue summary update
FileSize
6.92 KB
60.01 KB

Addresses feedback in #247 and updated issue summary.

bnjmnm’s picture

Status: Needs work » Needs review
FileSize
915 bytes
60.01 KB

The exception message changed as a result of addressing ##24.2, had to update ThemeInstallerTest to reflect that change.

tedbow’s picture

  1. The change in #250 + #252 look for addressing my feedback from #247
  2. Added #3100374: Make it possible to install dependent modules when installing theme to summary
  3. Moved the part in the issue summary about validation on installing from "API changes" to "Proposed resolution". This doesn't actually create a new API
  4. added #3005229: Provide optional support for using composer.json for dependency metadata to summary to say we should follow up in that issue to allow composer.json dependencies for themes too
  5. +++ b/core/core.services.yml
    @@ -547,6 +547,12 @@ services:
         arguments: ['@app.root', '@config.factory', '@extension.list.theme']
    

    We should inject this. @Wim Leers mention this in #207.3 and suggested if we can't for some reason we should put in a comment.

    I didn't see an explanation in the comments why we can't so I think we can inject it.

  6. +++ b/core/lib/Drupal/Core/Extension/ThemeExtensionList.php
    @@ -140,6 +141,16 @@ protected function doList() {
    +      if (!isset($theme->requires)) {
    +        $theme->requires = [];
    +      }
    +      $themes[$key]->module_dependencies = isset($theme->base_themes) ? array_diff_key($theme->requires, $theme->base_themes) : $theme->requires;
    

    Do we need $theme->requires to be set except for the next line?

    Looking at this and figuring out that in \Drupal\Core\Extension\ThemeExtensionList::createExtensionInfo() we add 'base theme' to dependencies which why have the existing call to \Drupal\Core\Extension\ModuleHandlerInterface::buildModuleDependencies().

    One problem that since dependencies is generic developers might think they can other themes to that setting in their theme .info.yml files. If they did then they would be added to module_dependencies and they would be labeled as "missing" in the UI(I tested this).

    One way to avoid this problem would be to instead do
    $themes[$key]->module_dependencies = isset($theme->requires) ? array_diff_key($theme->requires, $themes) : [];

    If we don't need $theme->requires this is simpler and will remove all themes from module_dependencies In addition understanding code doesn't require you know how what $theme->base_themes is, which is nice this just an documented property.

    UPDATE: I see know we do need $theme->requires in \Drupal\Core\Extension\ThemeInstaller::install() but still I like doing the array_key_diff on $themes since we already have it.

  7. +++ b/core/modules/system/tests/src/Functional/Theme/ThemeUiTest.php
    @@ -0,0 +1,319 @@
    +    $this->drupalLogin($this->adminUser = $this->drupalCreateUser([
    +      'administer themes',
    +      'administer modules',
    +    ]));
    +  }
    

    This is probably a copy paste error. We don't need $this->adminUser = here. We never use property.

  8. +++ b/core/modules/system/tests/src/Functional/Theme/ThemeUiTest.php
    @@ -0,0 +1,319 @@
    +    $this->assertContains('missing', $theme_container->getText());
    

    We should assert the whole string 'Requires: test_module_non_existing (missing)' to ensure the correct module is listed.

    Also we should assert that the link to /admin/modules does not exist.

  9. Just want to point out that what we support module info.yml files and theme info.yml files in the dependency is different

    In module we support just the module name or drupal:MODULE_NAME or "drupal:MODULE_NAME (>2.0)"

    Maybe this fine but I am guessing we want a follow up to at least support version constraints.
    The other option is simply to not allow this until this and if we want to support themes in #3005229: Provide optional support for using composer.json for dependency metadata we could say you have to use composer.json if you want this.

    In this patch we could throw an exception in \Drupal\Core\Extension\InfoParserDynamic::parse() if type === 'theme' and there is anything but a valid extension name. That way developer could get an exception we don't support constraints in theme declared module dependencies.

bnjmnm’s picture

Status: Needs work » Needs review
FileSize
5.99 KB
63.24 KB

This addresses #253

tedbow’s picture

Status: Needs review » Needs work
  1. +++ b/core/lib/Drupal/Core/Extension/InfoParserDynamic.php
    @@ -120,6 +120,19 @@ public function parse($filename) {
    +      if ($parsed_info['type'] === 'theme' && isset($parsed_info['dependencies'])) {
    +        $non_machine_name_dependencies = preg_grep('/[^a-z0-9_]+/', $parsed_info['dependencies']);
    +        if (!empty($non_machine_name_dependencies)) {
    +          throw new InfoParserException("Theme module dependencies must be machine names (only underscores, digits, and lowercase letters). At least one dependency declared by {$parsed_info['name']} is not in machine name format: " . implode(',', $non_machine_name_dependencies));
    +        }
    +
    +      }
    

    This looks good for blank line before the last bracket.

    We need test coverage for this in \Drupal\Tests\Core\Extension\InfoParserUnitTest

  2. +++ b/core/lib/Drupal/Core/Extension/ThemeExtensionList.php
    @@ -149,7 +149,7 @@ protected function doList() {
           if (!isset($theme->requires)) {
             $theme->requires = [];
           }
    -      $themes[$key]->module_dependencies = isset($theme->base_themes) ? array_diff_key($theme->requires, $theme->base_themes) : $theme->requires;
    +      $themes[$key]->module_dependencies = isset($theme->requires) ? array_diff_key($theme->requires, $themes) : [];
    

    hmm. my last snippet was wrong.

    $theme->requires will always be set because we just set it.
    we can assign as $theme->module_dependencies instead of $themes[$key]->module_dependencies since objects are always passed as a reference. Either way we should consistent inside the for loop. The we could get rid of $key altogether.
    I think we could just do

    if (!isset($theme->requires)) {
      $theme->requires = [];
      $themes->module_dependencies = [];
    }
    else {
      $theme->module_dependencies = array_diff_key($theme->requires, $themes);
    }
    
    
    
    
bnjmnm’s picture

Status: Needs work » Needs review
FileSize
67.27 KB
5.92 KB

Of course that needs unit tests! That and everything else from #255 is addressed here.

tedbow’s picture

+++ b/core/lib/Drupal/Core/Extension/ThemeExtensionList.php
@@ -142,14 +142,20 @@ protected function doList() {
+      if (isset($theme->requires)) {
+        $theme->module_dependencies = isset($theme->requires) ? array_diff_key($theme->requires, $themes) : [];
+      }

Something tells me inside this particular if block isset($theme->requires) will always be true 😉

we can just directly assign the return value of array_diff_key call.

bnjmnm’s picture

tedbow’s picture

Status: Needs review » Needs work

Just a couple things and I think we are RTBC!

  1. +++ b/core/lib/Drupal/Core/Extension/InfoParserDynamic.php
    @@ -120,6 +120,18 @@ public function parse($filename) {
    +      // format. The ability to specify version constraints or parse project
    +      // prefixes will be available when it becomes possible for composer.json
    +      // to support dependency metadata.
    

    I don't think we should mention composer.json at all. I am not sure in #474684: Allow themes to declare dependencies on modules if we will actually be able to support that.

    We should say though that constraints are not supported. Regardless of what happens in 474684 will will still need this validation logic.

    I commented on 474684 just now and relating it to this and mentioned should look into theme support.

  2. +++ b/core/tests/Drupal/Tests/Core/Extension/InfoParserUnitTest.php
    @@ -534,4 +534,94 @@ public function testUnparsableCoreVersionRequirement() {
    +# info.yml for testing invalid core_version_requirement value.
    ...
    +# info.yml for testing invalid core_version_requirement value.
    ...
    +# info.yml for testing invalid core_version_requirement value.
    

    This comment is copied and doesn't apply. I don't actually think we need any comments but it should either be removed or updated.

  3. +++ b/core/tests/Drupal/Tests/Core/Extension/InfoParserUnitTest.php
    @@ -534,4 +534,94 @@ public function testUnparsableCoreVersionRequirement() {
    +        'Theme module dependencies must be machine names (only underscores, digits, and lowercase letters). At least one dependency declared by Test Theme Depending on Modules is not in machine name format: module_with_some_version_metadata (>4.1)',
    

    I think we should include the filename with path instead of human readable name like the other exceptions shown in this test class.

bnjmnm’s picture

Status: Needs work » Needs review
FileSize
5.73 KB
67.01 KB

Addresses #259

alexpott’s picture

  1. I've done some manual testing via the UI and using drush to install and uninstall themes and modules they depend on and things are looking pretty good. The system is working as I would expect - you can't uninstall a module via the UI or Drush if an installed theme depends on it. If the theme is not installed you can uninstall the module. And you can't install a theme if a module it depends on is not installed. Nice work.
  2. We should catch the MissingDependencyException in \Drupal\system\Controller\ThemeController::install() and present a decent message to the user just in case. You can test this by
    1. loading the admin/appearance page
    2. hacking an uninstalled theme to be dependent on a missing module
    3. clicking on that themes install button.
  3. +++ b/core/lib/Drupal/Core/Extension/ModuleInstaller.php
    @@ -50,6 +50,13 @@ class ModuleInstaller implements ModuleInstallerInterface {
    +  /**
    +   * The theme extension list.
    +   *
    +   * @var \Drupal\Core\Extension\ThemeExtensionList
    +   */
    +  protected $themeExtensionList;
    

    Whilst it is not necessary because the theme extension list is always used before uninstalling a module I think we should update the themeExtensionList in \Drupal\Core\Extension\ModuleInstaller::updateKernel() since after updating the kernel the service in the container will not be the same.

  4. +++ b/core/modules/system/system.admin.inc
    @@ -131,6 +132,18 @@ function template_preprocess_system_admin_index(&$variables) {
    +        $form[$dependency]['#required_by'][] = $theme->info['name'] . ' (' . t('Theme') . ')' . (!$theme->status ? ' (' . t('Disabled') . ')' : '');
    

    I think this needs to match

    $row['#required_by'][$dependent] = $this->t('@module (<span class="admin-disabled">disabled</span>)', ['@module' => $modules[$dependent]->info['name']]);
    

    Yep I've tested this manually and it doesn't look right. See attached screenshot.

  5. Module dependencies often are in the format
    dependencies:
      - drupal:file
      - drupal:options
    

    But at the moment we don't support the project:module format just something like:

    dependencies:
      - file
      - options
    

    That's confusing and potential source of frustration and also problematic if you want to depend on a duplicate module from a project.

  6. Given the last point I'm now also wondering about the impact on DrupalCI and how that'd know to download a contrib module when its running tests.
alexpott’s picture

Here's the screenshot for #261.4

tedbow’s picture

Status: Needs review » Needs work

Looking at @alexpott's comments #261.5 I think my suggestion in #253.9 was wrong.

I think we actually might have constraint support right now. It just wasn't tested so I didn't notice. I tested removing the changes to \Drupal\Core\Extension\InfoParserDynamic() and constraints work. Probably - drupal:module will also work!

This is because \Drupal\Core\Extension\ModuleHandler::buildModuleDependencies() uses \Drupal\Core\Extension\Dependency::createFromString() and \Drupal\system\ModuleDependencyMessageTrait::checkDependencyMessage() also uses \Drupal\Core\Extension\Dependency::isCompatible(). So we already will show a message if the module is not compatible with the constraint set in the theme .info.yml file. We just need test coverage.

The only place we don't have it is in \Drupal\Core\Extension\ThemeInstaller::install() which would allow you to install themes where their dependencies are not compatible. Unfortunately the same problem exists in \Drupal\Core\Extension\ModuleInstaller::install() which does check isCompatible(). So we rely on form validation to stop that. I think drush and drupal console have added validation.

But since we are installing modules automatically when you enable a theme the situation is different. The current patch will already not allow you install a theme if the dependency is present but not compatible, this is good.

But I don't think we should do the same as \Drupal\Core\Extension\ModuleInstaller::install(). I think we should throw an exception if we have an incompatible dependency installed in \Drupal\Core\Extension\ThemeInstaller::install() it should be easy as we already have the dependency object and just to check \Drupal\Core\Extension\Dependency::isCompatible().

We also should test coverage for the incompatible dependency message on the theme page and that the link doesn't show up to install or go to the extend page. I think this should all already work.

tedbow’s picture

For another issue I just realized that in system_install() in the update phase we check if the module has missing or incompatible dependencies.

While you I don't think theme support hook_update_n should we also be doing the same check on themes here? Otherwise where would you be notified if you dependent module went missing or if dependent module was not longer compatible either because the constraint on the theme .info.file change to higher version or if module version was changed?(assuming I am correct about constraint support in #263)

bnjmnm’s picture

This addresses the feedback in #261, #263, and #264 with one exception (to the best of my knowledge):
One of the changes in this patch is modifying system_install() to include themes in the checks that were previously for modules only. There isn't test coverage yet for this change. It's not immediately clear how to do this, so I'm posting my progress while I reference various tests in Drupal\Tests\system\Functional\UpdateSystem to figure it out.

tedbow’s picture

@bnjmnm look at the last asserts in \Drupal\Tests\system\Functional\UpdateSystem\UpdateScriptTest::testRequirements()

\Drupal::state()->set('update_script_test.system_info_alter', ['dependencies' => ['node (<7.x-0.0-dev)']]);

This is used in \update_script_test_system_info_alter() alter the dependencies in the update_script_test module.

You would have to do something like this but with a theme(you could just using an existing theme or make test theme)

You could do it in the same test method.

bnjmnm’s picture

Status: Needs work » Needs review
FileSize
4.29 KB
76.63 KB

Updated test coverage for system_install() per the suggestions in #266.

tim.plunkett’s picture

Status: Needs review » Needs work

One template issue, some docs needs, and some nits. The changes in #265 and #267 look great, and I think sufficiently address @alexpott and @tedbow's concerns.

  1. +++ b/core/modules/system/templates/system-themes-page.html.twig
    @@ -62,6 +63,9 @@
    +              <div class="theme-info__requires">Requires: {{ theme.module_dependencies }}</div>
    

    Untranslatable?

  2. +++ b/core/modules/system/tests/src/Functional/Theme/ThemeUiTest.php
    @@ -0,0 +1,330 @@
    +  /**
    +   * Tests installing a theme with module dependencies.
    +   *
    +   * @dataProvider providerTestThemeInstallWithModuleDependencies
    +   */
    +  public function testThemeInstallWithModuleDependencies($theme_name, array $first_expected_required_list_items, array $first_module_enable, array $first_confirm_checked, array $second_expected_required_list_items, array $second_module_enable, array $second_confirm_checked, array $module_names, array $required_by_messages, $module_uninstall_message, $base_theme_to_uninstall, array $base_theme_module_names, $base_theme_module_uninstall_message) {
    ...
    +  /**
    +   * Data provider for testThemeInstallWithModuleDependencies().
    +   */
    +  public function providerTestThemeInstallWithModuleDependencies() {
    

    Missing documentation of the params, both on the test function and within the provider function.

    See \Drupal\Tests\update\Unit\ModuleVersionTest, check
    testGetVersionExtra() and providerVersionInfos()

  3. +++ b/core/modules/system/tests/themes/test_theme_depending_on_modules/test_module_required_by_theme/test_module_required_by_theme.module
    @@ -0,0 +1,20 @@
    +  if($file->getName() == 'test_theme_depending_on_modules') {
    

    Nit: missing space after `if`

  4. +++ b/core/tests/Drupal/KernelTests/Core/Theme/ThemeInstallerTest.php
    @@ -137,6 +139,79 @@ public function testInstallNameTooLong() {
    +   * @dataProvider providerTestInstallThemeWithUnmetModuleDependencies
    +   */
    +  public function testInstallThemeWithUnmetModuleDependencies($theme_name, $installed_modules, $message) {
    

    Same as above with docs

  5. +++ b/core/themes/claro/templates/system-themes-page.html.twig
    @@ -97,6 +98,9 @@
    +                  <div class="theme-info__requires">Requires: {{ theme.module_dependencies }}</div>
    
    +++ b/core/themes/stable/templates/admin/system-themes-page.html.twig
    @@ -60,6 +61,9 @@
    +              <div class="theme-info__requires">Requires: {{ theme.module_dependencies }}</div>
    

    Same as the other template

bnjmnm’s picture

Status: Needs work » Needs review
FileSize
80.02 KB
8.09 KB

Yikes thats a lotta @param documentation, but that alone demonstrates how much it needed to be added.
This covers the feedback in #268

lauriii’s picture

+++ b/core/modules/system/templates/system-themes-page.html.twig
@@ -64,7 +64,7 @@
-              <div class="theme-info__requires">Requires: {{ theme.module_dependencies }}</div>
+              <div class="theme-info__requires">{{ 'Requires:'|t }} {{ theme.module_dependencies }}</div>

Should we use a placeholder for printing the dependencies?

xjm’s picture

bnjmnm’s picture

Placeholdered re #270

lauriii’s picture

I reviewed most of the non-test code and this is all I found so far. I'm planning to extend my review to the full patch but I thought it might be useful to post this before.

  1. +++ b/core/lib/Drupal/Core/Extension/ModuleRequiredByThemesUninstallValidator.php
    @@ -0,0 +1,84 @@
    +  /**
    
    +++ b/core/modules/system/src/ModuleDependencyMessageTrait.php
    @@ -0,0 +1,53 @@
    +      return $this->t('@module_name (<span class="admin-missing">missing</span>)', ['@module_name' => $dependency]);
    ...
    +        return $this->t('@module_name (<span class="admin-missing">incompatible with</span> this version of Drupal core)', [
    ...
    +        return $this->t('@module_name (<span class="admin-missing">incompatible with</span> version @version)', [
    

    Do we have any better for generating this markup? It's a bit strange that the markup is in the translation string and this is not overridable by themes. It's also a bit strange that we generate these messages always with markup and then sometimes strip out the markup the markup. 🤔

  2. +++ b/core/modules/system/system.admin.inc
    @@ -131,6 +132,18 @@ function template_preprocess_system_admin_index(&$variables) {
    +        $form[$dependency]['#required_by'][] = $theme->status ? t('@theme', ['@theme (theme)' => $theme->info['name']]) : t('@theme (theme) (<span class="admin-disabled">disabled</span>)', ['@theme' => $theme->info['name']]);
    

    There's some markup in the translatable string here too which should be moved somewhere else.

tedbow’s picture

re #273

This is not ideal but also these are message that show up on the module form and there are other similar messages on the form we are not changing.

Is it more or less frustrating if you can alter the markup for some of the messages on the module form but not others? Should we do 1 issue to change them all?

markhalliwell’s picture

I'd rather this issue do what's already been done with modules (for current consistency sake).

We should open a new issue to address both in an abstract way though, perhaps creating a couple new template suggestions:

- item_list__extension__requirements
- item_list__extension__dependencies

We may be able to get away with preprocessing each item and adding additional metadata that can be appended (i.e. missing/disabled, etc).

If not, we may need to just create a new extension_label template along with it; but I think that will likely be overkill.

Regardless of how we solve it in the future, changing anything from what modules are currently doing is really out of scope for this issue now.

tedbow’s picture

I talked with @lauriii after my comment in #274 and we pretty much came up with the same conclusion as @markcarver in #275

The strings in ModuleDependencyMessageTrait are existing strings that were in \Drupal\system\Form\ModulesListForm but were pull out to the trait so we could share the same wording in the theme page. I think the trait is good to keep the wording from differing. So these should not be changed here.

The #require_by message in template_preprocess_system_admin_index() also is in a list with the existing items. So it would seem not to make sense to have some items in the list be overridable and others not.

created this follow-up #3111128: Allow the markup to be overridable for dependency and compatibility messages on the module and theme admin pages and added @markcarver's suggestion as a starting point

lauriii’s picture

  1. Are we concerned that themes that have dependencies on hidden modules, cannot be installed through the UI until #3100374: Make it possible to install dependent modules when installing theme has been resolved?
  2. +++ b/core/modules/system/src/Controller/SystemController.php
    @@ -231,9 +250,44 @@ public function themesPage() {
    +          if (!empty($modules[$dependency]->hidden)) {
    +            continue;
    +          }
    
    +++ b/core/modules/system/src/Form/ModulesListForm.php
    @@ -326,38 +329,19 @@ protected function buildRow(array $modules, Extension $module, $distribution) {
    +      if (!empty($modules[$dependency]->hidden)) {
    +        continue;
           }
    

    Shouldn't this be $modules[$dependency]->info['hidden']? If it's so, we should ensure we have test coverage for this.

bnjmnm’s picture

#277.1

Are we concerned that themes that have dependencies on hidden modules, cannot be installed through the UI until #3100374: Make it possible to install dependent modules when installing theme has been resolved?

. We'd at least need to make sure this limitation is known to users. Obviously in the CR, but perhaps it would be helpful to integrate this info into the themes UI?

#277.2 So you're correct that it should be $modules[$dependency]->info['hidden'], not $modules[$dependency]->hidden. It turns out the incorrect use of $modules[$dependency]->hidden has been the logic of ModulesListForm since 8.0, added in #1990544: Convert system_modules() to a Controller. This patch doesn't add this to that class, just moves it a bit. Altering this would change existing/expected (albeit incorrect) behavior that has been present for all of D8.

+++ b/core/modules/system/src/Form/ModulesListForm.php
@@ -326,38 +329,19 @@ protected function buildRow(array $modules, Extension $module, $distribution) {
+      // Only display missing or visible modules.
+      if (!empty($modules[$dependency]->hidden)) {
+        continue;
       }
-      // Only display visible modules.
-      elseif (empty($modules[$dependency]->hidden)) {
-        $name = $modules[$dependency]->info['name'];
-        // Disable the module's checkbox if it is incompatible with the
-        // dependency's version.

Of course, this incorrect use is also copied to SystemController which propagates the problem. I don't immediately know if consistency is the priority like it was for the string output discussed in 273-276, and I'm not sure what would be considered in-scope for this issue. I'm not yet sure what the best approach would be so feedback is welcome.

bnjmnm’s picture

I discussed #277 with @lauriii and @gábor-hojtsy and here's our current take:
#277.1

Are we concerned that themes that have dependencies on hidden modules, cannot be installed through the UI until #3100374: Make it possible to install dependent modules when installing theme has been resolved?

As long as this is made clear in the CR, this is acceptable.

#277.2

Shouldn't this be $modules[$dependency]->info['hidden']? If it's so, we should ensure we have test coverage for this.

As I discovered in #278, this check for the wrong property has been in Drupal since 8.0. The empty() check always returns true. It's not in the scope of this issue to fix that, so a followup was created #3117829: Hidden modules are visible in the "depends on: list. It does seem in scope to remove it, since this issue requires we do something with it. This patch removes the two incorrectly structured checks for 'hidden' and replaces them with a todo for #3117829.

lauriii’s picture

This addresses my concerns in #277.

catch’s picture

Just reviewed this specifically to see if there was anything to prevent commit of this during the (9.0.x and also 8.9.x) beta, and I think it is OK from that perspective.

We add a new validator, a new trait, add some markup to the template for the system themes admin page, and do some constructor deprecations - none of these should be disruptive. Maybe the template page could be but it'd require an admin theme overriding that specific template, and the 'disruption' would be the new messaging not showing up properly.

The actual functionality (new YAML key and validation on install etc.) is very optional for theme developers and not deprecating any old ways of doing things, so also not disruptive.

There is one issue, which also exists for module dependencies on modules, which is that if you already have a theme installed, and it adds a dependency on a module, and you upgrade without also installing the module, then it's going to break. Therefore themes using this feature should probably increment a minor version and communicate the new dependency in the release notes.

It would also be good to double check that if you install a theme with a module dependency specified in composer.json, that composer brings it all in properly. Can't see why it wouldn't since you can already install themes via composer, and composer doesn't know that they're themes as such.

The change record should be updated to:
1. Mention adding the dependency in composer.json as well as .info.yml
2. Mention incrementing a minor version and/or communicating the new dependency in release notes
3. Mention the template markup change

We will also need updates to the developer documentation on Drupal.org https://www.drupal.org/docs/8/creating-custom-modules/add-a-composerjson... doesn't really mention themes - maybe that can be generalised to include them? But not sure how that fits with the handbook IA.

Having said all that, it would still be much better to get this committed prior to the beta release(s) so that themes can start to rely on it.

tedbow’s picture

+++ b/core/modules/system/system.install
@@ -901,7 +901,9 @@ function system_requirements($phase) {
-    $files = \Drupal::service('extension.list.module')->getList();
+    $module_files = \Drupal::service('extension.list.module')->getList();
+    $theme_files = \Drupal::service('extension.list.theme')->getList();
+    $files = array_merge($module_files, $theme_files);
     foreach ($files as $module => $file) {

This section is need anymore after #2917600: update_fix_compatibility() puts sites into unrecoverable state because it did the same thing

That issue also added a todo that pointed to the current issue that I addressed in this reroll.

Otherwise this is straight reroll.

tedbow’s picture

@catch re #281

Therefore themes using this feature should probably increment a minor version and communicate the new dependency in the release notes.

Did you mean major version? Since themes don't have semantic version releases yet they only have "patch" level though sometimes it is called "minor".

catch’s picture

I tend to think of modules as having minor and patch but no majors, but yeah I guess a major version if we think of them having only major and patch but no minors.

tedbow’s picture

I was giving this issue a review and was taking look at \Drupal\Tests\system\Functional\Theme\ThemeUiTest::testThemeInstallWithModuleDependencies()

This method has 13 parameters 🙀.

I commented on this before in #242.3

It is kinda of hard to follow with all the arguments but I couldn't figure out a suggestion to make it simpler.

@bnjmnm improved the readability of the this function but we still had the same number of paramaters.

I decided to take a fresh look at this and see if I can get it to have less parameters so it is easier to follow. I was able to get it down to 6 parameters. I don't the function is too complicated.

I still need to update some comments and maybe rename some parameters now to match the changes but I wanted to post this patch so the interdiff would show how much as been eliminated.

tedbow’s picture

Updated parameters names and code comments of testThemeInstallWithModuleDependencies() because of changes in #286

alexpott’s picture

+++ b/core/modules/system/tests/themes/test_theme_depending_nonexisting_module/test_theme_depending_on_nonexisting_module.info.yml
@@ -0,0 +1,7 @@
+core: 8.x

+++ b/core/modules/system/tests/themes/test_theme_depending_on_constrained_modules/test_module_compatible_constraint/test_module_compatible_constraint.info.yml
@@ -0,0 +1,5 @@
+core: 8.x

+++ b/core/modules/system/tests/themes/test_theme_depending_on_constrained_modules/test_module_incompatible_constraint/test_module_incompatible_constraint.info.yml
@@ -0,0 +1,5 @@
+core: 8.x

+++ b/core/modules/system/tests/themes/test_theme_depending_on_constrained_modules/test_theme_depending_on_constrained_modules.info.yml
@@ -0,0 +1,7 @@
+core: 8.x

+++ b/core/modules/system/tests/themes/test_theme_depending_on_modules/test_another_module_required_by_theme/test_another_module_required_by_theme.info.yml
@@ -0,0 +1,5 @@
+core: 8.x

+++ b/core/modules/system/tests/themes/test_theme_depending_on_modules/test_module_required_by_theme/test_module_required_by_theme.info.yml
@@ -0,0 +1,5 @@
+core: 8.x

+++ b/core/modules/system/tests/themes/test_theme_depending_on_modules/test_theme_depending_on_modules.info.yml
@@ -0,0 +1,7 @@
+core: 8.x

+++ b/core/modules/system/tests/themes/test_theme_mixed_module_dependencies/test_theme_mixed_module_dependencies.info.yml
@@ -0,0 +1,6 @@
+core: 8.x

+++ b/core/modules/system/tests/themes/test_theme_with_a_base_theme_depending_on_modules/test_theme_with_a_base_theme_depending_on_modules.info.yml
@@ -0,0 +1,4 @@
+core: 8.x

I think we can get rid of all of these. As these are all part of core. It'll help get this to pass on 9.0.x too.

tedbow’s picture

  1. #287 good call. Removing
  2. +++ b/core/modules/system/tests/src/Functional/UpdateSystem/UpdateScriptTest.php
    @@ -175,6 +185,31 @@ public function testRequirements() {
    diff --git a/core/modules/system/tests/themes/test_theme_depending_nonexisting_module/test_theme_depending_on_nonexisting_module.info.yml b/core/modules/system/tests/themes/test_theme_depending_nonexisting_module/test_theme_depending_on_nonexisting_module.info.yml
    

    Looking at this file that @alexpott mentioned I notice the folder name doesn't match the info file. Fixed

saschaeggi’s picture

Great feature request. Waiting for this for a long time. Let's hope we can get this asap in core :)

jungle’s picture

Looks great to me.

  1. +++ b/core/lib/Drupal/Core/Extension/ModuleInstaller.php
    @@ -59,14 +66,21 @@ class ModuleInstaller implements ModuleInstallerInterface {
    +    if (is_null($extension_list_theme)) {
    

    But sorry, maybe out of the scope here. === NULL vs is_null, which one is recommended? Is it necessary to open an issue to do a benchmark?

    In PHP 7 (phpng), is_null is actually marginally faster than ===, although the performance difference between the two is far smaller.

    PHP 5.5.9
    is_null - float(2.2381200790405)
    === - float(1.0024659633636)
    === faster by ~100ns per call

    PHP 7.0.0-dev (built: May 19 2015 10:16:06)
    is_null - float(1.4121870994568)
    === - float(1.4577329158783)
    is_null faster by ~5ns per call

    See https://www.php.net/manual/en/function.is-null.php#117344

  2. +++ b/core/lib/Drupal/Core/ProxyClass/Extension/ModuleRequiredByThemesUninstallValidator.php
    @@ -0,0 +1,88 @@
    +     * @see \Drupal\Component\ProxyBuilder
    +     */
    

    \Drupal\Component\ProxyBuilder is a namespace, presumably, it's better changing it to \Drupal\Component\ProxyBuilder\ProxyBuilder

    As this file is generated, should change it from the source. About 28 matches found. Or do this with a separate issue.

catch’s picture

#1 we don't have a convention, core has examples of both. Interesting that PHP 7 might be faster with is_null().

For #2 let's move that to a separate issue with ProxyBuilder - if we change it we'll probably also want to rebuild all the proxy classes.

xjm’s picture

Issue tags: +Needs followup

Yeah if we're going to start avoiding === NULL we should also make that something we explore in a followup and adopt as a coding standard. So tagging "Needs followup" for both issues in #290.

jungle’s picture

Assigned: Unassigned » jungle

Creat follow-up issues

jungle’s picture

tim.plunkett’s picture

There's some fuzz with the patch when applying to 9.0.x, look out for that.
Here's a review, a lot of nits. Overall I think this is close!

  1. +++ b/core/lib/Drupal/Core/Extension/ModuleInstaller.php
    @@ -59,14 +66,21 @@ class ModuleInstaller implements ModuleInstallerInterface {
    +      @trigger_error('The extension.list.theme service must be passed to ' . __NAMESPACE__ . '\ModuleInstaller::__construct. It was added in Drupal 8.9.0 and will be required before Drupal 10.0.0.', E_USER_DEPRECATED);
    
    +++ b/core/lib/Drupal/Core/Extension/ThemeInstaller.php
    @@ -97,6 +112,11 @@ public function __construct(ThemeHandlerInterface $theme_handler, ConfigFactoryI
    +      @trigger_error('The extension.list.module service must be passed to ' . __NAMESPACE__ . '\ThemeInstaller::__construct. It was added in Drupal 8.9.0 and will be required before Drupal 10.0.0.', E_USER_DEPRECATED);
    

    Nit: I think this is supposed to be drupal:8.9.0 and drupal:10.0.0, and ::construct should have a ()

  2. +++ b/core/lib/Drupal/Core/Extension/ModuleRequiredByThemesUninstallValidator.php
    @@ -0,0 +1,84 @@
    +      $theme_names = implode(", ", $themes_depending_on_module);
    

    Nit: only use " when needed, otherwise '

  3. +++ b/core/lib/Drupal/Core/Extension/ThemeInstaller.php
    @@ -106,6 +126,8 @@ public function install(array $theme_list, $install_dependencies = TRUE) {
         $extension_config = $this->configFactory->getEditable('core.extension');
    ...
    +    $installed_themes = $this->configFactory->get('core.extension')->get('theme') ?: [];
    +    $installed_modules = $this->configFactory->get('core.extension')->get('module') ?: [];
    

    I think this could reuse $extension_config instead of doing more gets to the config factory

  4. +++ b/core/lib/Drupal/Core/Extension/ThemeInstaller.php
    @@ -116,16 +138,33 @@ public function install(array $theme_list, $install_dependencies = TRUE) {
           foreach ($theme_list as $theme => $value) {
    ...
    +        $module_list = $this->moduleExtensionList->getList();
    

    Can retrieving the $module_list be moved outside this loop?

  5. +++ b/core/lib/Drupal/Core/Extension/ThemeInstaller.php
    @@ -116,16 +138,33 @@ public function install(array $theme_list, $install_dependencies = TRUE) {
    +        $module_dependencies = $theme_data[$theme]->module_dependencies;
    +        $theme_dependencies = array_diff_key($theme_data[$theme]->requires, $module_dependencies);
    +        $unmet_module_dependencies = array_diff_key($module_dependencies, $installed_modules);
    

    This took me a couple read-throughs to understand. The variable names are nice, but some extra comments might help.

  6. +++ b/core/modules/system/src/Controller/SystemController.php
    @@ -231,9 +250,42 @@ public function themesPage() {
    +          // @todo add logic for not displaying hidden modules in
    +          //   https://drupal.org/node/3117829
    
    +++ b/core/modules/system/src/Form/ModulesListForm.php
    @@ -326,38 +329,17 @@ protected function buildRow(array $modules, Extension $module, $distribution) {
    +      // @todo add logic for not displaying hidden modules in
    +      //   https://drupal.org/node/3117829
    

    Nit: First word should start with a capital, sentence should end with a period, and there cannot be a blank line after an inline comment.

  7. +++ b/core/modules/system/src/ModuleDependencyMessageTrait.php
    @@ -0,0 +1,53 @@
    + * @internal
    + */
    +trait ModuleDependencyMessageTrait {
    

    Missing longer form description of the internal-ness

  8. +++ b/core/modules/system/tests/src/Functional/Form/ModulesListFormWebTest.php
    @@ -81,4 +69,32 @@ public function testModulesListFormWithInvalidInfoFile() {
    +  protected function createUserAdministerModules() {
    +    $this->drupalLogin(
    +      $this->drupalCreateUser(
    +        ['administer modules', 'administer permissions']
    +      )
    +    );
    +
    +    $this->drupalGet('admin/modules');
    +    $this->assertSession()->statusCodeEquals(200);
    

    IMO this can be a one-liner (without all the weird line wraps or at least only wrapping the array part), and would really hope we don't need that assert for each of them...

  9. +++ b/core/modules/system/tests/src/Functional/Theme/ThemeUiTest.php
    @@ -0,0 +1,316 @@
    +  protected $testModules = [
    

    Missing docblock

  10. +++ b/core/modules/system/tests/src/Functional/Theme/ThemeUiTest.php
    @@ -0,0 +1,316 @@
    +    $incompatible = $theme_container->find('css', '.incompatible');
    +    $expected_incompatible_text = 'This theme requires the listed modules to operate correctly. They must first be enabled via the Extend page.';
    +    $this->assertSame($expected_incompatible_text, $incompatible->getText());
    

    Can use $this->assertSession()->elementTextContains()

  11. +++ b/core/modules/system/tests/src/Functional/UpdateSystem/UpdateScriptTest.php
    @@ -24,7 +24,13 @@ class UpdateScriptTest extends BrowserTestBase {
    -  public static $modules = ['update_script_test', 'dblog', 'language'];
    +  public static $modules = [
    

    While you're changing this, should be protected

  12. +++ b/core/tests/Drupal/KernelTests/Core/Theme/ThemeInstallerTest.php
    @@ -137,6 +139,79 @@ public function testInstallNameTooLong() {
    +    $this->assertEmpty(array_keys($themes));
    

    Why the array_keys?

  13. +++ b/core/tests/Drupal/KernelTests/Core/Theme/ThemeInstallerTest.php
    @@ -137,6 +139,79 @@ public function testInstallNameTooLong() {
    +    $this->assertFalse(isset($themes[$name]));
    ...
    +    $this->assertTrue(isset($themes[$name]));
    

    assertArrayNotHasKey
    assertArrayHasKey

  14. +++ b/core/tests/Drupal/Tests/Core/Extension/ModuleRequiredByThemesUninstallValidatorTest.php
    @@ -0,0 +1,161 @@
    +   * @var \Drupal\Core\Extension\ModuleRequiredByThemesUninstallValidator|\PHPUnit\Framework\MockObject\MockObject
    

    Nit: I don't think these |MockObjects are correct or needed when using prophecy

  15. +++ b/core/tests/Drupal/Tests/Core/Extension/ModuleRequiredByThemesUninstallValidatorTest.php
    @@ -0,0 +1,161 @@
    +if (!defined('DRUPAL_MINIMUM_PHP')) {
    +  define('DRUPAL_MINIMUM_PHP', '7.0.8');
    +}
    

    Let's make sure this is correct depending on which versions it is committed to.

jungle’s picture

Forgot to unassign myself, well, I will work on this next. Thanks, @tim.plunkett for your reviewing!

jungle’s picture

Partially done.

Addressed #295.1, 2, 3, 4, 6, 9, 10

  1. #295.8 IMO this can be a one-liner (without all the weird line wraps or at least only wrapping the array part), and would really hope we don't need that assert for each of them...

    Should refactory and move $this->createUserAdministerModules(); to setup()?

  2. #295.11. While you're changing this, should be protected

    I'd suggest filing a new issue to handle this.

  3. #295.12. $this->assertEmpty(array_keys($themes));

    I think $this->assertEmpty($themes); but not sure is there a special reason. and more than one match, skip it.

jungle’s picture

jungle’s picture

Assigned: jungle » Unassigned
tedbow’s picture

Assigned: Unassigned » tedbow

@jungle thanks for the patch!

Assigning to myself to continue working on it

Status: Needs review » Needs work

The last submitted patch, 297: 474684-297.patch, failed testing. View results

tedbow’s picture

Assigned: tedbow » Unassigned
Status: Needs work » Needs review
FileSize
8.96 KB
76.2 KB

re #295 @tim.plunkett Thanks for the review

re #297 @jungle thanks for taking care of most of these!

I had couple corrections(I think) to #297 and finished the rest of item from #295

  • 1. Fixing the colon be
  1. +++ b/core/lib/Drupal/Core/Extension/ModuleInstaller.php
    @@ -59,14 +66,21 @@ class ModuleInstaller implements ModuleInstallerInterface {
    +      @trigger_error('The extension.list.theme service must be passed to ' . __NAMESPACE__ . '\ModuleInstaller::__construct(). It was added in drupal 8.9.0 and will be required before drupal 10.0.0.', E_USER_DEPRECATED);
    

    I think we still the : drupal:8.9.0 as @tim.plunkett suggested

  2. +++ b/core/lib/Drupal/Core/Extension/ThemeInstaller.php
    @@ -97,6 +112,11 @@ public function __construct(ThemeHandlerInterface $theme_handler, ConfigFactoryI
    +      @trigger_error('The extension.list.module service must be passed to ' . __NAMESPACE__ . '\ThemeInstaller::__construct(). It was added in drupal 8.9.0 and will be required before drupal 10.0.0.', E_USER_DEPRECATED);
    

    Need :

  3. +++ b/core/lib/Drupal/Core/Extension/ThemeInstaller.php
    @@ -116,16 +138,33 @@ public function install(array $theme_list, $install_dependencies = TRUE) {
    +        $module_dependencies = $theme_data[$theme]->module_dependencies;
    +        $theme_dependencies = array_diff_key($theme_data[$theme]->requires, $module_dependencies);
    

    Adding comments asked for in #295.5

  4. +++ b/core/modules/system/src/ModuleDependencyMessageTrait.php
    @@ -0,0 +1,53 @@
    + * @internal
    + */
    +trait ModuleDependencyMessageTrait {
    

    #295.7 added internalness description.

    Resisted putting "because reasons" 😜

  5. +++ b/core/modules/system/tests/src/Functional/Form/ModulesListFormWebTest.php
    @@ -81,4 +69,27 @@ public function testModulesListFormWithInvalidInfoFile() {
    +    $this->assertSession()->statusCodeEquals(200);
    

    Agree we don't need this assert here. We don't do this after every page request and this test is not testing the permission. Removing.

    Re @jungle in #297.1

    Should refactory and move $this->createUserAdministerModules(); to setup()?

    We can't do that because the request for admin/modules needs to be made after the broken.yml.info file is created in testModulesListFormWithInvalidInfoFile(). The logic for creating that file should remain in the test method because it only is needed for that method.

  6. +++ b/core/modules/system/tests/src/Functional/Theme/ThemeUiTest.php
    @@ -0,0 +1,320 @@
    +    $this->assertSession()->elementContains('css', '.incompatible', $expected_incompatible_text);
    

    @jungle addressed this is #297. But there is small difference from @tim.plunkett's suggestion. It should be elementTextContains() not elementContains(). Fixed.

  7. #295.13 I didn't know about these. Fixed
  8. #275.14 I agree they are not needed. Removed
  9. #275.15 Good point. It seems correct for Drupal 8 https://www.drupal.org/docs/8/system-requirements/php-requirements.
    It looks like for Drupal 9 it will be 7.3 https://www.drupal.org/docs/9/how-drupal-9-is-made-and-what-is-included/... but I am not sure if that is final decision
tedbow’s picture

Test failed in #279 because of #275.10

We can't replace
$this->assertSame($expected_incompatible_text, $incompatible->getText());
with
$this->assertSession()->elementTextContains()

Because not an easy CSS selector we can use to get $incompatible

we have
theme_container = $this->getSession()->getPage()->find('css', "h3:contains(\"$theme_name\")")->getParent();
$incompatible = $theme_container->find('css', '.incompatible');

So the CSS that was used in #297 will find the first element matching on the page not the one under the particular element we already found. Since there are multiple themes on the page that are not compatible sometimes it will match the wrong 1. That is why only 1 test case failed for the function and not all.

Reverting

The last submitted patch, 302: 474684-301.patch, failed testing. View results

tim.plunkett’s picture

Status: Needs review » Reviewed & tested by the community

Ahhh #303 is a good point, I missed that incompatible was already traversing other elements.
@jungle and @tedbow addressed all by feedback!

This looks ready to me, aside from ensuring that #295.15 is handled appropriately on commit, or multiple patches are generated
Marking RTBC for now, but still tagged for FM review.

Martijn de Wit’s picture

Nice !

Are the Follow-ups from the issue summery still relevant?

  1. #3100374: Make it possible to install dependent modules when installing theme
  2. Update #3005229: Provide optional support for using composer.json for dependency metadata to read composer.json theme's also
tedbow’s picture

Here is reroll for 9.0.x

Rerolling made me think we should make 1 small change to our additions to \Drupal\Tests\system\Functional\Form\ModulesListFormWebTest

In \Drupal\Tests\system\Functional\Form\ModulesListFormWebTest::testModulesListFormWithInvalidInfoFile() now the method loops and has multiple invalid yaml files that it is testing.

So we can call \Drupal\Tests\system\Functional\Form\ModulesListFormWebTest::createUserAdministerModules() outside of the loop but inside of the loop we still have
$this->drupalGet('admin/modules');

So we have an extra drupalGet being performed that we don't need since createUserAdministerModules(). It also is not apparent from the method title that we do the drupalGet. I think we are forcing 2 things in the method that don't need to be there together.

In fact all the test methods in this class need a longed in user. Not all of them need to do 1 $this->drupalGet('admin/modules');

If we add $this->drupalLogin($this->drupalCreateUser(['administer modules', 'administer permissions'])); to setUp() then we can just replace the calls to createUserAdministerModules() with $this->drupalGet('admin/modules'); for the ones that need that at the beginning of the method.

Then can get rid of createUserAdministerModules(). I think this makes sense for 8.9.x also regardless of the problem in 8.9.x.

tedbow’s picture

Status: Reviewed & tested by the community » Needs review
FileSize
2.52 KB
76.19 KB
2.66 KB
76.17 KB
tim.plunkett’s picture

Status: Needs review » Reviewed & tested by the community

Oh wonderful, I am glad to see that helper method go away. And thanks for the 8 and 9 patches, both look good!

  • catch committed a130897 on 9.0.x
    Issue #474684 by bnjmnm, dawehner, tedbow, pfrenssen, JohnAlbin,...

  • catch committed 32eee42 on 8.9.x
    Issue #474684 by bnjmnm, dawehner, tedbow, pfrenssen, JohnAlbin,...
catch’s picture

Status: Reviewed & tested by the community » Fixed
Issue tags: -Needs framework manager review, -Needs change record updates

Committed a130897 and pushed to 9.0.x.

Also committed 15214db and pushed to 8.9.x. Thanks!

I updated the change record for the points I asked for in #474684-281: Allow themes to declare dependencies on modules although it could be improved. Untagging for framework manager review since I reviewed it and I am one.

saschaeggi’s picture

🎉 thanks all for your effort, great to see this finally being committed

doxigo’s picture

Damn, nice work you all, took some time but well done 👏

leymannx’s picture

Oh my Druplicon! 😱 I'm following this issue for probably 2 or 3 years now. And it was always a joy to have its updates popping up in my inbox and seeing everybody involved pushing things forward step by step. So many good patches, so many good reviews and feedback. I'm really impressed how things worked out in the end. A massive thank you everybody! 💙

tedbow’s picture

Issue tags: +Needs followup
  1. @tim.plunkett thanks for the review! @catch thanks for committing

    🎉 Everyone thanks for all the hard work getting this done!

  2. I was reviewing another issue and found a @todo reference to this in \Drupal\Core\Extension\ThemeInstaller::uninstall()
    // Base themes cannot be uninstalled if sub themes are installed, and if
          // they are not uninstalled at the same time.
          // @todo https://www.drupal.org/node/474684 and
          //   https://www.drupal.org/node/1297856 themes should leverage the module
          //   dependency system.
          if (!empty($list[$key]->sub_themes)) {
            foreach ($list[$key]->sub_themes as $sub_key => $sub_label) {
              if (isset($list[$sub_key]) && !in_array($sub_key, $theme_list, TRUE)) {
                throw new \InvalidArgumentException("The base theme $key cannot be uninstalled, because theme $sub_key depends on it.");
              }
            }
          }
    

    the other issue was marked as duplicate. Since modules can't depend on themes I don't we need to do anything. But we should make a follow up to remove this todo or refactor to share logic with module installer.

SKAUGHT’s picture

🎉

jungle’s picture

Glad that an 11 years old issue got landed.

Thanks all for your contributions and I just want to blame @tim.plunkett and @tedbow, without you two, this issue would live longer for sure :p

xjm’s picture

Now that we can actually do this, we'll want to update the change record for the jQuery UI removals to mention it.

xjm’s picture

Issue tags: +Drupal 9

oops

catch’s picture

Updated the change record, pointing to the change record for this one.

JohnAlbin’s picture

OH. MY. GOD.

Let's party like its 2009! Seriously, you all are amazing. <3

kclarkson’s picture

Wow. This is unreal. Congratulations and thank you to everyone who contributed!!

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.