Setting a currently disabled theme as admin puts block admin in a broken state

Or, don't let it happen. It's a simple test.

But is it right? As it is, a theme can be a base theme, even though not enabled. And it also can be an admin theme, though not enabled. Is that the documented, correct behavior? Why even distinguish between enabled and disabled themes then?

I looked at the code and it would take 1 IF statement to have the select list on the Administration Theme panel include only enabled themes. Seems that would be easier.

All they would have to do is enable the theme. That would initialize the block table and you wouldn't see the error. And I think it would make sense to a new user, ie., there is a meaning to enabled/disabled status.

ps. There is some strange behavior I came across while testing. In an unmodified install, if you set a disabled theme to admin, the block table does get initialized, but not with the map for that theme. If you then enable that theme, it still doesn't get changed to the block/region map the theme is supposed to have. I haven't checked why this is so.

Here is an alternative patch.

Well, obviously the system tests assume a disabled theme(stark) can be used as an admin theme.
I don't know if the patch really makes the UI less usable but I do think, to a new user, it makes it more sensible. If you disable a module, can it still be called by another module? This topic has been discussed elsewhere but not as a separate issue. But I don't think tradition is going to be overturned here and this will continue as a Drupal 'quirk'.

I should put this back to needs review for #3. No telling what would be required for #8 if the philosophy is baked into Drupal. Do think this will come up again.

I looked further into this, because it still bothers me that disable=enable sometimes and it finally struck me that perhaps the real problem isn't actually addressed by this patch. I noticed that admin/structure/blocks shouldn't be displaying a disabled theme in any case. In fact, if you use any non-core theme for admin(while disabled) it won't show up in in structure/blocks. And gartner/stark don't show up while disabled unless they are the admin theme.I think it is the design that structure/blocks works only with enabled themes. So why do gartner and stark make an exception?

I have done some more experimentation and found some weird results. The code says that the themes listed at the top of the administer/structure/blocks form is limited to enabled themes and the current admin theme(see @line 180 in block.module). If you use either garland or stark as an admin theme, while never having enabled them, they show up on the administer/structure/blocks form, but don't have any blocks assigned to any regions, in particular 'Main page content' . If you have a non core admin theme (eg. root candy or om admin) and assign it as the admin theme, while never having it or its base them enabled, the admin theme does NOT show up in the administer/structure/blocks form. The 'admin theme' variable has the correct value. Nor does it show up in the 'block' table in the database, while the core themes do. All the themes used as admin, enabled or not, seem to work fine.

Well I found out why core and non core themes were treated differently. The 2 non core themes I used have a value for NAME in their .info file that is different than the name of the theme's directory. If you look at the code

function _drupal_theme_access($theme) {
  $admin_theme = variable_get('admin_theme');
  return !empty($theme->status) || ($admin_theme && $theme->name == $admin_theme);
}

You can see that the var admin-theme, which is equal to the 'machine' theme name, is compared to the variable theme->name which is equal to the 'human' readable name set in the theme's .info file. And in the 2 non core themes I tried, they are not equal. When I changed the .info file NAME to be equal to the theme directory name, the behavior was the same as with the core themes.

So, if it is not necessary to have a admin theme enabled to use it, why do we list it on the admin/structure/block page?

This is sad, as I can no longer reproduce the weird behavior. Very frustrating. But same question: why include admin theme in block theme list if not necessary? If necessary, then #3 above is a sufficient patch.

Well, I can recreate it. It just depends on when system_rebuild_theme_data gets run. It is probably the is_object test in #15 that fails. The error only occurs if you want to redo the block/region mapping for the admin theme, which is probably not unusual. The themes will still display just fine, but might have different blocks located in different locations, depending on how the theme was initially introduced to the system. Can someone mark #3 RTBC?

I rerolled the patch, with a small comment change, so we can set it for review.

Thanks for your work on this patch. Note that the Drupal 8.x patch will need to be rerolled, because the core directory structure for Drupal 8 has now changed. (For more information, see #22336: Move all core Drupal files under a /core folder to improve usability and upgrades). When the patch has been rerolled, please set the issue back to "Needs Review."

Tagging as novice for the task of rerolling the Drupal 8.x patch.

If you need help rerolling this patch, you can come to core office hours or ask in #drupal-gitsupport on IRC.

Re-rolled for new drupal. Also changed comment to use same terminology as in the function called.

All I found was the 2 space rule. Is that what you wanted?

Don't know what happened.

#25: admin_theme_blocks_broken-1293550-25.patch queued for re-testing.

Again, from beginning.

One more time, then look elsewhere.

#23 is the correct change. There appears to be a testbot issue at the moment, so I'd wait to retry the patch until it's fixed.

I kept trying because I thought I may have corrupted my 8.x clone. All the fixes are the same.

Alright, #31 and brethren look good. Thanks!

Can we write a test for this? The test should fail when uploaded in a patch by itself, and pass when patched together with #31.

The Block module is an optional module. Doesn't that mean this patch will lead to whitescreens when the function isn't defined?

It would look like that. One side affect of this patch is that every block defined in the default theme gets jammed into the admin theme's content region, forcing you to immediately go to the block form to clean up the block/region map.
I'd like to ask a question I asked before. Why does drupal insist that the admin theme show up in the block form even if the admin theme isn't enabled? Without this patch there doesn't seem to be any problem using a disabled admin theme except on the block form. And there wouldn't be a problem there if the admin theme wasn't visible.

This adds a test for the block module and does nothing if it doesn't exist, as the problem is moot then.

ps. Just a note. I discovered that system/page.tpl.php has several errors if block isn't enabled.

In the latest 8.x, I am unable to duplicate the bug described in the original issue summary. I follow those steps and load the block page, and the content appears in the appropriate region. I even deleted all rows in {block} that refer to stark, and they are re-generated upon visiting the block admin page. Marking as needs work, but perhaps would be better as "closed (cannot reproduce)"?

Certainly have been some code changes since this was last looked at. But might #1373312: Assign system_main block to 'content' region and help block to 'help' region by default (followup) have fixed this?

Should test with garland or seven though. Stark doesn't specify any regions so the above mentioned fix would be sufficient.

#1373312: Assign system_main block to 'content' region and help block to 'help' region by default (followup) may also be back ported to D7. Another related issue is #1172560: The block X was assigned to the invalid region Y and has been disabled.. I think still need to test other admin themes than stark. Will get to that if someone doesn't beat me to it.

If you'd ask me, then this issue won't fix.

The entire idea of being able to use a disabled extension is so fundamentally flawed it couldn't be more.

See #1067408: Themes do not have an installation status for a full stack of reasons why we do not want to pursue this anti-pattern anymore.

Yes, this means you will have to enable the theme you want to use as admin theme. Did you ever expect a module to be usable without being enabled? Of course not.

You can currently use base theme(s) that haven't been enabled as well. Shouldn't this be wrong too? Has this become a bug that needs to be fixed? Or a behavior that needs to be documented. The errors seem to occur because you can try to use a theme's templates/styles/scripts on the current page even though the "INFO" array for that theme hasn't been initialized.

What does #542828: Do not special case disabled admin theme do to this issue?

well, yeah, like that. But the auto-enabling behavior looks wonky to me. That's not necessarily what I want or expect as a user.

Instead, the select should just simply expose valid options only.

For D8, I'd really prefer the select to not contain invalid options as in #51.

Whether there is an additional link for "enable and set as administration theme" for each disabled theme, I don't really care (but am concerned about visual clutter in the interface).

However, magically auto-enabling extensions under the hood in a form submission handler is an anti-pattern that we don't do anywhere else, and is a utterly bad idea to do, because it doesn't account for all of the possible special conditions that may need to be handled, such as missing dependencies, incompatible Drupal core version, incompatible PHP version, etc.pp. To account for all of that, you'd have to resemble and duplicate the entire extension management logic in that form submission handler, add an appropriate validation handler, and react to special conditions properly in the UI.

Therefore, the proper fix is to keep all of that logic where it belongs and only expose enabled themes as possible admin theme options. That ensures that each option went through the required validation already and can actually be used as admin theme.

For D7, we could do it (#48), because the current functionality doesn't account for all of that either way, and the admin theme functionality cannot get any more broken either way. I.e., it looks like D7 allows you to configure and use an admin theme that is incompatible or lacks dependencies.

Ideally, I'd prefer to fix D7 in the same way as D8 though.

Is far as I can tell, this still need quite a bit of work.

Therefore, the proper fix is to keep all of that logic where it belongs and only expose enabled themes as possible admin theme options. That ensures that each option went through the required validation already and can actually be used as admin theme.

We still haven't figured out exactly how this problem should be solved.

I think the following issue (and specific comment) are relevant to this discussion:
#1104126-2: Admin theme might not be block configurable

So it turns out that #542828: Do not special case disabled admin theme will actually do what I have in mind for D8.

That likely means we can move this issue to D7, although we might want to wait with that until the patch of aforementioned issue got committed to D8.

This is fixed in D8. You can't select a disabled theme for admin.

Fixed issue for D7. With this patch, only enabled themes are listed in the drop-down for admin themes.

Do we need to handle the case where a user may already have a disabled theme enabled for the admin?

59: drupal7-only_enabled_themes_visible_in_admin_themes-1293550-59.patch queued for re-testing.

I think the tests need to be altered in order so that, admin theme may be selected only if its enabled.