Files: 
CommentFileSizeAuthor
#31 config.admin-theme.31.patch11.73 KBsun
PASSED: [[SimpleTest]]: [MySQL] 49,326 pass(es).
[ View ]
#28 1798872_admin_theme_28.patch11.36 KBvijaycs85
PASSED: [[SimpleTest]]: [MySQL] 49,280 pass(es).
[ View ]
#25 1798872_admin_theme_25.patch11.47 KBvijaycs85
PASSED: [[SimpleTest]]: [MySQL] 49,268 pass(es).
[ View ]
#21 1798872_admin_theme_21.patch11.54 KBvijaycs85
PASSED: [[SimpleTest]]: [MySQL] 48,963 pass(es).
[ View ]
#19 1798872_admin_theme_19.patch11.54 KBpcambra
PASSED: [[SimpleTest]]: [MySQL] 48,101 pass(es).
[ View ]
#11 1798872_admin_theme_11.patch11.54 KBpcambra
FAILED: [[SimpleTest]]: [MySQL] Invalid PHP syntax in core/modules/system/system.install.
[ View ]
#8 1798872_admin_theme_8.patch12.79 KBleschekfm
FAILED: [[SimpleTest]]: [MySQL] Invalid PHP syntax in core/modules/system/system.install.
[ View ]
#5 1798872_admin_theme_5.patch10.97 KBleschekfm
PASSED: [[SimpleTest]]: [MySQL] 46,582 pass(es).
[ View ]
#2 1798872_admin_theme_1.patch11.05 KBandreiashu
PASSED: [[SimpleTest]]: [MySQL] 41,898 pass(es).
[ View ]

Comments

andreiashu’s picture

I'm a bit confused that in system_themes_page() in system.admin.inc this variable defaults to int 0 ($admin_theme = variable_get('admin_theme', 0); and later on we compare it with string '0' (if ($theme->name == $admin_theme || ($theme->is_default && $admin_theme == '0')) {.
Keeping in mind that in PHP "0" != '' when I convert this variable to CMI I'll probably have initialise the default value to '' somewhere in system.site.yml which will make the above if statement fail?

andreiashu’s picture

Status:Active» Needs review
StatusFileSize
new11.05 KB
PASSED: [[SimpleTest]]: [MySQL] 41,898 pass(es).
[ View ]

a first stab. Something for testbot to chew

andreiashu’s picture

Issue tags:+Configuration system

Tagging

andreiashu’s picture

Status:Needs review» Needs work

not cool... Although I didn't address the issue in #1 the tests passed which means that actually #1 is a non issue or we simply don't have a test for that. will investigate

leschekfm’s picture

StatusFileSize
new10.97 KB
PASSED: [[SimpleTest]]: [MySQL] 46,582 pass(es).
[ View ]

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

leschekfm’s picture

Status:Needs work» Needs review
penyaskito’s picture

Status:Needs review» Needs work
Issue tags:+Needs upgrade path

This needs an upgrade path.

leschekfm’s picture

StatusFileSize
new12.79 KB
FAILED: [[SimpleTest]]: [MySQL] Invalid PHP syntax in core/modules/system/system.install.
[ View ]

Added upgrade path

leschekfm’s picture

Status:Needs work» Needs review
penyaskito’s picture

Status:Needs review» Needs work
Issue tags:-Needs 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 :/

pcambra’s picture

Assigned:andreiashu» Unassigned
Status:Needs work» Needs review
StatusFileSize
new11.54 KB
FAILED: [[SimpleTest]]: [MySQL] Invalid PHP syntax in core/modules/system/system.install.
[ View ]

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

Status:Needs review» Needs work

The last submitted patch, 1798872_admin_theme_11.patch, failed testing.

leschekfm’s picture

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

penyaskito’s picture

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.

alexpott’s picture

Status:Needs work» Postponed
pcambra’s picture

Status:Postponed» Needs review

Not blocked anymore

pcambra’s picture

Issue tags:-Configuration system

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

Status:Needs review» Needs work
Issue tags:+Configuration system

The last submitted patch, 1798872_admin_theme_11.patch, failed testing.

pcambra’s picture

Status:Needs work» Needs review
StatusFileSize
new11.54 KB
PASSED: [[SimpleTest]]: [MySQL] 48,101 pass(es).
[ View ]

Rerolled!

ACF’s picture

Status:Needs review» Needs work

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

vijaycs85’s picture

StatusFileSize
new11.54 KB
PASSED: [[SimpleTest]]: [MySQL] 48,963 pass(es).
[ View ]

Re-rolling with update_N

vijaycs85’s picture

Status:Needs work» Needs review

Status:Needs review» Needs work
Issue tags:-Configuration system

The last submitted patch, 1798872_admin_theme_21.patch, failed testing.

vijaycs85’s picture

Status:Needs work» Needs review
Issue tags:+Configuration system

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

vijaycs85’s picture

StatusFileSize
new11.47 KB
PASSED: [[SimpleTest]]: [MySQL] 49,268 pass(es).
[ View ]

Updating config value and update_N.

ACF’s picture

Status:Needs review» Reviewed & tested by the community

This looks fine, should be good to go.

ACF’s picture

Status:Reviewed & tested by the community» Needs work

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.

vijaycs85’s picture

Status:Needs work» Needs review
StatusFileSize
new11.36 KB
PASSED: [[SimpleTest]]: [MySQL] 49,280 pass(es).
[ View ]

Updating review comment changes.

ACF’s picture

Status:Needs review» Reviewed & tested by the community

Cool this now looks ready to go.

catch’s picture

Status:Reviewed & tested by the community» Fixed

Thanks! Committed/pushed to 8.x.

sun’s picture

Status:Fixed» Needs review
StatusFileSize
new11.73 KB
PASSED: [[SimpleTest]]: [MySQL] 49,326 pass(es).
[ View ]

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.

marcingy’s picture

Status:Needs review» Reviewed & tested by the community

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

Status:Reviewed & tested by the community» Needs work
Issue tags:-Configuration system

The last submitted patch, config.admin-theme.31.patch, failed testing.

ACF’s picture

Status:Needs work» Needs review
Issue tags:+Configuration system

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

sun’s picture

Status:Needs review» Reviewed & tested by the community

Thanks - testbot hiccup, back to RTBC.

catch’s picture

Status:Reviewed & tested by the community» Fixed

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

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