Convert admin_theme to CMI

I did a bit of research and found out that the value that gets saved by the ui is '0'. Providing a default value with config('system.site')->get('admin_theme') ?: 0 has a bad side effect. As the $admin_theme variable now is an integer the comparison $theme->name == $admin_theme tries to cast $theme->name also to an integer. A non numerical string gets evaluated to 0, which in the end results for the comparison to evaluate to TRUE.

I've adjusted the patch accordingly. Otherwise the patch looks fine.

Please review :)

This needs an upgrade path.

Added upgrade path

+++ b/core/includes/bootstrap.incundefined
@@ -2477,9 +2477,6 @@ function drupal_container(Container $new_container = NULL, $rebuild = FALSE) {
-    // Register the EntityManager.
-    $container->register('plugin.manager.entity', 'Drupal\Core\Entity\EntityManager');

Why is this moved to CoreBundle.php? It was by accident? I don't get it :/

Removed the change that moved the register to CoreBundle.php and rerolled.

@penyaskito: I have no idea how that happened...

This is funny: it's a problem when we are merging the previous config and the default config.

It uses \Drupal\Component\Utility\NestedArray::mergeDeepArray, which modifies the numeric keys.
I created #1831038: Upgrade fails updating 403 and 404 pages for tracking that.

This is blocked by #1825466: Allow NestedArray::mergeDeepArray() to preserve integer keys

Not blocked anymore

#11: 1798872_admin_theme_11.patch queued for re-testing.

Rerolled!

the update number just needs to be brought up to date then it should be RTBC.

Re-rolling with update_N

#21: 1798872_admin_theme_21.patch queued for re-testing.

Updating config value and update_N.

This looks fine, should be good to go.

sorry jumped too soon.

-      $admin_theme = variable_get('admin_theme', 0);
-      if ($admin_theme != 0 && $admin_theme != $theme) {
+      $admin_theme = config('system.site')->get('admin_theme');
+      if ($admin_theme && $admin_theme != $theme) {

Not sure why the second line has changed from admin_theme != 0 to $admin_theme, surely that changes the logic.

Updating review comment changes.

Cool this now looks ready to go.

Thanks! Committed/pushed to 8.x.

mmm... this was supposed to go into the system.theme config object :-/

Sorry, I think that wasn't documented or stated anywhere.

Essentially, system.theme is envisioned to look like this in the end:

default: bartik
admin: seven
enabled:
  bartik: 0
  seven: 0

As a result, all configuration values that are necessary to initialize the theme are contained in the single system.theme config object.

Attached patch moves the setting from system.site into system.theme.

Looks good to me and it makes sense having all theme stuff in one place.

#31: config.admin-theme.31.patch queued for re-testing.

Thanks - testbot hiccup, back to RTBC.

Yes that does make more sense in there, I can see both of these configuration objects being loaded without the other being necessary.