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
- #3100374: Make it possible to install dependent modules when installing theme
- 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.
Comment | File | Size | Author |
---|---|---|---|
#308 | 474684-308-9.0.x.patch | 76.17 KB | tedbow |
#308 | interdiff-9.0.x-307-308.txt | 2.66 KB | tedbow |
#308 | 474684-308-8.9.x.patch | 76.19 KB | tedbow |
#308 | interdiff-303-308-8.9.x.txt | 2.52 KB | tedbow |
#307 | 474684-306-9.0.patch | 76.29 KB | tedbow |
Comments
Comment #1
philbar CreditAttribution: philbar commented+1
See: #340967: Remove Dependency on Extra Voting Forms (Test Alternative Voting Modules)
I'm in the process of moving the default theme to a theme project page. It will depend on Drigg and Extra Voting Form.
Comment #2
stephthegeek CreditAttribution: stephthegeek commented+1, this would be an exciting option potentially encouraging some interesting contrib themes for various site recipes/configurations
Comment #5
laura s CreditAttribution: laura s commented+1: the bee's knees.
Comment #7
Jeff Burnz CreditAttribution: Jeff Burnz commentedYes, 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?
Comment #8
sreynen CreditAttribution: sreynen commentedThis 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.
Comment #9
rickvug CreditAttribution: rickvug commentedMarked #647014: Allow themes to declare module dependencies a duplicate.
Comment #10
beefzilla CreditAttribution: beefzilla commentedYes, 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
Comment #11
dvessel CreditAttribution: dvessel commented@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?
Comment #12
RobLoachHaving themes depend on certain modules would be great. xCSS or SASS are good examples where themes can benefit from required module dependencies.
Comment #13
webchickMarking this as having a patch. I like this approach. dvessel's point about UX challenges though is not to be ignored.
Comment #14
sreynen CreditAttribution: sreynen commentedI'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?
Comment #15
dvessel CreditAttribution: dvessel commentedsreynen, 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.
Comment #17
KrisBulman CreditAttribution: KrisBulman commentedI 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.
Comment #18
Jeff Burnz CreditAttribution: Jeff Burnz commented@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.
Comment #19
sreynen CreditAttribution: sreynen commentedMerging 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?
Comment #20
Jeff Burnz CreditAttribution: Jeff Burnz commentedOK, I postponed #1167444: Not clear which themes are enabled and disabled, we can still work on our ideas over there (UI changes).
Comment #21
catchCrossposting #1067408: Themes do not have an installation status as well.
Comment #22
laura s CreditAttribution: laura s commentedRe 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.
Comment #23
Todd Nienkerk CreditAttribution: Todd Nienkerk commentedCrossposting #1209958: Allow themes to load stylesheets conditionally if a certain CSS file exists.
Comment #24
sunBetter title. Regardless of annoyance, this is a feature request.
I'd highly recommend to defer the implicit
base theme
asdependencies[]
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.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.
Comment #25
KrisBulman CreditAttribution: KrisBulman commentedModule dependencies aside, what about sub themes having base themes as dependencies? Should a separate issue be created?
Comment #26
sun@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.Comment #28
Jeff Burnz CreditAttribution: Jeff Burnz commentedPosted follow up issue: #1297856: Base themes should be required dependencies of subthemes
Comment #29
RobLoachAnother follow-up: #304486: Theme Engines as Modules.
Comment #30
RobLoachDoesn't take sun's notes from above into considering. Just wanted to throw something up there to get things rolling again....
Comment #31
Snugug CreditAttribution: Snugug commentedIs there any movement on this?
Comment #32
loominade CreditAttribution: loominade commented#1435406: allow themes to require modules has been marked as a duplicate
Comment #33
rcross CreditAttribution: rcross commentedCross 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:
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.
Comment #34
rcross CreditAttribution: rcross commentedthat aside, what else is needed to move this forward?
Comment #35
sreynen CreditAttribution: sreynen commentedLooks 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().
Comment #36
Snugug CreditAttribution: Snugug commentedDid a bunch of work on this at DrupalCon Munich core sprint. Still needs work, but on its way.
Comment #39
yannickooSnugug, you should remove the dsm right? And I would recommend to use dpm instead of dsm ;)
Comment #40
Snugug CreditAttribution: Snugug commentedLike 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.
Comment #41
thedavidmeister CreditAttribution: thedavidmeister commentedBeen 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:
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.
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:
Comment #42
anandkp CreditAttribution: anandkp commentedJust 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)
Comment #43
thedavidmeister CreditAttribution: thedavidmeister commented@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.
Comment #44
davidneedhamTotally. 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. :-)
Comment #45
thedavidmeister CreditAttribution: thedavidmeister commentedLooking 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.
Comment #46
sunMost of that is going to be resolved by #1067408: Themes do not have an installation status
Comment #47
thedavidmeister CreditAttribution: thedavidmeister commentedHmmm. 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.
Comment #48
thedavidmeister CreditAttribution: thedavidmeister commentedAh, 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 :)
Comment #49
thedavidmeister CreditAttribution: thedavidmeister commentedI 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 :/
Comment #50
KrisBulman CreditAttribution: KrisBulman commentedThe 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!
Comment #51
thedavidmeister CreditAttribution: thedavidmeister commentedYeah, 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.
Comment #52
thedavidmeister CreditAttribution: thedavidmeister commentedconsidering 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 :/
Comment #53
sunAt 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()
.Comment #54
thedavidmeister CreditAttribution: thedavidmeister commentedBut 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:
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.
Comment #54.0
thedavidmeister CreditAttribution: thedavidmeister commentedsolving http://core.drupalofficehours.org/task/902
Comment #55
sunAdding some related issues.
Comment #56
jhedstromBumping to 8.1.x.
Comment #57
davidwbarratt CreditAttribution: davidwbarratt as a volunteer and at Golf Channel commentedMoving to Needs Review and 8.0.x to kick off the test bot of the patch submitted in #47.
Comment #61
mgifford@davidwbarratt do you want to re-roll the patch or should this be bounced up to 8.1 again?
Comment #62
Fabianx CreditAttribution: Fabianx as a volunteer and at Tag1 Consulting commentedAs feature request this is definitely 8.1.x material.
Comment #63
markhalliwellI 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 :-/
Comment #64
dawehnerJust 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.
Comment #65
Miguel.kode CreditAttribution: Miguel.kode commentedThis is the re-roll for this patch.
Comment #67
dawehnerThere we go.
This uses the implicit assumption that you cannot have themes with the same name as modules, which IMHO is a valid restriction.
Comment #69
star-szr@dawehner thanks, can you please elaborate on the potential for circular dependencies, maybe give an example?
Comment #70
catchI'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.
Comment #71
dcrocks CreditAttribution: dcrocks as a volunteer commentedIs 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.
Comment #72
davidwbarratt CreditAttribution: davidwbarratt at Golf Channel commentedI 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.
Comment #73
markhalliwellRe: 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.
Comment #74
markhalliwellThe 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.
Comment #75
markhalliwellAlso, 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).
Comment #76
markhalliwellJust 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.
Comment #77
dawehnerSo this issue clearly is about adding module dependencies for themes, not the other way round.
Comment #78
markhalliwellYes, 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".
Comment #79
RainbowArrayOne 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.
Comment #80
catchYes, 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.
Not if we only add support for themes to depend on modules here.
Comment #81
andypostSummary 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 supportsComment #82
dawehner@markcarver
Oh sorry yeah, partially caused by a browser tab being open for a day or two.
/me sighs about the failure.
Comment #83
RainbowArrayIt 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.
Comment #84
hass CreditAttribution: hass commentedA good example is a theme that implements HTML5 stuff and requires https://www.drupal.org/project/elements module.
Comment #85
markhalliwellAssuming 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).
Comment #86
markhalliwellThe 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).
Comment #87
markhalliwellRe: @catch in #80:
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.
Comment #88
dawehnerI 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.
Comment #90
dawehnerLet's now also display the dependencies ...
Comment #92
SKAUGHT+1
Comment #93
JohnAlbinReal 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.
Comment #94
almaudoh CreditAttribution: almaudoh commentedThis looks like a typo:
...| |keys...
Comment #95
almaudoh CreditAttribution: almaudoh commentedFixed #94
Comment #96
almaudoh CreditAttribution: almaudoh commentedComment #98
dawehnerSo yeah IMHO we need to figure out the admin UI, as well as validating the dependencies during install time.
Comment #99
SKAUGHTto 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.
Comment #100
dawehner@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.
Comment #101
SKAUGHT(: 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.
Comment #102
SKAUGHT@JohnAlbin :: #93
LOL.. html inline in the info message. i've don't that myself in the past. yep, hacky.
Comment #103
SKAUGHTComment #104
JohnAlbinRe-rolled #95 by applying it to latset 8.1.x and then cherry-picking onto 8.2.x. No other changes.
Comment #106
jhedstromGoing 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.
Comment #107
jhedstromIndeed 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).
Comment #109
JohnAlbinFollowing 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.
Comment #111
Wim LeersI get the addition of
'dependencies' => array()
. This ensures the newdependencies
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 mapsdependencies
torequires
?(I do see that
base theme
is mapped todependencies
already in HEAD:$themes[$key]->info['dependencies'][] = $themes[$key]->info['base theme'];
.)I do suspect that this is why the patch is failing.
Missing description.
Nit: Typehint to
string
?Nit: s/from/of/
@return string|null
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.
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()
No longer necessary.
test_theme_depending_on_module
would be clearer I think.Comment #112
JohnAlbinFixed 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.
Comment #114
JohnAlbinFixing 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?
Comment #116
Fabianx CreditAttribution: Fabianx as a volunteer and at Tag1 Consulting commentedThis is the todo that still needs to be resolved.
Therefore setting to CNW.
Comment #118
keithm CreditAttribution: keithm commented@JohnAlbin #474684-114: Allow themes to declare dependencies on modules
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.
Comment #120
sunThe 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:
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.
Comment #121
Wim Leers@sun++
Comment #122
chr.fritschSorry for the noise, but welcome back in the issue queue @sun 😊
Comment #123
dbazuin CreditAttribution: dbazuin commentedSubscribing
Comment #124
camoa CreditAttribution: camoa at Acquia commentedI 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.
Comment #125
kclarkson CreditAttribution: kclarkson commentedhilarious 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 :)
Comment #126
dawehnerI've expanded the test coverage with a case of a theme with missing dependencies.
Comment #127
dawehnerI actually forgot to add the new test in the latest patch.
Comment #128
markhalliwellWhat's needed to get this in? Bootstrap really, really, needs this.
Comment #130
dawehner@markcarver
I pinged @joelpittet for a review on this issue, but yeah that's basically it I guess :)
Comment #131
Wim LeersTrying to find any problems that a committer might point out, including nits.
"used to install module dependent by theme" is wrong.
What about "used to install modules depended on by themes"?
Nit: one blank line too many.
Missing trailing period in the last UI string.
Should use the messenger service per https://www.drupal.org/node/2774931.
Should fit on a single line per our cs.
This only supports module dependencies. The parameter name and description suggest it also supports other types of extensions.
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).Has this been addressed? If not, can we create an issue to link to?
This is not used anywhere AFAICT?
Comment #132
dawehnerThank you wim!
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!
Done.
Thank you for figuring this out!
Good point. I added a followup to address this.
It is totally used in the template ...
core/modules/system/templates/system-themes-page.html.twig
Comment #134
dawehneroops
Comment #136
dillix CreditAttribution: dillix commentedIs there a chance to get this committed into 8.5.0?
Comment #137
dpagini CreditAttribution: dpagini as a volunteer commentedAttempting to fix the last patch.
Comment #138
kevineinarsson CreditAttribution: kevineinarsson commentedThe 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.
Comment #139
dawehnerLooking at the remaining failures
Comment #140
kalpaitch CreditAttribution: kalpaitch as a volunteer and commentedThere's an assumption that any theme dependencies will be newly installed:
But the failing tests are down to the message above being dropped before the next page load.
Comment #141
kevineinarsson CreditAttribution: kevineinarsson as a volunteer commentedThe 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').
Comment #142
kalpaitch CreditAttribution: kalpaitch as a volunteer and commentedFor sure, seems to me the simplest option is to continue with the deprecated `drupal_set_message()` until this issue is resolved.
Comment #143
dawehnerNice research @kalpaitch!
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.
Comment #144
kalpaitch CreditAttribution: kalpaitch as a volunteer and commentedIssue 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
Comment #145
kalpaitch CreditAttribution: kalpaitch as a volunteer and commentedOopsie woopsie, let's just ignore that last one.
Comment #146
markhalliwellThis 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()->*
.Comment #147
dawehner@kalpaitch Nice research! It felt right to nerd snipe you with this issue.
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"
I'm curious, should we add a test to
\Drupal\Tests\Core\Extension\ThemeHandlerTest
for this addition?Comment #148
dawehnerThe 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.
Comment #149
markhalliwellLet's see what #148 comes back with.
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 allowingNULL
as a default value.While we certainly don't really risk any race conditions here since
ModuleInstaller
doesn't requireThemeInstaller
, 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.Comment #151
dawehnerIt 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?
Comment #152
markhalliwellI'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.
Comment #153
dawehnerWell, 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.Comment #154
PolI just tested the patch against 8.5.x and it's working pretty fine.
Comment #155
SKAUGHT--edit.
#154 Pol: do you have a sandbox of the "openEuropa Theme". a common tool for manual testing would be good.
Comment #156
dawehnerThe 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.Comment #157
markhalliwellHm. 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.Comment #158
dawehnerYeah, 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?
Comment #159
markhalliwellSure, I just want to get this in ASAP (and even 8.5.x if possible)
Comment #160
Pol@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
Comment #161
dawehner@markcarver
Sure, let's give it a try.
Comment #162
larowlanThanks for working on this, its really needed. Some observations
We could
continue
here and avoid the elseif, it could be just a standard if.we need short array syntax here
Given we inject the kernel, can we inject this too?
we inject the messenger but don't use it?
we're not really disabling a checkbox anymore
the previous if returned, so we can use just plain if here, no need for elseif
should we call this statically to make it clear there is no mutation of $this?
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?Might be easier to maintain in the long-run if we used the CssSelector component here? Xpath tends to scare people off
Comment #163
dillix CreditAttribution: dillix commented@larowlan 9. in #162 may not work on languages other then English, so I think we should avoid it.
Comment #164
dawehnerThank you for your review @larowlan!
Nice suggestion
All we really need to ensure is that the refresh logic works. Nice catch
Let's remove it for now, see https://www.drupal.org/project/drupal/issues/2940148
Fair
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?
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.
I don't think test strings for translations matter in tests :)
Comment #165
pfrenssenThis 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.
Comment #166
borisson_Coding standards: else should be on the next line.
This doesn't seem to match anymore.
I think there are words missing in this comment.
Needs docblock
Comment #167
pfrenssenAddressed remarks from @borisson_.
Comment #168
borisson_Thanks @pfrenssen! This looks good now, I can't find any other nits to pick and the patch has sufficient test-coverage.
Comment #169
fgmAny reason why this method uses this global accessor instead of the injected theme_handler service ?
Comment #170
dawehner@fgm
Sure, it is documented 3 lines above in the code:
This is the full context. We want to reload the service after the installation of the theme.
Comment #171
markhalliwellThese shouldn't be putting static markup into translations. It should really be turned into proper render arrays.
Comment #172
markhalliwellI'll take a quick stab at it.
Comment #173
markhalliwellBleh, 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.
Comment #174
dawehnerThank you @markcarver. I totally agree with you. The way how some of these admin pages are build, is less than ideal :)
Comment #175
fgm@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 ?
Comment #176
alexpottWe 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.
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.
Similar to the other comment.
Comment #177
markhalliwellI 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.
Comment #178
markhalliwellOk, 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.
Comment #179
markhalliwellSetting to CNR to trigger testing, will set it back to CNW to address #176.5 & 6.
Comment #180
ademarco CreditAttribution: ademarco at Nuvole commentedWe 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.
Comment #181
ademarco CreditAttribution: ademarco at Nuvole commentedAttached a patch that fixed the problem highlighted in #180.
Comment #182
ademarco CreditAttribution: ademarco at Nuvole commentedComment #184
ademarco CreditAttribution: ademarco at Nuvole commentedFix broken tests.
Comment #185
markhalliwellComment #186
vdacosta@voidtek.com CreditAttribution: vdacosta@voidtek.com as a volunteer commentedTest and improve the readability of the code.
Comment #187
markhalliwell#176.5 and #176.6 still need to be addressed.
Comment #189
pfrenssenPatch didn't apply any more on 8.6.0. Straight reroll.
We should have a look at the test failures.
Comment #190
jasonawantI'm experiencing an error when installing from configuration. I'm listing the dependencies as follows.
When installing from a single exported configuration directory, see change record Installing Drupal from configuration (only certain profiles), I see the following errors.
Digging into this, I see Drupal\Core\EventSubscriber\ConfigImportSubscriber::validateThemes() checks to see if a theme's theme dependencies have been met.
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.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.
Comment #191
markhalliwellI'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.
Comment #193
TLWatson+1. I am experiencing the same issues as in #190.
Comment #194
kamkejj CreditAttribution: kamkejj at Nerdery commented8.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
Comment #195
SKAUGHTComment #196
lauriiiI'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.
Comment #197
Wim LeersComment #198
effulgentsia CreditAttribution: effulgentsia at Acquia commentedTagging this with "Drupal 9" (meaning, Drupal 9 could highly benefit from this getting done in Drupal 8) due to #196.
Comment #200
edmilsonefsHello! 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?
Comment #201
DamienMcKenna@edmilsonefs: Thanks for stepping up to help! If you have time to deal with this, you should:
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).
Comment #202
edmilsonefsComment #203
shaalI 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.
Comment #204
bnjmnmApologies 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.
Comment #205
bnjmnmAddressing the test failures and deprecated code.
Still working on #176.5-6
Comment #206
bnjmnmThis ,
ThemeUiTest
which I assume was unintentionally removed from an earlier iteration of this patchThemeUiTest
to cover the functionality added hereTwo 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.
Comment #207
Wim Leers#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
This reads like a temporary comment that is not intended for commit? 😅
🤓 Missing
are
.🤔 Either we should inject this, or we should document why this can't be injected.
🤓 This should use
__NAMESPACE__
.🤓 s/should not be/are not/
(And I think that that change will make it fit on a single line too!)
👍 If
\Drupal\system\Form\ModulesUninstallConfirmForm
is allowed to be@internal
in HEAD, then I think this one can be too.👍 I was gonna say "why explicitly say machine name" — but then it became clear :)
🤔 Woah. I've never seen the request object being retrieved in a constructor. Do we do the same thing somewhere else?
😅 Ehm … how does this work exactly? 😀
🤔🤓 I think "set as the default" is better?
🤔🤓 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.
Comment #208
bnjmnmAll 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
. 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
The closest I could find was
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 fromextension.list.module
this works fine, but once I try to add something whereextension.list.module
gets info a listextension.list.theme
the recursion starts putting up a fuss -- the focal point happening atdrupal_get_filename()
. If there were a way to get dependency info from theme yml files without having to calldrupal_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.
Comment #209
Wim LeersAnother round of review. Still fairly superficial. I would need at least half a day to review this as thoroughly as needed for an RTBC.
🤔 AFAICT this is not being used?
🤯😨
👎 These should use
$this->t()
🤔 Is
Unicode::ucfirst()
truly appropriate here?😅 This looks like a debug leftover?
🤓 Missing a
\
beforeModulesUninstallForm
👍
🤓
to
can be moved to the preceding line.🤓
admin/appearance
→/admin/appearance
🤓 Übernit:
{inheritdoc}
.🤔
$user
is not used anywhere. Why not do$this->drupalLogin($this->drupalCreateUser(…
?🤓 Comments don't match what's being tested.
🤔 Why
stable
and notstark
?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.🤔 These seem bugfixes that could land independently?
🤓Extraneous whitespace.
🤔 Let's use
StateInterface::class
.🤓 Übernit: unnecessary whitespace change.
Comment #210
phenaproximaDidn't get all the way through, but...
Should the state service be injected?
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:
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.
Does this require (eugh) a BC layer?
🤔 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?
This deprecation notice has the wrong version of Drupal core. :)
Comment #211
alexpottIt'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.
Comment #212
alexpottmodule_install.uninstall_validator
tagged service. Seeas 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.
I hope this is not needed. The container shouldn't be stored on this object.
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.
If any of the modules are experimental we're going to need the experimental disclaimer.
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 looks like a sign of something not being quite right with the approach. Ideally system module should be becoming less required not more.
Comment #213
bnjmnmRe: #212.5
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:
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.Comment #214
bnjmnmThere's also the use case of Experimental Themes redirecting to their own confirmation form, so Experimental themes with module dependencies would require confirmation of:
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.
Comment #215
bnjmnmThis 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:
isset()
it will return false if either condition is untrue.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.Comment #216
alexpottWe 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.
The theme extension list knows about what's installed - see \Drupal\Core\Extension\ExtensionList::getAllInstalledInfo().
Comment #217
bnjmnmImplemented the recommendations in #216
Comment #218
bnjmnmCancelled the test runs in #217 -- it occurred to me the changes I made there also require updating tests.
Comment #219
bnjmnmUpdated with the additional steps that didn't make it into #217
Comment #220
rensingh99 CreditAttribution: rensingh99 at Red Crackle commentedHi,
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
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
Comment #221
tedbowGreat this issue is getting some traction. First pass at reviewing mainly test coverage.
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 theif ($uninstall_dependents) {
block further down the method.
$this->themeExtensionList->getAllInstalledInfo()
is called 2x in these 2 lines. We can just use$installed_themes
for the foreach loopThis file only has 1 blank line change. Maybe a left over?
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.
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.
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.
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.
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.
This service actually used anywhere in a test?
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.Comment #222
tedbowforgot I rerolled in order to review. Here is the reroll
Comment #223
tedbowComment #224
bnjmnmThis 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.
Comment #226
bnjmnmHad to change a deprecated method for tests to pass.
Comment #227
tedbow@bnjmnm thanks for the updates. Didn't have the bandwidth for full review but here is some more. Generally looking good though!
This comment seems outdated. There is no
$this->modulesThemesDependOn
any more.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
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.
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
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.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.
These 2 permissions aren't actually needed. They can removed from all instances in the test.
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.
Can simply use
$assert_session->pageTextContains($confirmation_message);
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');
We should be put keys in the dataprovider array so we get better errors messages.
Core more often uses method names that start with "assert" here. So like
assertUninstallableTheme
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.
Personal preference but we could just call
parent()
directly on$element[0]
Also here if
li
was just added to the selector above you won't need$requires
This file is left over. The services.yml was removed in your last patch
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.
Comment #228
bnjmnmRe: #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 be5. ✅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.
Comment #229
tedbowre #228
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.
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.The first
$this->moduleExtensionList = $module_extension_list;
should be removed.
$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 theblock. So we can probably just define it at the top of this block like
Unrelated change.
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.I think all the test themes need
version: VERSION
it seems other test themes have this.
Super-nit
Most test themes and modules seem to use sentence case
title
and have descriptionComment #230
bnjmnm#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.
Comment #232
bnjmnmFixed 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.Comment #233
tedbowSorry have mention before multiple info.yml should switch to title case
Same here
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.
Comment #234
bnjmnmAddresses #233 by title casing all the other info.yml files and refactoring the try catch checks for exceptions in
ThemeInstallerTest
Comment #236
bnjmnmThemeUiTest needed updating to reflect the title case changes in #234
Comment #237
tedbowGot 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
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
Comment #238
bnjmnm#237
Nope - had it like that for an earlier iteration that needed it, but it's not necessary anymore.
Comment #239
tedbowI 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.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 checkingSince this also handles incompatible core dependency should we change the wording to something like "missing modules or incompatible dependencies."
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.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 callinginstall()
andexpected_missing_dependencies
then we could test a theme withWe 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.Comment #240
bnjmnmTakes 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.Comment #242
tedbowThis be removed in 10.0.0 not 9.0.0
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
maybetheme with own module dependencies installed, not base theme's
instead of
test_theme_mixed_module_dependencies - test modules enabled
maybetheme 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.
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.
Comment #243
bnjmnm#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.
Comment #244
tedbowI couldn't find another case where we pass all the test case arguments in one
$data
argument. I found some test methods likeBut 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.
Comment #245
bnjmnmI 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 anassertCount($data)
at the beginning of the test to get the benefits of separate arguments. Any thoughts on that?Comment #246
bnjmnm@tedbow reminded me of a much easier way to structure the data provider. Here's that change.
Comment #247
tedbowThis very similar to the exception we throw in
\Drupal\Core\Extension\ModuleInstaller::install()
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.
$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.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.Comment #248
HongPong CreditAttribution: HongPong at kor group commentedWould just like to suggest that it may be a good idea to have 'recommended' modules as well as required.
Comment #249
tedbow@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
Comment #250
bnjmnmAddresses feedback in #247 and updated issue summary.
Comment #252
bnjmnmThe exception message changed as a result of addressing ##24.2, had to update ThemeInstallerTest to reflect that change.
Comment #253
tedbowWe 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.
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 tomodule_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 frommodule_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 thearray_key_diff
on$themes
since we already have it.This is probably a copy paste error. We don't need
$this->adminUser =
here. We never use property.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.
dependency
is differentIn 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()
iftype === '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.Comment #254
bnjmnmThis addresses #253
Comment #255
tedbowThis looks good for blank line before the last bracket.
We need test coverage for this in
\Drupal\Tests\Core\Extension\InfoParserUnitTest
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
Comment #256
bnjmnmOf course that needs unit tests! That and everything else from #255 is addressed here.
Comment #257
tedbowSomething 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.Comment #258
bnjmnmRe #257
Comment #259
tedbowJust a couple things and I think we are RTBC!
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.
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.
I think we should include the filename with path instead of human readable name like the other exceptions shown in this test class.
Comment #260
bnjmnmAddresses #259
Comment #261
alexpottWhilst 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.
I think this needs to match
Yep I've tested this manually and it doesn't look right. See attached screenshot.
But at the moment we don't support the project:module format just something like:
That's confusing and potential source of frustration and also problematic if you want to depend on a duplicate module from a project.
Comment #262
alexpottHere's the screenshot for #261.4
Comment #263
tedbowLooking 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 checkisCompatible()
. 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.
Comment #264
tedbowFor another issue I just realized that in
system_install()
in theupdate
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)
Comment #265
bnjmnmThis 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 inDrupal\Tests\system\Functional\UpdateSystem
to figure it out.Comment #266
tedbow@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 theupdate_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.
Comment #267
bnjmnmUpdated test coverage for
system_install()
per the suggestions in #266.Comment #268
tim.plunkettOne 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.
Untranslatable?
Missing documentation of the params, both on the test function and within the provider function.
See
\Drupal\Tests\update\Unit\ModuleVersionTest
, checktestGetVersionExtra()
andproviderVersionInfos()
Nit: missing space after `if`
Same as above with docs
Same as the other template
Comment #269
bnjmnmYikes thats a lotta @param documentation, but that alone demonstrates how much it needed to be added.
This covers the feedback in #268
Comment #270
lauriiiShould we use a placeholder for printing the dependencies?
Comment #271
xjmComment #272
bnjmnmPlaceholdered re #270
Comment #273
lauriiiI 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.
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. 🤔
There's some markup in the translatable string here too which should be moved somewhere else.
Comment #274
tedbowre #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?
Comment #275
markhalliwellI'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.
Comment #276
tedbowI 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 intemplate_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
Comment #277
lauriiiShouldn't this be
$modules[$dependency]->info['hidden']
? If it's so, we should ensure we have test coverage for this.Comment #278
bnjmnm#277.1
. 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.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.
Comment #279
bnjmnmI discussed #277 with @lauriii and @gábor-hojtsy and here's our current take:
#277.1
As long as this is made clear in the CR, this is acceptable.
#277.2
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.Comment #280
lauriiiThis addresses my concerns in #277.
Comment #281
catchJust 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.
Comment #282
tedbowThis 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.
Comment #283
tedbow@catch re #281
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".
Comment #284
catchI 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.
Comment #285
tedbowI 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
@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.
Comment #286
tedbowUpdated parameters names and code comments of
testThemeInstallWithModuleDependencies()
because of changes in #286Comment #287
alexpottI 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.
Comment #288
tedbowLooking at this file that @alexpott mentioned I notice the folder name doesn't match the info file. Fixed
Comment #289
saschaeggiGreat feature request. Waiting for this for a long time. Let's hope we can get this asap in core :)
Comment #290
jungleLooks great to me.
But sorry, maybe out of the scope here.
=== NULL
vsis_null
, which one is recommended? Is it necessary to open an issue to do a benchmark?See https://www.php.net/manual/en/function.is-null.php#117344
\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.
Comment #291
catch#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.
Comment #292
xjmYeah 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.Comment #293
jungleCreat follow-up issues
Comment #294
jungleComment #295
tim.plunkettThere'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!
Nit: I think this is supposed to be
drupal:8.9.0
anddrupal:10.0.0
, and::construct
should have a ()Nit: only use " when needed, otherwise '
I think this could reuse
$extension_config
instead of doing more gets to the config factoryCan retrieving the $module_list be moved outside this loop?
This took me a couple read-throughs to understand. The variable names are nice, but some extra comments might help.
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.
Missing longer form description of the internal-ness
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...
Missing docblock
Can use
$this->assertSession()->elementTextContains()
While you're changing this, should be
protected
Why the array_keys?
assertArrayNotHasKey
assertArrayHasKey
Nit: I don't think these |MockObjects are correct or needed when using prophecy
Let's make sure this is correct depending on which versions it is committed to.
Comment #296
jungleForgot to unassign myself, well, I will work on this next. Thanks, @tim.plunkett for your reviewing!
Comment #297
junglePartially done.
Addressed #295.1, 2, 3, 4, 6, 9, 10
Should refactory and move $this->createUserAdministerModules(); to setup()?
I'd suggest filing a new issue to handle this.
I think $this->assertEmpty($themes); but not sure is there a special reason. and more than one match, skip it.
Comment #298
jungleRelevant to #297.2 or #295.11
Comment #299
jungleComment #300
tedbow@jungle thanks for the patch!
Assigning to myself to continue working on it
Comment #302
tedbowre #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
I think we still the
:
drupal:8.9.0
as @tim.plunkett suggestedNeed :
Adding comments asked for in #295.5
#295.7 added internalness description.
Resisted putting "because reasons" 😜
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
We can't do that because the request for
admin/modules
needs to be made after the broken.yml.info file is created intestModulesListFormWithInvalidInfoFile()
. The logic for creating that file should remain in the test method because it only is needed for that method.@jungle addressed this is #297. But there is small difference from @tim.plunkett's suggestion. It should be
elementTextContains()
notelementContains()
. Fixed.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
Comment #303
tedbowTest 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
Comment #305
tim.plunkettAhhh #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.
Comment #306
Martijn de WitNice !
Are the Follow-ups from the issue summery still relevant?
Comment #307
tedbowHere 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']));
tosetUp()
then we can just replace the calls tocreateUserAdministerModules()
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.Comment #308
tedbowComment #309
tim.plunkettOh wonderful, I am glad to see that helper method go away. And thanks for the 8 and 9 patches, both look good!
Comment #312
catchCommitted 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.
Comment #313
saschaeggi🎉 thanks all for your effort, great to see this finally being committed
Comment #314
doxigo CreditAttribution: doxigo as a volunteer commentedDamn, nice work you all, took some time but well done 👏
Comment #315
leymannxOh 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! 💙
Comment #316
tedbow🎉 Everyone thanks for all the hard work getting this done!
\Drupal\Core\Extension\ThemeInstaller::uninstall()
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.
Comment #317
SKAUGHT🎉
Comment #318
jungleGlad 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
Comment #319
xjmNow that we can actually do this, we'll want to update the change record for the jQuery UI removals to mention it.
Comment #320
xjmoops
Comment #321
catchUpdated the change record, pointing to the change record for this one.
Comment #322
JohnAlbinOH. MY. GOD.
Let's party like its 2009! Seriously, you all are amazing. <3
Comment #323
kclarkson CreditAttribution: kclarkson commentedWow. This is unreal. Congratulations and thank you to everyone who contributed!!