Problem
- Theme settings/configuration/variables are never removed.
- All themes are loaded unconditionally, regardless of whether they are going to be used.
- Base themes are initialized and used even if they are disabled, and even if they are not installed at all.
- Themes can have configuration and can participate in hooks, but they are not able to maintain their data in any way.
- String translations of a theme are re-imported whenever a theme is re-enabled.
- The update system/manager is not able to determine whether a theme is actually used.
- ...
- Themes cannot be installed or uninstalled.
Goal
- Sanity.
Proposed solution
-
Treat themes like modules. A theme is either installed and enabled or it is not. An extension that is not enabled cannot be used.
Essential impact:
- Admin themes have to be enabled in order to be used.
#542828: Do not special case disabled admin theme - Base themes have to be enabled in order to be used by sub-themes.
#1663142: Require that the base theme be enabled to use a subtheme
- Admin themes have to be enabled in order to be used.
- No change: Themes can be enabled and disabled.
- Separate follow-up: Allow to uninstall themes.
Particular @todos to resolve/revert
Follow-up issues
- #2247355: Allow to initialize ActiveTheme without Extension object (see comment #223)
- (needs to be opened) change the signature of hook_system_info_alter() (see comment #222)
- *sub issues of* #2228093: Modernize theme initialization
- #2206347: Use event system in ModuleHandler
- #2244917: Centralize extension name length validation into ExtensionDiscovery
- #2016629: Refactor bootstrap to better utilize the kernel
- #2232605: Themes cannot be uninstalled
- #2234613: Add DrupalUnitTestBase helper methods to enable/disable themes
- #2208429: Extension System, Part III: ExtensionList, ModuleExtensionList and ProfileExtensionList
- #2206347: Use event system in ModuleHandler
Comment | File | Size | Author |
---|---|---|---|
#222 | interdiff.txt | 4.02 KB | sun |
#222 | theme.install.222.patch | 92.6 KB | sun |
#217 | 1067408.217.patch | 93.61 KB | alexpott |
#217 | 213-217-interdiff.txt | 1.66 KB | alexpott |
#213 | interdiff.txt | 3.77 KB | sun |
Comments
Comment #1
andypostCould you describe this, I think that theme can change while been disabled so better to re-import strings.
Comment #2
sun@andypost: Locale module imports translations, not strings. Translation files may only change if a theme was updated. But since we don't know whether a theme was updated or not, because we don't even know whether it is installed or not, this event cannot be acted upon. Furthermore, even translations for modules are not re-imported when a module is updated. Hence, we have a huge difference in logic with regard to string translation imports. But in the end, that's just one of many problems caused by themes not having any installation information.
Comment #3
catchSubscribing. This would help to make list_themes() a bit less of a headache.
Comment #4
David_Rothstein CreditAttribution: David_Rothstein commentedI believe there's a patch that may have gotten started on some of this at #1008390: Allow themes to be removed via the update manager (but needs work).
Comment #5
bfroehle CreditAttribution: bfroehle commentedAlso, it would help prevent uninstalled themes from breaking an entire installation, cf., #619542: Malformed theme .info files break menu_router generation.
Comment #6
idflood CreditAttribution: idflood commentedsub
Comment #7
nielsvm CreditAttribution: nielsvm commentedsubscribing
Comment #8
yoroy CreditAttribution: yoroy commentedIs #1008390: Allow themes to be removed via the update manager the place to start working or do we need to hash out a more specific action plan here first?
Comment #10
sunComment #11
andypostI think a saved piece of theme provided config could have meaning of "status configured"
in case no config stored in active store and no default settings means "disabled"
all this about we just need a (bool)theme_access_runtime($theme_name)
Comment #12
dwwFWIW, I totally support this. There are a lot of wonky special cases in the Update manager to handle themes, base themes, sub themes, etc, etc due to all of this. The update status/manager code will hopefully be a lot simpler and cleaner if this was done.
Thanks,
-Derek
Comment #13
sunI suspect that we'll run into installer challenges with this, but FWIW, on an already installed site, this works beautifully.
Comment #15
sunOh nice, the installer still works (also verified manually). Not exactly sure how, but I guess it brute-forces the hard-coded maintenance theme to appear.
Most of the test failures actually are identical to #542828: Do not special case disabled admin theme, so I'm postponing further work here until that is committed.
Comment #16
sun#13: framework.themes.13.patch queued for re-testing.
Comment #18
sunSeriously! Let's get rid of this crap. /cc @catch :)
Fixed new system_list() $type triggers fatal error, since cache contains old $type.
Fixed theme_menu_local_task() requires 'localized_options' despite optional.
Fixed menu local tasks depend on hard-coded 'count' based on menu router.
Revamped block_menu(), introduced router argument loader and title callback for themes.
Comment #20
sunAwesome progress!
Removed unnecessary calls to menu_router_rebuild().
Fixed Appearance page theme enable/disable/default operations.
Fixed menu link weight is not taken into account for local tasks and actions.
Fixed Block admin UI does not always show the expected tabs.
Fixed various tests assume disabled themes to exist.
One of the remaining major issues is the question of how we want/expect the base/sub theme functionality to work. To explain the problem space:
Right now, all themes always exist, regardless of whether they are installed or enabled. This means that a theme that depends on a base theme can simply be enabled, since the dependency "magically" exists already.
With a proper installation status, that is no longer the case. Essentially, there are three options:
1) Automatically enable the base theme(s) of a theme when enabling it.
2) Require the user to manually enable the base theme(s) first.
3) Implement some heavy+weird confirmation form process, kind of identical to the module dependency handling on the Modules page.
To get this patch in faster, I'd highly prefer option 2) plus a new major issue for figuring out the "ideal" theme dependency handling on the Appearance page (which also involves UX considerations and thus potentially UI changes).
Comment #22
sunHm. I'm not able to reproduce the ManageDisplayTest + SearchRankingTest test failures locally.
Regardless of that:
Fixed Drupal\taxonomy\Tests\ThemeTest does not enable all themes it wants to use.
Fixed MENU_DEFAULT_LOCAL_TASKs do not always appear as first tab by default.
Comment #24
sunRoot cause for the false test failures: #1770902: Theme of parent site executing test leaks into all tests
Comment #26
sunThere's a fully working fix for the unexpected test failures of this patch, which could use some attention:
#1770902: Theme of parent site executing test leaks into all tests
Comment #27
sun#22: framework.themes.22.patch queued for re-testing.
Comment #29
sunMerged in HEAD.
The only remaining test failures I'm expecting are in Theme\ThemeTest, which are caused by the still open question in #20 regarding how to handle possible base theme dependencies on the Appearance UI page when enabling a theme.
Comment #31
sunFixed theme is re-initialized too early. (see follow-up fix in #1770902-14: Theme of parent site executing test leaks into all tests ;))
Comment #33
sunAlright, the last remaining puzzle piece:
- Added dependency handling (for base themes) to theme_enable(), identical to module_enable().
Comment #34
sunSorry, stale/bogus variable name in system_rebuild_theme_data().
Comment #35
sunFinal adjustments:
Fixed fatal error due to missing SCHEMA_* constants in install.inc.
Clarified Block menu router path for default theme.
Fixed stale comment in BlockAdminThemeTest.
Comment #36
sunThis is a major/critical task, because the configuration system needs to be able to properly install/uninstall configuration of a theme.
I got plenty of positive feedback on the goals of this issue/patch through other channels already. However, zero technical reviews.
This patch is RTBC from my perspective.
To clarify what it does:
The ability to actually uninstall a theme is left for a separate follow-up task/issue - this patch is big enough already.
Comment #37
tim.plunkettAll of the changes before theme.inc seem unrelated, like the local tasks, and the s/module_enabled/module changes. What's the reasoning for that?
Comment #38
rbayliss CreditAttribution: rbayliss commentedThis is probably a good and necessary step, but it means that the block table and block_rebuild() get nastier, since we'll be rehashing and storing blocks for base themes too. Could this be a performance/memory concern?
Comment #39
sunre: #37:
1) The removal of disabled themes from list_themes() led to the problem that some functionality and also tests were still expecting the UI to magically work, so I was confronted with having to decide between injecting even more calls to menu_router_rebuild() or eliminating that dependency altogether. Injecting more didn't make sense, so I converted block_menu() to not have a dependency on list_themes() anymore and expand the tabs for each enabled theme dynamically instead. Doing so revealed that the local tasks do not respect/inherit the defined weight of router items, so the default tab appeared at an arbitrary position in the expanded tabs. This could be split into an own issue/patch, but I'm not sure whether that is necessary.
2) I had to decide between renaming system_list()'s 'theme' argument into 'theme_enabled' or doing the reverse, renaming 'module_enabled' into 'module'. The former didn't make sense, since the function no longer returns anything disabled with this patch, so I renamed 'module_enabled' to 'module'.
re: #38:
That's a good point. I do not see a way around that though. However, if Blocks/Layouts succeeds, then block_rehash() will cease to exist anyway, AFAIK.
Comment #40
sunAnything else I could clarify? :)
Comment #41
rbayliss CreditAttribution: rbayliss commentedSome more things I found:
- The block demo page stopped working after applying this patch (the router item's path is no longer admin/structure/block/demo/ . $theme).
- Unless I'm missing something, there's no process for updates of themes (hook_update_n). Are we assuming this isn't necessary because there is no "install" step?
Comment #42
sunFixed stale router path checked in block_page_build() for block region demo.
Right, this issue does not intend to introduce database schema support or update functions for themes. That requires a discussion of its own.
Comment #44
sunRandom test failures...
Comment #45
sunAny chance to move forward here?
Comment #46
rbayliss CreditAttribution: rbayliss commentedLooks RTBC to me, but this is a big enough change that I don't feel comfortable changing the status myself.
Comment #47
carwin CreditAttribution: carwin commentedsun's patch in #42 applied cleanly.
Steps to make an error appear:
Really nothing to report other than the fact that it works fine. I'd say it's ready to roll.
Comment #48
sunMerged in latest HEAD. No other changes.
Comment #49
catchWhat's all this got to do with installation status for themes?
Also why rename the key here?
This can just use IN() instead of OR now.
Any reason why this needs to be a private function?
Why would bogus '0', FALSE or NULL values be passed in here? And why fail silently without an error message when that happens?
Do we really want to let themes do this?
This is a lot of work just for a few tabs, why's it necessary? Is it just to avoid the foreach() creating router items?
Not really keen on the API change, I don't understand why we need to change local task theming here at all tbh.
Comment #50
sunHrm. I already clarified most of that in #39... (:(
Comment #51
sunUse SQL IN instead of OR in system_list().
Added clarification for why _theme_menu_title() is declared "private".
Removed stray debugging early-return for bogus $theme value in drupal_theme_access().
Removed last remaining call to list_themes() in hook_menu() implementations.
From #39:
The removal of disabled themes from list_themes() led to the problem that some functionality and also tests were still expecting the UI to magically work, so I was confronted with having to decide between injecting even more calls to menu_router_rebuild() or eliminating that dependency altogether. Injecting more didn't make sense, so I converted block_menu() to not have a dependency on list_themes() anymore and expand the tabs for each enabled theme dynamically instead. Doing so revealed that the local tasks do not respect/inherit the defined weight of router items, so the default tab appeared at an arbitrary position in the expanded tabs. This could be split into an own issue/patch, but I'm not sure whether that is necessary.
From #39:
I had to decide between renaming system_list()'s 'theme' argument into 'theme_enabled' or doing the reverse, renaming 'module_enabled' into 'module'. The former didn't make sense, since the function no longer returns anything disabled with this patch, so I renamed 'module_enabled' to 'module'.
Yes, I think we want. If themes can be properly installed and uninstalled, then why not give a theme a chance to be involved? I'm fairly sure that advanced themes like Omega and others would have a use-case for that.
The important change is that the removal of the foreach() in hook_menu() removes one of the most troublesome interdependencies between the router and the system list, as well as the current theme configuration of the site. The routes are always identical, regardless of the system list and theme configuration. As mentioned above, I had to decide between implanting even more calls to menu_router_rebuild() throughout the system, or properly decoupling the router from the list of themes. We can certainly investigate whether this could be simplified through an abstraction in a follow-up issue.
Comment #53
sunThe test failures are caused by #843114: DatabaseConnection::__construct() and DatabaseConnection_mysql::__construct() leaks $this (Too many connections) :-/
Comment #54
Jeff Burnz CreditAttribution: Jeff Burnz commentedYes, all us advanced theme developers want this - sure for small themes they do not, then again neither do a lot of modules but they have the recourse, themes have no recourse and must hack. So it's about parity, themes are just another type of extension and the differentiation between theme and module is pretty gray, they need the same tools. Certainly this is a feature I am requested to hack into my themes on a weekly basis.
Comment #55
sun#51: framework.themes.51.patch queued for re-testing.
Comment #57
sunMerged in HEAD.
#843114: DatabaseConnection::__construct() and DatabaseConnection_mysql::__construct() leaks $this (Too many connections) is still unresolved though, so I doubt this will come back green. (That said, I have no idea at all why this patch triggers/reveals that bug in the database system.)
Comment #59
sun#57: framework.themes.57.patch queued for re-testing.
Comment #61
sunMerged HEAD.
Fixed merge conflicts.
Current state is massively broken now due to the system list config conversion.
Comment #63
sunFixed drupal_install_system() annuls fixed module list too early.
Fixed _drupal_maintenance_theme() does not load required code in case of early Kernel errors.
Fixed DrupalKernel does not initialize maintenance theme.
Updated system_list() and system_rebuild_theme_data() for config conversion.
Comment #65
sunFixed drupal_load('theme', $key) is not possible; "loads" (prints) the .info file.
Fixed stale 'module_enabled' in newly introduced update.inc code.
Fixed theme info/data is not rebuilt upon system list reset.
Comment #67
Sylvain Lecoy CreditAttribution: Sylvain Lecoy commentedThat's make even more sense as a theme name cannot be the same as a module name, and vice-versa.
Comment #68
thedavidmeister CreditAttribution: thedavidmeister commentedsun, do you need a hand or are you just smashing this one out yourself?
Comment #69
sunMerged HEAD.
Comment #70.0
(not verified) CreditAttribution: commentedRevamped issue summary.
Comment #71
podaroktagging for 4 and 5 items from summary
Comment #72
sunMerged HEAD.
I will try to remove the menu.inc changes now — even though they are absolutely solid, and after removing them here, we definitely want to redo them for D8, but well, yeah...
Comment #73
sunComment #75
sunThe installation failure is caused by #1798732: Convert install_task, install_time and install_current_batch to use the state system
Revert "Issue #1798732 by Dean Reilly, westie, alexpott: Convert install_task() variable to use state system."
Comment #77
sunFWIW, I'm not able to reproduce the installation test failures. If you're able to reproduce them, please tell me how.
(Please don't bug testing infrastructure/testbot maintainers about this; it's guaranteed to be a logical bug in code.)
Comment #78
markhalliwellsun, I'm glad you've put a lot of work into this, it's much needed! Not sure if or how I can help, but I'm willing. These changes will make #445990: [META] Refactor color module much easier to implement!
Comment #79
sunWe need to figure out under which exact conditions the patch in #75 breaks the Drupal installer.
I.e., the testbot fails even before running any tests, as it performs a "manual" installation of Drupal core first, and the Drupal installer does not seem to work as expected in a (un)certain situation. IIRC, I tested all possible combinations already (with and without pre-existing settings.php), but wasn't able to reproduce the installation failure. We need to figure out under which condition the installer throws an error.
Comment #80
KrisBulman CreditAttribution: KrisBulman commentedSun, what can us patch testers do? Simply apply the patch and test installation under any installation circumstance we can think of?
Comment #81
markhalliwellI created a local test site and pulled down the latest HEAD, applied patch (mostly clean), started the install process and everything when fine until it "finished" the install process and reloaded the screen. Got the following:
Not sure if this is related at all, but did a quick search and found this issue: #1783692: Test the bundle manager to show the fail on update.php which leads to #1708692: Bundle services aren't available in the request that enables the module
Comment #82
sunAh, thanks! Installing the Standard profile definitely helps to see the same errors. ;)
So I dived into the installer some more and how it installs modules and boots the kernel, and ended up in the very same conclusions I had before already:
Merging my patch from #1798732-54: Convert install_task, install_time and install_current_batch to use the state system into this one here allows to successfully install the Standard profile.
Since it isn't possible to merge it cleanly, I'm attaching a patch that needs to be applied on top of the patch in #73. (EDIT: Sorry, NOT #75!)
Furthermore, aforementioned issue is blocked on its own by a testbot problem, #1835144: pifr_drupal.client.inc makes too many assumptions about Drupal installer internals, which in turn leads to a dependency chain of two issues for this here, so I'm temporarily changing the status to postponed.
Comment #83
sunUnfortunately, the dependency of this issue just started to get hi-jacked, and history/experience leaves me with sufficient confidence that whichever final solution over there will not be sufficient for this issue.
Due to that, this feature request barely has a chance to land for D8. And yes, this is a feature request, since Drupal was perfectly able to operate without this separation in the past, and it will continue to do so. Needless to say, this change would also cause an unknown amount of follow-up issues (since we never had to deal with an installation status in themes). Correcting the issue category accordingly.
Also demoting priority, so as to free up threshold counters in any case.
Comment #84
markhalliwellsun, I completely understand your frustration. Per #1798732-87: Convert install_task, install_time and install_current_batch to use the state system though it seems he was just trying to organize where the patches live in the issue queue (trivial, I think).
Work is about to free up for the holidays and I was planning on doing some major work on color. It'd be a shame to not have this issue included with 8, but understandable given the time constraints and too many "chefs in the kitchen".
I know I'd probably just be in the way as I really haven't been following a whole lot of initiatives. Despite the fact that I'm rather unfamiliar with the intensive core re-architecture... if you need a twenty billionth pair of eyes on something specific (logic/bug related) that's been giving you a headache, feel free to contact me.
Comment #85
thedavidmeister CreditAttribution: thedavidmeister commentedJust cross-linking #474684: Allow themes to declare dependencies on modules that would love to see themes have a "real" installation status.
Comment #86
sunIt is close to impossible to get this done a few hours before feature freeze.
If this patch does not pass tests, then this feature will have to wait for Drupal 9. I spent a lot of time and worked in-depth on this change proposal, and I consider it as too dangerous and risky to be introduced late in the release cycle (i.e., after feature freeze).
I am aware that various @todos have been added to the code base that are referencing this very issue, but it is possible to resolve all of those in different ways. Their existence is not sufficient to claim any kind of exception.
The time-frame for adding this functionality was long enough. Unfortunately, the effort got blocked by systematic/architectural problems elsewhere in the system. Shit happens.
Updated for new drupal_container().
Comment #88
sunDrupal 9.
Comment #89
tim.plunkettYou do realize that feature freeze isn't until February 18th, right?
Comment #90
sunNo, feature freeze is Dec 1st, 2012.
Comment #91
dwwYay, issue status pushing match!
Feature freeze is Dec 1st, but according to Dries the feature completion stage continues until February 18, 2013. This feature/issue certainly qualifies under this definition:
There's clearly been "substantial progress" here long before Dec 1st. This isn't an almost-working patch posted for the first time at 11:59pm on Nov 30th... you've been posting almost working patches here for weeks. ;)
Seriously though, I think this is *exactly* the kind of feature Dries had in mind when announcing the new feature freeze, so please don't unilaterally decide this has to be postponed until the next version just yet. That's Dries's job. ;)
Cheers,
-Derek
Comment #92
markhalliwellAgreed! I feel this issue definitely qualifies as described by Dries and is MUCH needed. Thank you sun for all your hard work on this, almost there!
Also reverting status back to "needs work" since it failed testing.
Comment #93
sunSpin-off: #1864066: Simplify hook_menu_local_tasks() and MENU_DEFAULT_LOCAL_TASK usage
Comment #94
BerdirI'm wondering if this could be one way to fix those nasty deadlock issues, see #1862222-5: Random deadlock issues in simpletest. Haven't yet reviewed this, though.
Comment #95
sun#1798732: Convert install_task, install_time and install_current_batch to use the state system is back green and needs review.
Comment #96
YesCT CreditAttribution: YesCT commented@catch suggested this might be a better way to fix #986888-57: Undefined index warning in "Available Updates List" with hidden=TRUE base themes
Comment #97
jibranas per #88
Comment #98
markhalliwellper #89, #90, #91, #92. I wouldn't change this... I'm pretty sure they're close. That is, unless sun thinks this was an accurate tag.
Comment #99
markhalliwellOh, unless it's past Feb 18... ugh, shame.
Comment #100
mgiffordIs this a feature or a bug?
It's gotten bigger as there are a lot of related issues, but maybe there can still be some minor improvements in D8 that help.
If this can't be done in D8, is it possible to write a module (or maybe a Drush script) to address this in the interim?
The launch of D9 is a long ways away and this is still going to cause problems for D7 sites.
Comment #101
thedavidmeister CreditAttribution: thedavidmeister commenteddamn. I was hoping this would make it >.<
Comment #102
effulgentsia CreditAttribution: effulgentsia commentedI think this issue is likely necessary as a consequence of D8 changes like the configuration system, so recategorizing as a task. @sun: Do you agree or do you still stand by your claim in #83 that this is a feature?
Comment #103
sunTo clarify:
This patch is based on the framework-themes-1067408-sun and I can re-roll/merge it anytime, but without that second dependency, it will not work.
Comment #104
gddI'm happy leaving this postponed on #1798732: Convert install_task, install_time and install_current_batch to use the state system, but I do think this is extremely important to get in. I'm not sure how we're going to support deploying themes between sites without it.
Comment #105
jibranRelated #1167444: Not clear which themes are enabled and disabled
Comment #106
catchIf this blocks CMI it should probably be critical.
Comment #107
markhalliwellref: #2002606: Allow themes to provide services.yml
Comment #108
catchComment #109
Jeff Burnz CreditAttribution: Jeff Burnz commentedFYI autoloading does not work for classes in base themes if they are not enabled, we need to be able to force the base theme to be enabled otherwise no base theme can reliably use autoloading without risking fatal errors if the sub-theme gets enabled without it (without first enabling the base theme). I'm not sure if that is still part of this issue (setting dependancies) but it really needs to be.
Comment #109.0
Jeff Burnz CreditAttribution: Jeff Burnz commentedUpdated issue summary.
Comment #110
dawehnerComment #111
jibranAs @sun said in #103
Can you post WIP patch so that we can move forward.
Comment #112
xjmHmm. If we still intend to do this, it should probably be beta-blocking.
Comment #113
effulgentsia CreditAttribution: effulgentsia commentedSo long as this issue is critical, then I agree with #112, so tagging for now. However, I'm not that clear at this point as to whether this is really critical or not. Also, the issue summary still distinguishes between installed and enabled, but is that still relevant after #1199946: Disabled modules are broken beyond repair so the "disable" functionality needs to be removed?
Comment #114
klonos@effulgentsia: AFAIK themes do not tamper with site data (they don't access the actual db) so I don't think that #1199946 applies to them. The main reason behind #1199946 was that by allowing people to enable/disable modules at will could possibly break their sites in unrecoverable ways. OTOH the worse that could happen by enabling a theme is to *visually* break the site - something that is relatively easily recoverable.
Correct me if I'm wrong.
Comment #115
Gábor Hojtsy@klonos, @effulgentsia: themes do have settings and therefore subject to the config installation / uninstallation process, which is the main reason to do this issue AFAIS.
Comment #116
klonosYes, I understand that. My main point back in #114 was that neither theme themselves nor their settings have the potential to break a site in *unrecoverable* ways (data loss for example) like modules. So, yes this issue here is still valid, but #1199946: Disabled modules are broken beyond repair so the "disable" functionality needs to be removed does not make sense for themes (other than perhaps in order to be in tandem if one considers both themes and modules extensions).
Comment #117
dawehnerThis is a first approach for a rerole, which tries to reduce the amount of changes to make this patch somehow manageable.
Comment #119
neetu morwani CreditAttribution: neetu morwani commentedPatch applied in comment#117 was not being applied successfully. Patch Rerolled so that the patch applies successfully and then functionality of the issue can be worked upon.
Comment #120
neetu morwani CreditAttribution: neetu morwani commentedComment #122
neetu morwani CreditAttribution: neetu morwani commentedPatch rerolled again.
Comment #125
sunSince the entirety of the extension system is a big sad mess of spaghetti code right now, I'm working very actively to clean up, modernize, and speed up all components in a bottom-up approach in #2186491: [meta] D8 Extension System: Discovery/Listing/Info
The currently active step is #2188661: Extension System, Part II: ExtensionDiscovery, which already removes a bunch of interdependencies between modules and themes, which is something this patch/issue currently has to take into account, but shouldn't have to.
My plan is to come back to this issue and get it done as soon as the underlying layers are sufficiently prepared to properly manage and handle [theme] extensions, which I expect to be the case very soon. That is, because this patch here only adds more spaghetti on top of the already existing spaghetti, which makes the whole thing even more complex than it is already. Instead, we will be able to implement the necessary changes here in a clean way, as soon as the base extension system no longer is a pile of circular dependencies and infinite recursions.
Comment #126
saki007sterComment #127
cosmicdreams CreditAttribution: cosmicdreams commentedParts I and II are now in core. Is that enough to move forward on this issue?
#2188661: Extension System, Part II: ExtensionDiscovery
Comment #128
sunVery naive re-roll of #117. A lot of required changes seem to have gone lost in previous re-rolls, so this patch is expected to fail.
Comment #130
sunComment #131
sunPostponing on #2187717: Remove code duplication in system_list() and ThemeHandler::rebuildThemeData()
Comment #132
sunComment #133
catchUntagging as a beta blocker. We can't do head to head updates until this is in, but that's not going to be tied to the first beta, and the rest of the changes here aren't going to break contrib modules that are porting so I'd happily put it in with beta already out.
Comment #134
sunThat one has landed, next one in line is #2210197: Remove public access to Extension::$type, ::$name, ::$filename, which touches lots of related code, so plenty of conflicts.
Comment #135
alexpottDiscussed this with @catch - retagging as a beta blocker since this would required a head to head update and exactly how the upgrade path might work is tricky since one of the problems this is trying to solve is that base themes do not have to be installed in order for their code to be running.
Comment #136
sunWaiting for #340723: Make modules and installation profiles only require .info.yml files to land, which might appear unrelated, but isn't.
Comment #137
BerdirThat went in, so this should (finally) be unblocked? :)
Comment #138
Désiré CreditAttribution: Désiré commentedI have just rerolled the patch from #128.
Comment #140
Désiré CreditAttribution: Désiré commented138: theme.install.138.patch queued for re-testing.
Comment #142
mgiffordIt applies fine locally...
Comment #143
tstoeckler138: theme.install.138.patch queued for re-testing.
Comment #145
Désiré CreditAttribution: Désiré commentedThe first reroll in #138 wasn't correct, here I uploaded the correct one.
This will also fail at the same point.
What I found yet, is that \Drupal\Core\Extension\ThemeHandler::rebuildThemeData() will return an empty array on theme enable. Here:
Comment #146
Désiré CreditAttribution: Désiré commentedComment #148
Désiré CreditAttribution: Désiré commentedComment #149
sunNote: Attached patch is totally WIP and not functional yet. Only posting it because @dawehner asked in IRC.
Status quo:
This pretty much means that #1770902: Theme of parent site executing test leaks into all tests was not actually magically fixed by some other issue, so I've re-opened that issue.
Comment #151
sunWowza! Writing that DUTB test was the best idea ever. TDD++ Tons of fixes:
The new test passes, the interactive installer works correctly, etc. Curious what the testbot thinks.
Expected failures: Some code won't be happy with the
global $theme = NULL
fallback indrupal_theme_initialize()
, also covered in a corresponding code comment.Some of those test failures are caused by the testing framework itself, because it doesn't properly reset all theme related global variables currently. Those (false) test environment failures have to be addressed in #1770902: Theme of parent site executing test leaks into all tests
Comment #153
sunJust to be clear: I'm just "hacking forward" here. A lot of the contained changes will have to be split into separate issues (with proper tests). For now, I'm just trying to figure out which architectural problems are actually ahead of us and need to be addressed — or in other words, trying to see the big picture of changes required to make this happen.
Comment #155
sunFixed bogus 'theme_default' property in Standard profile.
Comment #156
sunCreated quick-fix spin-offs with patches:
#1770902: Theme of parent site executing test leaks into all tests
#2227355: InstallerServiceProvider does not override all cache bins to use memory backend
#2227363: system.theme.disabled config schema defines wrong data type
Comment #158
sunComment #159
markhalliwell155: theme.install.155.patch queued for re-testing.
Comment #161
sunThe quickfixes landed.
Comment #163
sunTwo issues that will help us to move forward here:
#2228921: Consolidate system.module + system.theme + system.theme.disabled into core.extension config
Resolves a critical @todo in this patch (cf.
install_profile_themes()
in install.core.inc):Installation profiles should only specify the list of themes to install in their
.info.yml
files (in the same way they specify a list of modules to install). The.info.yml
file should not contain a'theme_default'
property to configure the default theme.The configuration for the default theme + admin theme is regular configuration. Installation profiles should supply that config like any other configuration, by shipping with a
system.theme.yml
file.But that's not possible right now, because the default configuration of an installation profile overrides all other configuration. → Overriding the
system.module
+system.theme
configuration causes a level of inception that isn't fun anymore.Moving the list of installed/enabled extensions into core.extension doesn't fix the potential inception, but it does enable installation profiles to ship with a
system.theme
config file to configure the default + admin themes.#2218655: Core expects $theme.settings default config file to exist
Comment #164
sunDidn't look into the remaining test failures yet, but merged 8.x and:
Comment #166
sunThis patch implants a new "core" (dummy/null) theme, so as to make DUTB tests run against the raw/original/default theme output of core and modules by default. In other words, no theme extension is installed, but yet, Drupal is still able to generate themed output.
@see
drupal_theme_initialize()
In addition, #2218039: Render the maintenance/install page like any other HTML page will help to resolve the
RenderElementTypesTest
failures, because that issue removes the offending test method entirely. :PComment #168
jessebeach CreditAttribution: jessebeach commentedDrupal\system\Tests\Common\RenderElementTypesTest
fails are resolved by #2218039: Render the maintenance/install page like any other HTML page.Drupal\config\Tests\DefaultConfigTest
,Drupal\system\Tests\Extension\ThemeInstallTest
andDrupal\system\Tests\Theme\TwigSettingsTest
remained unaffected with the same number of fails.Comment #169
jessebeach CreditAttribution: jessebeach commentedThere's a @todo in
bartik.info.yml
that should be addressed in this issue:Comment #170
sunWell-spotted! :-)
Yeah, the current patch deletes some of those related @todos already — fact is, they are completely unrelated to this issue and the generic aspect of themes having an installation status.
As partially (perhaps scope-creep-ely) attacked by #2218655: Core expects $theme.settings default config file to exist, the complete theme settings configuration situation is totally flawed right now — (1) there's no config schema for the (arbitrary) settings being added by modules (in addition to the default/global theme settings), and (2) in case there are any module settings, those are not maintained in case e.g. the originating module is uninstalled; leaving stale settings behind (forever).
So that's a completely different problem space, which has little to do with introducing an installation status for themes. Much rather, it's a problem concerning the interaction between modules and themes within the context of configuation (theme settings), whereas both the list of installed modules as well as the list of installed themes can change at any time.
→ We need a separate issue for that. (I wasn't able to think of an initial solution proposal, so I never created one.)
Comment #171
jessebeach CreditAttribution: jessebeach commentedI've fixed the
TwigSettingsTest
failures and removed the setting frombartik.install
that I noted in #169.This sounds very similar to modules trying to inject settings into config entities, like translations sync does.
#2224761: Add a generic way to add third party configuration on configuration entities and implement for field configuration
Comment #172
jessebeach CreditAttribution: jessebeach commentedComment #174
jessebeach CreditAttribution: jessebeach commentedFollow-up for the settings cleanup on uninstall issue mentioned by sun in #170. #2232605: Themes cannot be uninstalled.
Because #2232605 depends on #2218655: Core expects $theme.settings default config file to exist, it also gets elevated to blocker status.
Comment #175
jessebeach CreditAttribution: jessebeach commentedThe Change Record just needs a review.
https://drupal.org/node/2232651
Comment #176
sunApparently, one of my earlier test failure fixes completely destroyed the whole point of this issue ;-) So...
TwigSettingsTest
fix from #171. (Leaving theme settings change to #2218655: Core expects $theme.settings default config file to exist)InfoAlterTest
+ThemeTest
.ThemeHandler
cannot be injected intoModuleHandler
. → #2206347: Use event system in ModuleHandlersystem_rebuild_theme_data()
.State
service intoThemeHandler
.rebuildThemeData()
method instead ofsystem_rebuild_theme_data()
inThemeHandler::enable()
.UpdateServiceProvider
's theme_handler service definition.Curious where that leaves us... My only hope is that it doesn't melt down entirely. ;-)
Comment #178
sunhah, chasing HEAD is a different kind of meltdown. ;)
Comment #180
sunComment #182
sunThe only remaining 2 test failures should be in RenderElementTypesTest → #2218039: Render the maintenance/install page like any other HTML page
Comment #183
dawehnerUrgs ... i know this is the same code as before but this assumes internals of the theme handler. What about a follow up to be able to retrieve this data from the theme handler?
Why can't we write a theme negotiator which provides the fallback?
THis is a clear sign where we can inject it.
Comment #185
dawehnerTried to convert some of the tests and yeah the theme handler does too much to be properly unit tested on large scale at the moment.
Comment #186
sunChecking to see whether the interdiff from #185 is safe to apply to my sandbox branch.
Comment #188
sunMerged 8.x.
Comment #190
jibran188: theme.install.188.patch queued for re-testing.
Comment #192
Jalandhar CreditAttribution: Jalandhar commentedUpdating the patch with reroll. Please review.
Comment #194
sunComment #195
sunI rewrote the change notice from scratch, in order to properly explain the actual (immense) consequences of this change, separated by audience:
https://drupal.org/node/2232651
Comment #196
Gábor Hojtsy@sun: the change notice looks good, does not talk about translations anymore though?!
Comment #197
sun@Gábor Hojtsy: It does mention string translations (the old did not). Grep for "translation"? — That said, we can happily improve it further; my main objective was to replace the previous text that talked about uninteresting internals with a proper explanation of what this change means for all affected parties.
Comment #198
Gábor HojtsyIndeed, looks good! Thanks!
Comment #199
sunMerged 8.x + resolved complex merge conflicts. Hope it will still pass.
FWIW, I was skeptical about the new
ThemeHandler::setDefault()
method originally, because it appears to be just a wrapper around configuration right now and thus appears to be out of scope forThemeHandler
's concerns — however, after fully thinking through it, changing the default theme might actually require additional, system-wide operations (for example, the order of tabs might need to change, etc)Therefore, we should have an explicit API method for changing the default theme, and we should even trigger an event and/or invoke a hook in there (in a separate follow-up issue). Same applies to the admin theme (consumed in slightly weird ways by e.g. quickedit module), for which no method is added here yet; equally a follow-up.
Comment #201
sunMoved default config files.
Comment #202
dawehnerDo we also want to add themes which are not enabled here? I would assume that the code is doing that.
Do we really need to keep reset and refreshInfo separate?
Comment #203
sun#2218039: Render the maintenance/install page like any other HTML page has landed (again), so this needed a merge.
@dawehner:
ThemeHandler::addTheme()
works (sorta) identically toModuleHandler::addModule()
. They are adding extensions at runtime, and are meant for the super-special edge-case environments only (install.php & Co). My hope is that we can still clean that up for D8, once #2016629: Refactor bootstrap to better utilize the kernel has landed (by introducing separate aInstallerKernel
& Co).refreshInfo()
method without proper events. That's going to be obsolete and can be refactored, once #2206347: Use event system in ModuleHandler has landed.Comment #204
dawehnerOkay, cool
Comment #205
alexpottIs this change necessary - and if it is should we be using the rebuildThemeData method on the themehandler?
Where is a test for disabling a base theme before a sub theme?
This is now unnecessary and so is enabling the bartik theme at the beginning of this test.
Why are we skipping these tests? If we are going to skip these tests and comeback at a later date then fine but this needs a @todo but I really don't think we should be removing them.
Comment #206
alexpottForgot to say - fantastic work!
Making profiles declare their themes huge +1 the programatic enabling in hook_install was always ugly.
Comment #207
sunComment #209
sunFixed MenuRouterTest.
Comment #210
alexpottIs this tested anyway else? The other removed tests certainly are but I'm not sure about this one.
Comment #211
jessebeach CreditAttribution: jessebeach commentedIt's worth mentioning in a comment that 'core/core.info.yml' doesn't exist. In fact, would it not work just as well to pass an empty string as the second argument?
These are missing the @return line in the docblock.
We already have
\Drupal\Tests\Core\Extension\ThemeHandlerTest
. Maybe combine them?The type for this param should be \Drupal\Core\Extension\Extension|string
Comment #212
jessebeach CreditAttribution: jessebeach commentedOh! Forgot to mention. The updates to the Change Notice are great. That's good to go.
Comment #213
sun@alexpott: #2244917: Centralize extension name length validation into ExtensionDiscovery
Comment #214
jessebeach CreditAttribution: jessebeach commentedThose are my concerns addressed. Thanks sun!
Comment #215
alexpott@sun: but we're removing test coverage and then saying it's going to be replaced in an issue that it not in yet. Seems wrong.
Comment #216
sunWell, let's figure that out first then. It's equally wrong to add back a test for that to
ThemeHandlerTest
, because it's none of its business in the first place, and allowing too long extension names to enter the system in any way can have wide-ranging consequences that aren't accounted for at all yet.→ #2244917: Centralize extension name length validation into ExtensionDiscovery
Comment #217
alexpott@sun well that's the current functionality - fine if we're going to refactor it in #2244917: Centralize extension name length validation into ExtensionDiscovery but that's not a beta-blocker or as high priority. So let's add a test back and hopefully get refactor it over there.
Comment #218
xjmYeah, converting this to a DUTB for the time being seems the way to go to get this in, since it's so close to ready.
Comment #219
xjmIs there an issue for this?
Comment #220
jessebeach CreditAttribution: jessebeach commentedNo, and there are 6 more @todos in the file without issues. I don't understand from the comment why the status property should be removed so I can't log an issue for it. I would just as soon rip out the @todo since it's not actionable, but then again, sun has a good understanding of the plan for improving the theme layer and I don't want to pull out commentary that he's left as a sign post. I would encourage that he enter issues for these todos with explanations and then update the comments so others can help him with these tasks.
My opinion here is we have the functionality and the tests. I'm loath to see us fritter away more hours focused on comments that can be updated at a later date and that aren't holding us back in any acute away.
Comment #221
xjmAgreed. Thanks @jessebeach!
Comment #222
sunSure, I can live with that; just a minor adjustment:
Merged ThemeTestLongName(+Test) into ThemeHandlerTest.
Adjusting issue title + category to give a better sense for why we have to do this.
This patch removes a whole bunch of @todos.
Pretty much all of the @todos that are introduced here will be addressed by:
#2228093: Modernize theme initialization
#2206347: Use event system in ModuleHandler
The only one that may not is the one about changing the signature of
hook_system_info_alter()
, but that should not hold up this patch.Comment #223
catchInterdiff looks good.
I started reviewing the patch but didn't get all the way though. No real complaints as yet. Main thing I'm not sure about is the core.info.yml I think I agree with xjm that an empty string might be clearer. But also we could open a follow-up to figure that out.
Shouldn't stop someone else committing if they get here before me.
Comment #224
YesCT CreditAttribution: YesCT commentedI read through the comments here, the sidebar of issue relation meta data, and the @todo's in the patch which reference issue numbers.
adding to the issue summary:
Follow-up issues
might need clarification on which of these are nice-to-haves, and which are needed follow-ups because of this issue, and which are already existing.
Comment #225
YesCT CreditAttribution: YesCT commentedupdated summary with a link to the newly created (postponed) child #2247355: Allow to initialize ActiveTheme without Extension object
Comment #226
webchickAll right, did a walk through of this patch w/ Jesse Beach, Alex Bronstein, et al. I didn't find anything apart from the weird core.info.yml thing that catch spotted as well in #222, but YesCT already identified that as one of the follow-ups. (empty string sounds very intuitive to me as well).
Otherwise, this seems like it's getting rid of some assumptions in the extension system, so that's good stuff. The change record looks awesome.
Committed and pushed to 8.x. Thanks!
Comment #229
chx CreditAttribution: chx commentedI have no idea why the _install hook was dropped after #119 and there's no explanation either so it seems rather like an oversight so I filed #2652542: Add .install file abilities for Themes