Themes do not have an installation status

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

I suspect that we'll run into installer challenges with this, but FWIW, on an already installed site, this works beautifully.

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

#13: framework.themes.13.patch queued for re-testing.

Seriously! 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.

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

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

Root cause for the false test failures: #1770902: Theme of parent site executing test leaks into all tests

There'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

#22: framework.themes.22.patch queued for re-testing.

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

Fixed theme is re-initialized too early. (see follow-up fix in #1770902-14: Theme of parent site executing test leaks into all tests ;))

Alright, the last remaining puzzle piece:

- Added dependency handling (for base themes) to theme_enable(), identical to module_enable().

Sorry, stale/bogus variable name in system_rebuild_theme_data().

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

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

1. Only enabled themes are loaded. Only enabled themes can be used.
2. Base themes are treated like module dependencies, they need to be enabled first.
3. The Block module administration UI is adjusted to only care about enabled themes. This allowed to heavily simplify the menu router definitions, and to remove the dependency on the actual list of themes from block_menu(). Tabs for each theme are expanded through hook_menu_local_tasks_alter() instead.
4. theme_enable()/theme_disable() are almost identical to module_enable()/module_disable() now. Fully merging them into a single API is perfectly possible now and should be investigated in a follow-up issue.
5. The 'module_enabled' argument of system_list() & co is simplified to just 'module', since 'theme' also returns only enabled now.

The ability to actually uninstall a theme is left for a separate follow-up task/issue - this patch is big enough already.

re: #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.

Anything else I could clarify? :)

Fixed stale router path checked in block_page_build() for block region demo.

there's no process for updates of themes (hook_update_n). Are we assuming this isn't necessary because there is no "install" step?

Right, this issue does not intend to introduce database schema support or update functions for themes. That requires a discussion of its own.

Random test failures...

Any chance to move forward here?

Merged in latest HEAD. No other changes.

Hrm. I already clarified most of that in #39... (:(

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

What's all this [menu/tasks stuff] got to do with installation status for themes?

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.

why rename the key here?

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

Do we really want to let themes do this [hook_install()]?

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.

This [block_menu_local_tasks_alter()] is a lot of work just for a few tabs, why's it necessary? Is it just to avoid the foreach() creating router items?

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.

The test failures are caused by #843114: DatabaseConnection::__construct() and DatabaseConnection_mysql::__construct() leaks $this (Too many connections) :-/

#51: framework.themes.51.patch queued for re-testing.

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

#57: framework.themes.57.patch queued for re-testing.

Merged HEAD.
Fixed merge conflicts.

Current state is massively broken now due to the system list config conversion.

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

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

Merged HEAD.

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

1. Removed menu local tasks fixes/improvements.
2. Re-implemented System and Block module tabs/tasks without menu system improvements.
3. Fixed Drupal installer tries to write to {key_value} before keyvalue service is available.
4. Fixed _drupal_maintenance_theme() must scan filesystem for themes if requested maintenance theme is not available.

The 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."

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

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

Ah, 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.

Unfortunately, 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.

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

Drupal 9.

No, feature freeze is Dec 1st, 2012.

Spin-off: #1864066: Simplify hook_menu_local_tasks() and MENU_DEFAULT_LOCAL_TASK usage

#1798732: Convert install_task, install_time and install_current_batch to use the state system is back green and needs review.

To clarify:

1. With regard to overall architectural base system changes in Drupal 8, this patch is definitely in line and SHOULD be part of it. Especially with regard to CMI.
2. However, we never had a proper installation status for themes in Drupal's history. During my course of working on this and related issues, it became clear that this change WILL have repercussions down the line. It is possible to adjust all other parts of the system to account for this change, but we only know of the parts that happen to have test coverage — of which there isn't a lot. We should expect that a few things that are not covered by tests will potentially break in flabbergasting ways and resolving them may be challenging.
3. Meanwhile, the spin-off #1864066: Simplify hook_menu_local_tasks() and MENU_DEFAULT_LOCAL_TASK usage has landed, so that's a first dependency that's off the table.
4. #1798732: Convert install_task, install_time and install_current_batch to use the state system is still required and has to land first, but got blocked a second time on a discussion about technical perfectionism.

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.

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

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

Postponing on #2187717: Remove code duplication in system_list() and ThemeHandler::rebuildThemeData()

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

Waiting for #340723: Make modules and installation profiles only require .info.yml files to land, which might appear unrelated, but isn't.

Note: Attached patch is totally WIP and not functional yet. Only posting it because @dawehner asked in IRC.

1. Make ThemeHandler::enable() return Boolean operation status + cleanups.
2. Fixed theme_enable()/theme_disable() do not pass forward return value.
3. Fixed ThemeHandler::enable() must use system_rebuild_theme_data() to get dependency data.
4. Reverted unnecessary changes.
5. Synchronized ThemeHandler::enable() with ModuleHandler::install().
6. Removed obsolete _system_rebuild_theme_data() wrapper.
7. Make system_list() only return enabled themes, without fallback.
8. Allow _drupal_maintenance_theme() to dynamically load an uninstalled theme.
9. TDD: Added new DUTB test to verify enabling, disabling, uninstalling of themes. [WIP]

Status quo:

1. The new test contains exactly 3 assertions, and all they assert is that there are no themes.
2. The test passes when executed with run-tests.sh.
3. The test fails when executed in the Simpletest UI.

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.

Wowza! Writing that DUTB test was the best idea ever. TDD++ Tons of fixes:

1. Fixed #1770902: Theme of parent site executing test leaks into all tests
2. Fixed Graph breaks on empty array.
3. Fixed system_rebuild_theme_data() must return all available themes, not only enabled.
4. Fixed TestBase::prepareEnvironment() does not unset all theme globals.
5. Removed obsolete cache clears from system_list_reset().
6. Fixed theme_get_setting() does not check whether theme exists.
7. Fixed ThemeHandler::enable() + ::disable() do not maintain state of system.theme.data.
8. Fixed faulty config schema for system.theme.disabled.
9. Added default theme settings config files to test base/sub themes.
10. Added theme_get_setting() and disable+enable test coverage.
11. Added @todos.

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 in drupal_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

1. Stop installing Stark by default; make installation profiles specify themes to install.
2. Fixed Appearance page only shows enabled themes.
3. Fixed installer task list (sidebar) disappears if installer theme is not in themes to install.
4. Added major @todo to drupal_theme_initialize() to cope with a possible situation of no valid theme.

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

Fixed bogus 'theme_default' property in Standard profile.

Created 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

The quickfixes landed.

1. Re-adjusted ThemeHandler::disable() for secondary PHP array handling bug reported by #2011128: theme_disable() can disable the wrong theme
2. Fixed PHPUnit ThemeHandlerTest can no longer test enable() due to system_rebuild_theme_data() dependency.
3. Fixed _theme() may be called before the theme is initialized (e.g. in DUTB tests).

Two issues that will help us to move forward here:

1. #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.

2. #2218655: Core expects $theme.settings default config file to exist

Didn't look into the remaining test failures yet, but merged 8.x and:

1. Updated code for new core.extension config object.
2. Replaced 'theme_default' in installation profile .info.yml with regular system.theme config override.

1. Fixed typo in Batch\PageTest.
2. Fixed tests.

This 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. :P

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

Apparently, one of my earlier test failure fixes completely destroyed the whole point of this issue ;-) So...

1. Incorporated TwigSettingsTest fix from #171. (Leaving theme settings change to #2218655: Core expects $theme.settings default config file to exist)
2. Vastly enhanced test coverage, consolidating test assertions from InfoAlterTest + ThemeTest.
3. Fixed theme info data is not refreshed when a module is installed or uninstalled.
4. Added comment to explain why ThemeHandler cannot be injected into ModuleHandler. → #2206347: Use event system in ModuleHandler
5. Properly separate refresh logic from rebuilding logic + deprecate system_rebuild_theme_data().
6. Properly inject State service into ThemeHandler.
7. Use rebuildThemeData() method instead of system_rebuild_theme_data() in ThemeHandler::enable().
8. Updated UpdateServiceProvider's theme_handler service definition.

Curious where that leaves us... My only hope is that it doesn't melt down entirely. ;-)

hah, chasing HEAD is a different kind of meltdown. ;)

1. Fixed default configuration is re-installed when a theme is re-enabled.
2. Fixed DefaultConfigTest.
3. Fixed systemListReset() != resetSystem(); only the latter rebuilds routes (required).