Problem

  1. Theme settings/configuration/variables are never removed.
  2. All themes are loaded unconditionally, regardless of whether they are going to be used.
  3. Base themes are initialized and used even if they are disabled, and even if they are not installed at all.
  4. Themes can have configuration and can participate in hooks, but they are not able to maintain their data in any way.
  5. String translations of a theme are re-imported whenever a theme is re-enabled.
  6. The update system/manager is not able to determine whether a theme is actually used.
  7. ...
  8. Themes cannot be installed or uninstalled.

Goal

  • Sanity.

Proposed solution

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

  2. No change: Themes can be enabled and disabled.
  3. Separate follow-up: Allow to uninstall themes.

Particular @todos to resolve/revert

Follow-up issues

CommentFileSizeAuthor
#222 interdiff.txt4.02 KBsun
#222 theme.install.222.patch92.6 KBsun
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 68,161 pass(es). View
#217 1067408.217.patch93.61 KBalexpott
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 68,213 pass(es). View
#217 213-217-interdiff.txt1.66 KBalexpott
#213 interdiff.txt3.77 KBsun
#213 theme.install.213.patch91.42 KBsun
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 68,205 pass(es). View
#209 interdiff.txt3.79 KBsun
#209 theme.install.209.patch90.49 KBsun
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 68,195 pass(es). View
#207 interdiff.txt17.78 KBsun
#207 theme.install.207.patch86.7 KBsun
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 68,116 pass(es), 0 fail(s), and 1 exception(s). View
#203 theme.install.203.patch80.1 KBsun
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 68,066 pass(es). View
#201 interdiff.txt1.05 KBsun
#201 theme.install.201.patch81.31 KBsun
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 68,070 pass(es). View
#199 theme.install.199.patch81.24 KBsun
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 67,943 pass(es), 119 fail(s), and 1 exception(s). View
#194 interdiff.txt2.35 KBsun
#194 theme.install.194.patch83.71 KBsun
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 67,848 pass(es). View
#192 drupal8-theme_install-1067408-192.patch81.91 KBJalandhar
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 67,796 pass(es), 3 fail(s), and 0 exception(s). View
#188 theme.install.188.patch81.92 KBsun
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch theme.install.188.patch. Unable to apply patch. See the log in the details link for more information. View
#185 interdiff.txt5.64 KBdawehner
#185 theme.install.184.patch85.98 KBdawehner
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 67,693 pass(es), 2 fail(s), and 0 exception(s). View
#182 interdiff.txt10.48 KBsun
#182 theme.install.182.patch81.99 KBsun
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 67,651 pass(es), 2 fail(s), and 0 exception(s). View
#180 interdiff.txt9.13 KBsun
#180 theme.install.180.patch71.8 KBsun
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 67,530 pass(es), 7 fail(s), and 0 exception(s). View
#178 theme.install.178.patch66.57 KBsun
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 67,132 pass(es), 6 fail(s), and 0 exception(s). View
#176 interdiff.txt37.48 KBsun
#176 theme.install.176.patch66.58 KBsun
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch theme.install.176.patch. Unable to apply patch. See the log in the details link for more information. View
#171 interdiff.txt2.33 KBjessebeach
#171 theme-install-1067408-171.patch51.86 KBjessebeach
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 67,104 pass(es), 18 fail(s), and 0 exception(s). View
#166 interdiff.txt9.67 KBsun
#166 theme.install.166.patch49.54 KBsun
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 67,022 pass(es), 19 fail(s), and 0 exception(s). View
#164 interdiff.txt8.57 KBsun
#164 theme.install.164.patch44.52 KBsun
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 66,887 pass(es), 31 fail(s), and 7 exception(s). View
#161 interdiff.txt3.31 KBsun
#161 theme.install.161.patch45.98 KBsun
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 66,721 pass(es), 31 fail(s), and 9 exception(s). View
#155 interdiff.txt1.54 KBsun
#155 theme.install.155.patch47.11 KBsun
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch theme.install.155.patch. Unable to apply patch. See the log in the details link for more information. View
#153 interdiff.txt8.08 KBsun
#153 theme.install.153.patch47.01 KBsun
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Drupal installation failed. View
#151 interdiff.txt18.7 KBsun
#151 theme.install.151.patch41.36 KBsun
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Failed to run tests: PHP Fatal error encountered during run_tests.sh. See review log for details.. View
#149 interdiff.txt26.89 KBsun
#149 theme.install.149.patch29.63 KBsun
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Failed to run tests: @reason. View
#145 theme.install.142.patch19.79 KBDésiré
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Failed to run tests: @reason. View
#138 theme.install.138.patch20.55 KBDésiré
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Failed to run tests: @reason. View
#128 theme.install.128.patch20.57 KBsun
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Failed to run tests: failed to login to test site. View
#122 1067408-framework.themes.merge-installer-services-123.patch29.78 KBneetu morwani
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed. View
#122 1067408-framework.themes.merge-installer-services-123.patch29.78 KBneetu morwani
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed. View
#119 1067408-framework.themes.merge-installer-services-119.patch53.54 KBneetu morwani
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed. View
#117 theme-1067408.patch23 KBdawehner
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed. View
#86 framework.themes.86.patch47.8 KBsun
FAILED: [[SimpleTest]]: [MySQL] 48,439 pass(es), 66 fail(s), and 82 exception(s). View
#86 interdiff.txt714 bytessun
#82 framework.themes.merge-installer-services.do-not-test.patch37.71 KBsun
#75 framework.themes.75.patch53.57 KBsun
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed. View
#75 interdiff.txt4.79 KBsun
#73 framework.themes.73.patch48.96 KBsun
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed. View
#73 interdiff.txt17.49 KBsun
#72 framework.themes.72.patch55.86 KBsun
FAILED: [[SimpleTest]]: [MySQL] 2,277 pass(es), 586 fail(s), and 1,474 exception(s). View
#69 framework.themes.69.patch56.27 KBsun
FAILED: [[SimpleTest]]: [MySQL] 2,207 pass(es), 582 fail(s), and 1,162 exception(s). View
#65 framework.themes.65.patch56.97 KBsun
FAILED: [[SimpleTest]]: [MySQL] 42,036 pass(es), 54 fail(s), and 2,610 exception(s). View
#65 interdiff.txt2.71 KBsun
#63 framework.themes.63.patch55.6 KBsun
FAILED: [[SimpleTest]]: [MySQL] 42,160 pass(es), 89 fail(s), and 75 exception(s). View
#63 interdiff.txt8.1 KBsun
#61 framework.themes.61.patch48.04 KBsun
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed. View
#61 interdiff.txt3.04 KBsun
#57 framework.themes.57.patch50.33 KBsun
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch framework.themes.57.patch. Unable to apply patch. See the log in the details link for more information. View
#51 framework.themes.51.patch50.92 KBsun
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch framework.themes.51_0.patch. Unable to apply patch. See the log in the details link for more information. View
#51 interdiff.txt7.2 KBsun
#48 framework.themes.48.patch46.8 KBsun
PASSED: [[SimpleTest]]: [MySQL] 41,319 pass(es). View
#42 framework.themes.42.patch46.79 KBsun
PASSED: [[SimpleTest]]: [MySQL] 41,292 pass(es). View
#42 interdiff.txt1.08 KBsun
#35 framework.themes.35.patch45.89 KBsun
PASSED: [[SimpleTest]]: [MySQL] 41,209 pass(es). View
#35 interdiff.txt2.6 KBsun
#34 framework.themes.34.patch45.55 KBsun
PASSED: [[SimpleTest]]: [MySQL] 41,211 pass(es). View
#33 framework.themes.33.patch45.52 KBsun
PASSED: [[SimpleTest]]: [MySQL] 41,205 pass(es). View
#33 interdiff.txt8.23 KBsun
#31 framework.themes.31.patch38.27 KBsun
FAILED: [[SimpleTest]]: [MySQL] 41,191 pass(es), 6 fail(s), and 11 exception(s). View
#29 framework.themes.29.patch36 KBsun
FAILED: [[SimpleTest]]: [MySQL] 41,175 pass(es), 6 fail(s), and 17 exception(s). View
#22 framework.themes.22.patch35.38 KBsun
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch framework.themes.22.patch. Unable to apply patch. See the log in the details link for more information. View
#22 interdiff.txt1.62 KBsun
#20 framework.themes.20.patch30.29 KBsun
FAILED: [[SimpleTest]]: [MySQL] 40,358 pass(es), 7 fail(s), and 53 exception(s). View
#20 interdiff.txt14.12 KBsun
#18 framework.themes.18.patch18.23 KBsun
FAILED: [[SimpleTest]]: [MySQL] 40,050 pass(es), 17 fail(s), and 167 exception(s). View
#18 interdiff.txt13.03 KBsun
#13 framework.themes.13.patch5.93 KBsun
FAILED: [[SimpleTest]]: [MySQL] 40,051 pass(es), 18 fail(s), and 169 exception(s). View
Members fund testing for the Drupal project. Drupal Association Learn more

Comments

andypost’s picture

String translations of a theme are re-imported whenever a theme is re-enabled. That's utterly wrong.

Could you describe this, I think that theme can change while been disabled so better to re-import strings.

sun’s picture

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

catch’s picture

Subscribing. This would help to make list_themes() a bit less of a headache.

David_Rothstein’s picture

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

bfroehle’s picture

Also, it would help prevent uninstalled themes from breaking an entire installation, cf., #619542: Malformed theme .info files break menu_router generation.

yoroy’s picture

Is #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?

sun’s picture

Category: task » feature
andypost’s picture

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

dww’s picture

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

sun’s picture

Category: feature » task
Priority: Major » Normal
Status: Active » Needs review
FileSize
5.93 KB
FAILED: [[SimpleTest]]: [MySQL] 40,051 pass(es), 18 fail(s), and 169 exception(s). View

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

Status: Needs review » Needs work

The last submitted patch, framework.themes.13.patch, failed testing.

sun’s picture

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.

sun’s picture

Status: Needs work » Needs review
FileSize
13.03 KB
18.23 KB
FAILED: [[SimpleTest]]: [MySQL] 40,050 pass(es), 17 fail(s), and 167 exception(s). View

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.

Status: Needs review » Needs work

The last submitted patch, framework.themes.18.patch, failed testing.

sun’s picture

Component: base system » theme system
Priority: Normal » Major
Status: Needs work » Needs review
FileSize
14.12 KB
30.29 KB
FAILED: [[SimpleTest]]: [MySQL] 40,358 pass(es), 7 fail(s), and 53 exception(s). View

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

Status: Needs review » Needs work

The last submitted patch, framework.themes.20.patch, failed testing.

sun’s picture

Status: Needs work » Needs review
FileSize
1.62 KB
35.38 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch framework.themes.22.patch. Unable to apply patch. See the log in the details link for more information. View

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.

sun’s picture

Status: Needs work » Needs review
sun’s picture

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

sun’s picture

Status: Needs work » Needs review
FileSize
36 KB
FAILED: [[SimpleTest]]: [MySQL] 41,175 pass(es), 6 fail(s), and 17 exception(s). View

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.

Status: Needs review » Needs work

The last submitted patch, framework.themes.29.patch, failed testing.

sun’s picture

Status: Needs work » Needs review
FileSize
38.27 KB
FAILED: [[SimpleTest]]: [MySQL] 41,191 pass(es), 6 fail(s), and 11 exception(s). View

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

Status: Needs review » Needs work

The last submitted patch, framework.themes.31.patch, failed testing.

sun’s picture

Status: Needs work » Needs review
FileSize
8.23 KB
45.52 KB
PASSED: [[SimpleTest]]: [MySQL] 41,205 pass(es). View

Alright, the last remaining puzzle piece:

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

sun’s picture

FileSize
45.55 KB
PASSED: [[SimpleTest]]: [MySQL] 41,211 pass(es). View

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

sun’s picture

FileSize
2.6 KB
45.89 KB
PASSED: [[SimpleTest]]: [MySQL] 41,209 pass(es). View

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.

sun’s picture

Issue tags: +Configuration system

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.

tim.plunkett’s picture

All 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?

rbayliss’s picture

2. Base themes are treated like module dependencies, they need to be enabled first.

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

sun’s picture

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.

sun’s picture

Anything else I could clarify? :)

rbayliss’s picture

Some 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?

sun’s picture

FileSize
1.08 KB
46.79 KB
PASSED: [[SimpleTest]]: [MySQL] 41,292 pass(es). View

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.

Status: Needs review » Needs work

The last submitted patch, framework.themes.42.patch, failed testing.

sun’s picture

Status: Needs work » Needs review

Random test failures...

sun’s picture

Any chance to move forward here?

rbayliss’s picture

Looks RTBC to me, but this is a big enough change that I don't feel comfortable changing the status myself.

carwin’s picture

Status: Needs review » Reviewed & tested by the community

sun's patch in #42 applied cleanly.

Steps to make an error appear:

  • Enabled / Disabled various themes.
  • Set various themes to default.
  • Changed admin themes.
  • On the Block UI page, re-arranged blocks on multiple themes.
  • Disabled themes after arranging blocks.
  • Re-enabled those themes and found that block settings persisted

Really nothing to report other than the fact that it works fine. I'd say it's ready to roll.

sun’s picture

FileSize
46.8 KB
PASSED: [[SimpleTest]]: [MySQL] 41,319 pass(es). View

Merged in latest HEAD. No other changes.

catch’s picture

Status: Reviewed & tested by the community » Needs work
+++ b/core/includes/menu.incundefined
@@ -2195,7 +2199,7 @@ function menu_primary_local_tasks() {
 /**
@@ -2203,7 +2207,7 @@ function menu_secondary_local_tasks() {

@@ -2203,7 +2207,7 @@ function menu_secondary_local_tasks() {
  */
 function menu_local_actions() {
   $links = menu_local_tasks();
-  return $links['actions']['output'];
+  return $links['actions'];
 }

What's all this got to do with installation status for themes?

+++ b/core/includes/module.incundefined
@@ -65,7 +65,7 @@ function module_load_all($bootstrap = FALSE) {
+function module_list($type = 'module', array $fixed_list = NULL) {
   // This static is only used for $fixed_list.
   $module_list = &drupal_static(__FUNCTION__);
 
@@ -101,13 +101,13 @@ function module_list_reset() {

@@ -101,13 +101,13 @@ function module_list_reset() {
  *
  * @param $type
  *   The type of list to return:
- *   - module_enabled: All enabled modules.
+ *   - module: All enabled modules.
  *   - bootstrap: All enabled modules required for bootstrap.
- *   - theme: All themes.
+ *   - theme: All enabled themes.

Also why rename the key here?

+++ b/core/includes/module.incundefined
@@ -158,11 +158,11 @@ function system_list($type) {
+      $result = db_query("SELECT * FROM {system} WHERE (type = 'theme' OR type = 'module') AND status = 1 ORDER BY weight ASC, name ASC");

This can just use IN() instead of OR now.

+++ b/core/includes/theme.incundefined
@@ -38,6 +38,34 @@
+function _theme_menu_title($theme) {
+  return check_plain($theme->info['name']);

Any reason why this needs to be a private function?

+++ b/core/includes/theme.incundefined
@@ -48,11 +76,18 @@
 function drupal_theme_access($theme) {
+  // Early-return for potentially bogus '0', FALSE, or NULL values.
+  if (empty($theme)) {
+    return FALSE;

Why would bogus '0', FALSE or NULL values be passed in here? And why fail silently without an error message when that happens?

+++ b/core/includes/theme.incundefined
@@ -1387,29 +1422,130 @@ function theme_render_template($template_file, $variables) {
+
+        drupal_set_installed_schema_version($key, $version);
+        // Allow the theme to perform install tasks.
+        $function = $key . '_install';
+        if (function_exists($function)) {
+          $function();

Do we really want to let themes do this?

+++ b/core/modules/block/block.moduleundefined
@@ -141,39 +149,86 @@ function block_menu() {
+/**
+ * Implements hook_menu_local_tasks_alter().
+ */
+function block_menu_local_tasks_alter(&$data, $router_item, $root_path) {
+  if ($root_path == 'admin/structure/block' || strpos($root_path, 'admin/structure/block/list') === 0) {
+    // Replace the tab title of the default local task with the name of the

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?

+++ b/core/modules/forum/forum.moduleundefined
index bfba2fd..d879d35 100644
--- a/core/modules/node/node.module

--- a/core/modules/node/node.module
+++ b/core/modules/node/node.moduleundefined

+++ b/core/modules/node/node.moduleundefined
+++ b/core/modules/node/node.moduleundefined
@@ -2042,7 +2042,7 @@ function node_menu_local_tasks_alter(&$data, $router_item, $root_path) {

@@ -2042,7 +2042,7 @@ function node_menu_local_tasks_alter(&$data, $router_item, $root_path) {
   if ($root_path == 'admin/content') {
     $item = menu_get_item('node/add');
     if ($item['access']) {
-      $data['actions']['output'][] = array(
+      $data['actions'][] = array(

Not really keen on the API change, I don't understand why we need to change local task theming here at all tbh.

sun’s picture

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

sun’s picture

Status: Needs work » Needs review
FileSize
7.2 KB
50.92 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch framework.themes.51_0.patch. Unable to apply patch. See the log in the details link for more information. View

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.

Status: Needs review » Needs work

The last submitted patch, framework.themes.51.patch, failed testing.

sun’s picture

Jeff Burnz’s picture

I'm fairly sure that advanced themes like Omega and others would have a use-case for that.

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

sun’s picture

Status: Needs work » Needs review
Issue tags: -Framework Initiative, -API clean-up, -Configuration system

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

Status: Needs review » Needs work
Issue tags: +Framework Initiative, +API clean-up, +Configuration system

The last submitted patch, framework.themes.51.patch, failed testing.

sun’s picture

Status: Needs work » Needs review
FileSize
50.33 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch framework.themes.57.patch. Unable to apply patch. See the log in the details link for more information. View

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

Status: Needs review » Needs work
Issue tags: -Framework Initiative, -API clean-up, -Configuration system

The last submitted patch, framework.themes.57.patch, failed testing.

sun’s picture

Status: Needs work » Needs review

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

Status: Needs review » Needs work
Issue tags: +Framework Initiative, +API clean-up, +Configuration system

The last submitted patch, framework.themes.57.patch, failed testing.

sun’s picture

Status: Needs work » Needs review
FileSize
3.04 KB
48.04 KB
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed. View

Merged HEAD.
Fixed merge conflicts.

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

Status: Needs review » Needs work

The last submitted patch, framework.themes.61.patch, failed testing.

sun’s picture

Status: Needs work » Needs review
FileSize
8.1 KB
55.6 KB
FAILED: [[SimpleTest]]: [MySQL] 42,160 pass(es), 89 fail(s), and 75 exception(s). View

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.

Status: Needs review » Needs work

The last submitted patch, framework.themes.63.patch, failed testing.

sun’s picture

Status: Needs work » Needs review
FileSize
2.71 KB
56.97 KB
FAILED: [[SimpleTest]]: [MySQL] 42,036 pass(es), 54 fail(s), and 2,610 exception(s). View

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.

Status: Needs review » Needs work

The last submitted patch, framework.themes.65.patch, failed testing.

Sylvain Lecoy’s picture

That's make even more sense as a theme name cannot be the same as a module name, and vice-versa.

thedavidmeister’s picture

sun, do you need a hand or are you just smashing this one out yourself?

sun’s picture

Status: Needs work » Needs review
FileSize
56.27 KB
FAILED: [[SimpleTest]]: [MySQL] 2,207 pass(es), 582 fail(s), and 1,162 exception(s). View

Merged HEAD.

Status: Needs review » Needs work

The last submitted patch, framework.themes.69.patch, failed testing.

Anonymous’s picture

Issue summary: View changes

Revamped issue summary.

podarok’s picture

Issue tags: +D8MI, +CMI

tagging for 4 and 5 items from summary

sun’s picture

Status: Needs work » Needs review
FileSize
55.86 KB
FAILED: [[SimpleTest]]: [MySQL] 2,277 pass(es), 586 fail(s), and 1,474 exception(s). View

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

sun’s picture

FileSize
17.49 KB
48.96 KB
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed. View
  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.

Status: Needs review » Needs work

The last submitted patch, framework.themes.73.patch, failed testing.

sun’s picture

Status: Needs work » Needs review
FileSize
4.79 KB
53.57 KB
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed. View

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

Status: Needs review » Needs work

The last submitted patch, framework.themes.75.patch, failed testing.

sun’s picture

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

markcarver’s picture

sun, 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!

sun’s picture

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.

KrisBulman’s picture

Sun, what can us patch testers do? Simply apply the patch and test installation under any installation circumstance we can think of?

markcarver’s picture

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

Additional uncaught exception thrown while handling exception.
Original

Symfony\Component\DependencyInjection\Exception\InvalidArgumentException: The service definition "plugin.manager.views.access" does not exist. in Symfony\Component\DependencyInjection\ContainerBuilder->getDefinition() (line 690 of /server/websites/d8-test.sandbox/core/vendor/symfony/dependency-injection/Symfony/Component/DependencyInjection/ContainerBuilder.php).
Additional

Symfony\Component\DependencyInjection\Exception\InvalidArgumentException: The service definition "plugin.manager.views.access" does not exist. in Symfony\Component\DependencyInjection\ContainerBuilder->getDefinition() (line 690 of /server/websites/d8-test.sandbox/core/vendor/symfony/dependency-injection/Symfony/Component/DependencyInjection/ContainerBuilder.php).

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

sun’s picture

Status: Needs work » Postponed
FileSize
37.71 KB

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.

sun’s picture

Category: task » feature
Priority: Major » Normal

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.

markcarver’s picture

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

thedavidmeister’s picture

Just cross-linking #474684: Allow themes to declare dependencies on modules that would love to see themes have a "real" installation status.

sun’s picture

Status: Postponed » Needs review
FileSize
714 bytes
47.8 KB
FAILED: [[SimpleTest]]: [MySQL] 48,439 pass(es), 66 fail(s), and 82 exception(s). View

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

Status: Needs review » Needs work

The last submitted patch, framework.themes.86.patch, failed testing.

sun’s picture

Status: Needs work » Postponed

Drupal 9.

tim.plunkett’s picture

Status: Postponed » Needs review

You do realize that feature freeze isn't until February 18th, right?

sun’s picture

Status: Needs review » Postponed

No, feature freeze is Dec 1st, 2012.

dww’s picture

Status: Postponed » Needs review

Yay, 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:

The purpose of this phase will be to provide dedicated time to tie up loose ends on any features that have either been committed already, or features still in the queue that have demonstrated substantial progress before December 1, but are not quite "there" yet. While hard-and-fast rules around "substantial progress" are difficult to define, generally it means patches should be well underway, with a recent patch posted in the past two weeks, ideally with several community reviews, tests passing or nearly passing, and a clear path to getting the feature completed within the timeframe of Feature Completion Phase. Almost-working patches posted for the first time at 11:59pm on November 30 unfortunately won't cut it. :-)

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

markcarver’s picture

Status: Needs review » Needs work

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

sun’s picture

Berdir’s picture

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

sun’s picture

YesCT’s picture

jibran’s picture

Version: 8.x-dev » 9.x-dev

as per #88

markcarver’s picture

Version: 9.x-dev » 8.x-dev

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

markcarver’s picture

Version: 8.x-dev » 9.x-dev

Oh, unless it's past Feb 18... ugh, shame.

mgifford’s picture

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

thedavidmeister’s picture

damn. I was hoping this would make it >.<

effulgentsia’s picture

Version: 9.x-dev » 8.x-dev
Category: feature » task

I 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?

sun’s picture

Priority: Normal » Major
Status: Needs work » Postponed

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.

heyrocker’s picture

Issue tags: -CMI

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

jibran’s picture

catch’s picture

Priority: Major » Critical

If this blocks CMI it should probably be critical.

markcarver’s picture

catch’s picture

Status: Postponed » Active
Jeff Burnz’s picture

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

Jeff Burnz’s picture

Issue summary: View changes

Updated issue summary.

dawehner’s picture

Issue summary: View changes
jibran’s picture

As @sun said in #103

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.

Can you post WIP patch so that we can move forward.

xjm’s picture

Hmm. If we still intend to do this, it should probably be beta-blocking.

effulgentsia’s picture

Issue tags: +beta blocker

So 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?

klonos’s picture

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

Gábor Hojtsy’s picture

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

klonos’s picture

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

dawehner’s picture

Status: Active » Needs review
FileSize
23 KB
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed. View

This is a first approach for a rerole, which tries to reduce the amount of changes to make this patch somehow manageable.

Status: Needs review » Needs work

The last submitted patch, 117: theme-1067408.patch, failed testing.

neetu morwani’s picture

FileSize
53.54 KB
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed. View

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

neetu morwani’s picture

Assigned: sun » neetu morwani
Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 119: 1067408-framework.themes.merge-installer-services-119.patch, failed testing.

neetu morwani’s picture

Status: Needs work » Needs review
FileSize
29.78 KB
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed. View
29.78 KB
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed. View

Patch rerolled again.

The last submitted patch, 122: 1067408-framework.themes.merge-installer-services-123.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 122: 1067408-framework.themes.merge-installer-services-123.patch, failed testing.

sun’s picture

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.

saki007ster’s picture

Assigned: neetu morwani » Unassigned
cosmicdreams’s picture

Parts I and II are now in core. Is that enough to move forward on this issue?

#2188661: Extension System, Part II: ExtensionDiscovery

sun’s picture

Assigned: Unassigned » sun
Status: Needs work » Needs review
Related issues: +#2208429: Extension System, Part III: ExtensionList, ModuleExtensionList and ProfileExtensionList
FileSize
20.57 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Failed to run tests: failed to login to test site. View

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.

Status: Needs review » Needs work

The last submitted patch, 128: theme.install.128.patch, failed testing.

sun’s picture

sun’s picture

sun’s picture

catch’s picture

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

sun’s picture

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.

alexpott’s picture

Issue tags: -beta target +beta blocker

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

sun’s picture

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

Berdir’s picture

Status: Postponed » Active

That went in, so this should (finally) be unblocked? :)

Désiré’s picture

Status: Active » Needs review
FileSize
20.55 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Failed to run tests: @reason. View

I have just rerolled the patch from #128.

Status: Needs review » Needs work

The last submitted patch, 138: theme.install.138.patch, failed testing.

Désiré’s picture

Status: Needs work » Needs review

138: theme.install.138.patch queued for re-testing.

Status: Needs review » Needs work

The last submitted patch, 138: theme.install.138.patch, failed testing.

mgifford’s picture

It applies fine locally...

tstoeckler’s picture

Status: Needs work » Needs review

138: theme.install.138.patch queued for re-testing.

Status: Needs review » Needs work

The last submitted patch, 138: theme.install.138.patch, failed testing.

Désiré’s picture

FileSize
19.79 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Failed to run tests: @reason. View

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

  public function enable(array $theme_list, $enable_dependencies = TRUE) {
    if ($enable_dependencies) {
      // Get all theme data so we can find dependencies and sort.
      $theme_data = $this->rebuildThemeData();
      // Create an associative array with weights as values.
      $theme_list = array_flip(array_values($theme_list));
Désiré’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 145: theme.install.142.patch, failed testing.

Désiré’s picture

Assigned: sun » Désiré
sun’s picture

Assigned: Désiré » sun
Status: Needs work » Needs review
FileSize
29.63 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Failed to run tests: @reason. View
26.89 KB

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.

Status: Needs review » Needs work

The last submitted patch, 149: theme.install.149.patch, failed testing.

sun’s picture

Status: Needs work » Needs review
FileSize
41.36 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Failed to run tests: PHP Fatal error encountered during run_tests.sh. See review log for details.. View
18.7 KB

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

Status: Needs review » Needs work

The last submitted patch, 151: theme.install.151.patch, failed testing.

sun’s picture

  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.

Status: Needs review » Needs work

The last submitted patch, 153: theme.install.153.patch, failed testing.

sun’s picture

Status: Needs work » Needs review
FileSize
47.11 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch theme.install.155.patch. Unable to apply patch. See the log in the details link for more information. View
1.54 KB

Fixed bogus 'theme_default' property in Standard profile.

Status: Needs review » Needs work

The last submitted patch, 155: theme.install.155.patch, failed testing.

sun’s picture

markcarver’s picture

155: theme.install.155.patch queued for re-testing.

The last submitted patch, 155: theme.install.155.patch, failed testing.

sun’s picture

Status: Needs work » Needs review
FileSize
45.98 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 66,721 pass(es), 31 fail(s), and 9 exception(s). View
3.31 KB

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

Status: Needs review » Needs work

The last submitted patch, 161: theme.install.161.patch, failed testing.

sun’s picture

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

sun’s picture

Status: Needs work » Needs review
FileSize
44.52 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 66,887 pass(es), 31 fail(s), and 7 exception(s). View
8.57 KB

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.

Status: Needs review » Needs work

The last submitted patch, 164: theme.install.164.patch, failed testing.

sun’s picture

Status: Needs work » Needs review
Related issues: +#2218039: Render the maintenance/install page like any other HTML page
FileSize
49.54 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 67,022 pass(es), 19 fail(s), and 0 exception(s). View
9.67 KB
  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

Status: Needs review » Needs work

The last submitted patch, 166: theme.install.166.patch, failed testing.

jessebeach’s picture

Drupal\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 and Drupal\system\Tests\Theme\TwigSettingsTest remained unaffected with the same number of fails.

jessebeach’s picture

There's a @todo in bartik.info.yml that should be addressed in this issue:

# @todo D8: Remove once themes have to be installed.
settings:
  shortcut_module_link: '0'
sun’s picture

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

jessebeach’s picture

FileSize
51.86 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 67,104 pass(es), 18 fail(s), and 0 exception(s). View
2.33 KB

I've fixed the TwigSettingsTest failures and removed the setting from bartik.install that I noted in #169.

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

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

jessebeach’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 171: theme-install-1067408-171.patch, failed testing.

jessebeach’s picture

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

jessebeach’s picture

The Change Record just needs a review.

https://drupal.org/node/2232651

sun’s picture

Status: Needs work » Needs review
FileSize
66.58 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch theme.install.176.patch. Unable to apply patch. See the log in the details link for more information. View
37.48 KB

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

Status: Needs review » Needs work

The last submitted patch, 176: theme.install.176.patch, failed testing.

sun’s picture

Status: Needs work » Needs review
FileSize
66.57 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 67,132 pass(es), 6 fail(s), and 0 exception(s). View

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

Status: Needs review » Needs work

The last submitted patch, 178: theme.install.178.patch, failed testing.

sun’s picture

Status: Needs work » Needs review
FileSize
71.8 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 67,530 pass(es), 7 fail(s), and 0 exception(s). View
9.13 KB
  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).

Status: Needs review » Needs work

The last submitted patch, 180: theme.install.180.patch, failed testing.

sun’s picture

Status: Needs work » Needs review
FileSize
81.99 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 67,651 pass(es), 2 fail(s), and 0 exception(s). View
10.48 KB
  1. Fixed PHPUnit ThemeHandlerTest.
  2. Fixed ConfigImportUITest.
  3. Moved configuration translation UI test case for themes into separate ConfigTranslationUiThemeTest.
  4. Fixed ConfigTranslationUiThemeTest. config_translation module does not clear cached plugin definitions.

The only remaining 2 test failures should be in RenderElementTypesTest → #2218039: Render the maintenance/install page like any other HTML page

dawehner’s picture

  1. +++ b/core/includes/module.inc
    @@ -40,32 +32,15 @@ function system_list($type) {
    +    // ThemeHandler maintains the 'system.theme.data' state record.
    +    $theme_data = \Drupal::state()->get('system.theme.data', array());
         foreach ($theme_data as $name => $theme) {
    

    Urgs ... 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?

  2. +++ b/core/includes/theme.inc
    @@ -101,7 +101,19 @@ function drupal_theme_initialize() {
    +  $theme = \Drupal::service('theme.negotiator')->determineActiveTheme($request);
    +
    +  // If no theme could be negotiated, or if the negotiated theme is not within
    +  // the list of enabled themes, fall back to the default theme output of core
    +  // and modules (similar to Stark, but without a theme extension at all). This
    +  // is possible, because _drupal_theme_initialize() always loads the Twig theme
    +  // engine.
    +  if (!$theme || !isset($themes[$theme])) {
    +    $theme = 'core';
    +    $theme_key = $theme;
    +    _drupal_theme_initialize(new Extension('theme', 'core/core.info.yml'));
    +    return;
    +  }
    

    Why can't we write a theme negotiator which provides the fallback?

  3. +++ b/core/modules/system/lib/Drupal/system/Controller/SystemController.php
    --- a/core/modules/system/lib/Drupal/system/Controller/ThemeController.php
    +++ b/core/modules/system/lib/Drupal/system/Controller/ThemeController.php
    
    +++ b/core/modules/system/lib/Drupal/system/Controller/ThemeController.php
    @@ -32,11 +32,12 @@ class ThemeController extends ControllerBase {
    +    $theme_handler = \Drupal::service('theme_handler');
    
    @@ -74,14 +75,11 @@ public function disable(Request $request) {
    +    $theme_handler = \Drupal::service('theme_handler');
    

    THis is a clear sign where we can inject it.

Status: Needs review » Needs work

The last submitted patch, 182: theme.install.182.patch, failed testing.

dawehner’s picture

FileSize
85.98 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 67,693 pass(es), 2 fail(s), and 0 exception(s). View
5.64 KB

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

sun’s picture

Status: Needs work » Needs review

Checking to see whether the interdiff from #185 is safe to apply to my sandbox branch.

Status: Needs review » Needs work

The last submitted patch, 185: theme.install.184.patch, failed testing.

sun’s picture

Status: Needs work » Needs review
FileSize
81.92 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch theme.install.188.patch. Unable to apply patch. See the log in the details link for more information. View

Merged 8.x.

Status: Needs review » Needs work

The last submitted patch, 188: theme.install.188.patch, failed testing.

jibran’s picture

Status: Needs work » Needs review

188: theme.install.188.patch queued for re-testing.

Status: Needs review » Needs work

The last submitted patch, 188: theme.install.188.patch, failed testing.

Jalandhar’s picture

Assigned: sun » Unassigned
Status: Needs work » Needs review
FileSize
81.91 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 67,796 pass(es), 3 fail(s), and 0 exception(s). View

Updating the patch with reroll. Please review.

Status: Needs review » Needs work

The last submitted patch, 192: drupal8-theme_install-1067408-192.patch, failed testing.

sun’s picture

Assigned: Unassigned » sun
Status: Needs work » Needs review
FileSize
83.71 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 67,848 pass(es). View
2.35 KB
  1. Updated test_subtheme.schema.yml.
  2. Fixed RenderElementTypesTest.
sun’s picture

I 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

Gábor Hojtsy’s picture

@sun: the change notice looks good, does not talk about translations anymore though?!

sun’s picture

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

Gábor Hojtsy’s picture

Indeed, looks good! Thanks!

sun’s picture

FileSize
81.24 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 67,943 pass(es), 119 fail(s), and 1 exception(s). View

Merged 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 for ThemeHandler'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.

Status: Needs review » Needs work

The last submitted patch, 199: theme.install.199.patch, failed testing.

sun’s picture

Status: Needs work » Needs review
FileSize
81.31 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 68,070 pass(es). View
1.05 KB

Moved default config files.

dawehner’s picture

Status: Needs review » Reviewed & tested by the community
  1. +++ b/core/includes/install.core.inc
    @@ -1836,6 +1838,34 @@ function install_profile_modules(&$install_state) {
    +  $current_themes = $theme_handler->listInfo();
    ...
    +  foreach ($current_themes as $theme) {
    +    $theme_handler->addTheme($theme);
    +  }
    

    Do we also want to add themes which are not enabled here? I would assume that the code is doing that.

  2. +++ b/core/lib/Drupal/Core/Extension/ThemeHandler.php
    @@ -200,52 +321,63 @@ public function disable(array $theme_list) {
    +  public function refreshInfo() {
    +    $this->reset();
    

    Do we really need to keep reset and refreshInfo separate?

sun’s picture

FileSize
80.1 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 68,066 pass(es). View

#2218039: Render the maintenance/install page like any other HTML page has landed (again), so this needed a merge.

@dawehner:

  1. Yeah, ThemeHandler::addTheme() works (sorta) identically to ModuleHandler::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 a InstallerKernel & Co).
  2. I didn't see a way around the separate refreshInfo() method without proper events. That's going to be obsolete and can be refactored, once #2206347: Use event system in ModuleHandler has landed.
dawehner’s picture

Okay, cool

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
  1. +++ b/core/modules/system/lib/Drupal/system/Controller/SystemController.php
    @@ -183,16 +183,19 @@ public function systemAdminMenuBlockPage() {
    -    $themes = $this->themeHandler->listInfo();
    +    $themes = system_rebuild_theme_data();
         uasort($themes, 'system_sort_modules_by_info_name');
    

    Is this change necessary - and if it is should we be using the rebuildThemeData method on the themehandler?

  2. +++ b/core/modules/system/lib/Drupal/system/Tests/Extension/ThemeHandlerTest.php
    @@ -0,0 +1,335 @@
    +  /**
    +   * Tests disabling a sub-theme.
    +   */
    +  function testDisableSubTheme() {
    +    $name = 'test_subtheme';
    +    $base_name = 'test_basetheme';
    +
    +    $this->themeHandler()->enable(array($name));
    +    $this->themeHandler()->disable(array($name));
    +
    +    $themes = $this->themeHandler()->listInfo();
    +    $this->assertFalse(isset($themes[$name]));
    +    $this->assertTrue(isset($themes[$base_name]));
    +  }
    

    Where is a test for disabling a base theme before a sub theme?

  3. +++ b/core/modules/system/lib/Drupal/system/Tests/System/ThemeTest.php
    @@ -213,7 +213,7 @@ function testAdministrationTheme() {
         \Drupal::config('system.theme')
    -      ->set('default', 'bartik')
    +      ->set('default', 'stark')
           ->save();
    

    This is now unnecessary and so is enabling the bartik theme at the beginning of this test.

  4. +++ b/core/tests/Drupal/Tests/Core/Extension/ThemeHandlerTest.php
    @@ -113,14 +115,17 @@ protected function setUp() {
    +   * @requires function system__too_much_mocking
    +   *
    
    @@ -131,6 +136,8 @@ public function testThemeEnableWithTooLongName() {
    +   * @requires function system__too_much_mocking
    +   *
    
    @@ -177,6 +184,8 @@ public function testEnableSingleTheme() {
    +   * @requires function system__too_much_mocking
    +   *
    

    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.

alexpott’s picture

Forgot to say - fantastic work!

Making profiles declare their themes huge +1 the programatic enabling in hook_install was always ugly.

sun’s picture

Status: Needs work » Needs review
FileSize
86.7 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 68,116 pass(es), 0 fail(s), and 1 exception(s). View
17.78 KB
  1. Use injected ThemeHandler instead of system_rebuild_theme_data() in SystemController.
  2. Updated for #2202629: Move Drupal\Core\KeyValueStore\State* into Drupal\Core\State\State*
  3. Added test for disabling base theme before sub theme (and both at the same time).
  4. Added tests for enabling and disabling a non-existing theme.
  5. Cleaned up tests.
  6. Removed ThemeHandlerTest phpunit test methods that require too many mocked dependencies.

Status: Needs review » Needs work

The last submitted patch, 207: theme.install.207.patch, failed testing.

sun’s picture

Status: Needs work » Needs review
FileSize
90.49 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 68,195 pass(es). View
3.79 KB

Fixed MenuRouterTest.

alexpott’s picture

+++ b/core/tests/Drupal/Tests/Core/Extension/ThemeHandlerTest.php
@@ -122,134 +122,6 @@ protected function setUp() {
   /**
-   * Tests enabling a theme with a name longer than 50 chars.
-   *
-   * @requires function system__too_much_mocking
-   *
-   * @expectedException \Drupal\Core\Extension\ExtensionNameLengthException
-   * @expectedExceptionMessage Theme name <em class="placeholder">thisNameIsFarTooLong0000000000000000000000000000051</em> is over the maximum allowed length of 50 characters.
-   */
-  public function testThemeEnableWithTooLongName() {
-    $this->themeHandler->enable(array('thisNameIsFarTooLong0000000000000000000000000000051'));
-  }

Is this tested anyway else? The other removed tests certainly are but I'm not sure about this one.

jessebeach’s picture

Status: Needs review » Needs work
  1. +++ b/core/includes/theme.inc
    @@ -103,7 +103,19 @@ function drupal_theme_initialize() {
    +  if (!$theme || !isset($themes[$theme])) {
    +    $theme = 'core';
    +    $theme_key = $theme;
    +    _drupal_theme_initialize(new Extension('theme', 'core/core.info.yml'));
    +    return;
    +  }
    

    It'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?

  2. +++ b/core/includes/theme.inc
    @@ -874,7 +888,7 @@ function theme_get_setting($setting_name, $theme = NULL) {
    @@ -981,7 +995,7 @@ function theme_settings_convert_to_config(array $theme_settings, Config $config)
    
    @@ -981,7 +995,7 @@ function theme_settings_convert_to_config(array $theme_settings, Config $config)
      * @see \Drupal\Core\Extension\ThemeHandler::enable().
      */
     function theme_enable($theme_list) {
    -  \Drupal::service('theme_handler')->enable($theme_list);
    +  return \Drupal::service('theme_handler')->enable($theme_list);
     }
     
     /**
    @@ -996,7 +1010,7 @@ function theme_enable($theme_list) {
    
    @@ -996,7 +1010,7 @@ function theme_enable($theme_list) {
      * @see \Drupal\Core\Extension\ThemeHandler::disable().
      */
     function theme_disable($theme_list) {
    -  \Drupal::service('theme_handler')->disable($theme_list);
    +  return \Drupal::service('theme_handler')->disable($theme_list);
     }
     
     /**
    

    These are missing the @return line in the docblock.

  3. +++ b/core/modules/system/lib/Drupal/system/Tests/Extension/ThemeHandlerTest.php
    @@ -0,0 +1,409 @@
    +
    +/**
    + * @file
    + * Contains \Drupal\system\Tests\Extension\ThemeHandlerTest.
    + */
    +
    +namespace Drupal\system\Tests\Extension;
    +
    +use Drupal\Core\DependencyInjection\ContainerBuilder;
    +use Drupal\simpletest\DrupalUnitTestBase;
    +
    +/**
    + * Tests installing/enabling, disabling, and uninstalling of themes.
    + */
    +class ThemeHandlerTest extends DrupalUnitTestBase {
    

    We already have \Drupal\Tests\Core\Extension\ThemeHandlerTest. Maybe combine them?

  4. +++ b/core/modules/system/system.module
    @@ -1357,22 +1322,24 @@ function _system_default_theme_features() {
      *
    - * @param $theme_key
    - *   The name of a theme.
    + * @param $theme
    + *   A theme object, or the name of a theme.
    

    The type for this param should be \Drupal\Core\Extension\Extension|string

jessebeach’s picture

Oh! Forgot to mention. The updates to the Change Notice are great. That's good to go.

sun’s picture

Status: Needs work » Needs review
FileSize
91.42 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 68,205 pass(es). View
3.77 KB

@alexpott: #2244917: Centralize extension name length validation into ExtensionDiscovery


  1. Added comments for fake/null/core theme.
  2. Added missing @return docs to enable() and disable().
  3. see #185
  4. Polished phpDoc + code of system_region_list().
jessebeach’s picture

Those are my concerns addressed. Thanks sun!

alexpott’s picture

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

sun’s picture

Status: Needs review » Postponed

Well, 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

alexpott’s picture

Status: Postponed » Needs review
FileSize
1.66 KB
93.61 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 68,213 pass(es). View

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

xjm’s picture

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

xjm’s picture

+++ b/core/lib/Drupal/Core/Extension/ThemeHandler.php
@@ -143,53 +223,107 @@ public function enable(array $theme_list) {
+      // @todo Remove all code that relies on $status property.

Is there an issue for this?

jessebeach’s picture

Status: Needs review » Reviewed & tested by the community

+ // @todo Remove all code that relies on $status property.

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

xjm’s picture

Agreed. Thanks @jessebeach!

sun’s picture

Title: Themes need an installation status » Themes do not have an installation status
Category: Task » Bug report
FileSize
92.6 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 68,161 pass(es). View
4.02 KB

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

catch’s picture

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

YesCT’s picture

Issue summary: View changes

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

YesCT’s picture

Issue summary: View changes

updated summary with a link to the newly created (postponed) child #2247355: Allow to initialize ActiveTheme without Extension object

webchick’s picture

Status: Reviewed & tested by the community » Fixed

All 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!

  • Commit 2c90c67 on 8.x by webchick:
    Issue #1067408 by alexpott, Jalandhar, jessebeach, Désiré, neetu morwani...

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.

chx’s picture

I 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