The fix applied in #676800: Fieldsets break design badly does not get included during install as shown in the attached images.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

sun’s picture

cosmicdreams’s picture

subscribing

casey’s picture

Status: Active » Needs review
FileSize
1000 bytes

Same as done in garland.

cosmicdreams’s picture

tested patch from #3 in Firefox 3.5, Chrome 5 (beta) , Opera 10, IE 8, IE 7, IE 6

Firefox, Opera, and Chrome were fine.

IE 8, 7, and 6 all displayed the error shown above.

cosmicdreams’s picture

The issue appears to be with Fieldset .fieldset-legend's absolute positioning.

casey’s picture

Did you clear drupal cache?

cosmicdreams’s picture

Indeed I did. Casey are you seeing something different than I am?

casey’s picture

Status: Needs review » Needs work

I didn't test it; I was sure it would work. But apparently I was wrong :p

cosmicdreams’s picture

I used drush to flush the cache of the site. (since I had attempted to flush the cache after I had disabled my sql server settings). I'll try flushing my cache using drupal's Performance page tonight when I have more time.

cosmicdreams’s picture

Perhaps a second opinion on whether this patch fixes the issue will help this issue go forward. I could be doing something wrong in my testing of this.

casey’s picture

Status: Needs work » Needs review
FileSize
4.35 KB

Found the problem. theme_install_page() and theme_update_page() only calls template_ preprocess/process(); not theme's preprocess/process functions.

At first I wanted to add theme's preprocess/process functions to theme_install_page() and theme_update_page(), but then I figured why don't we just call theme('maintenance_page', $variables)?

It does work, but I think we need some theme system experts here. I am not sure of all consequenses.

tstoeckler’s picture

+  // While markup for normal pages is split into page.tpl.php and html.tpl.php,
+  // the markup for the maintenance page is all in the single
+  // maintenance-page.tpl.php template.

Is that a good thing? It feels like an inconsistency to me, but I wouldn't know.

I'm tempted to mark this as critical, as everyone installing Drupal on IE will see the pictures in #1 during install. If I would be installing something for the first time and seeing something like that I would probably stop right there...

casey’s picture

Priority: Normal » Critical

@tstoeckler I think it works this way because on maintenance pages you can't use the whole theming system as you not always can use a db. You could open a new issue for that however.

I agree on the critical

sun’s picture

Component: markup » theme system

This touches the innards of the theme system, so we need expert advice here.

cosmicdreams’s picture

FileSize
4.59 KB

reroll

cosmicdreams’s picture

FileSize
20.1 KB
41.52 KB

This patch does solve the problem listed above for IE 7 (Selection_018.png) and IE 6 (Selection_019.png)

Georg’s picture

Status: Needs review » Reviewed & tested by the community

Tested in IE8. Problem solved.

works!

sun’s picture

Status: Reviewed & tested by the community » Needs review

uh oh. This doesn't seem to be right. I can only guess that switching the maintenance_theme to Garland via settings.php, its CSS equally won't be properly included. We'd want to make the maintenance template work regardless of theme.

CVS annotate should reveal the issue(s) that introduced the code in theme_install_page() and theme_update_page(), which got removed here. IIRC, there were quite some edge-case problems that needed to be solved. (I guess that the theme system may not be available yet under certain conditions or something along the lines of that).

Georg’s picture

Status: Needs review » Needs work

I guess, this needs work then?

james.elliott’s picture

Subscribing

catch’s picture

I seem to remember seeing exactly this patch in another issue, but no idea which issue it was or why it's not committed yet. Not very helpful but maybe someone else's memory will be jogged.

Georg’s picture

Installing Drupal in IE still doesn't look right.

#15 still solves this cosmetic issue.

It kind of bugs me, that IE is such a pain in the ass, but we already have a solution for this problem, what are we waiting for. (I admit, that I don't understand the argument in #18. All I see, is that with the applied patch, it seems to work.)

casey’s picture

I don't understand #18 either. Is patch in #15 not ok?

ff1’s picture

settings.php currently states:

/**
 * A custom theme can be set for the offline page. This applies when the site
 * is explicitly set to maintenance mode through the administration page or when
 * the database is inactive due to an error. It can be set through the
 * 'maintenance_theme' key. The template file should also be copied into the
 * theme. It is located inside 'modules/system/maintenance-page.tpl.php'.
 * Note: This setting does not apply to installation and update pages.
 */
# $conf['maintenance_theme'] = 'garland';

So I'm not sure that #18 is valid. But, like others, I don't really understand #18, so maybe with this patch applied the $conf setting above now does apply to installation and update pages?

effulgentsia’s picture

subscribing

catch’s picture

Status: Needs work » Needs review

This needs review, not work. I still can't remember the other patch which looked the same as this one. I think sun's worried that by fixing an obscure bug here, we will regress an obscure bug somewhere else. That's fine but sadly the code this patch removes isn't particularly well commented, so someone needs to run cvs annotate, find the issue it was introduced, then see if that bug can be reproduced with the patch here applied. I'll do the cvs annotate bit if no-one beats me to it later today.

catch’s picture

The two issues which last touched this significantly were #141727: Regression: Allow maintenance to be themable from December 2007 and #421062: Maintenance theme is ugly and cannot be altered from December 2009. They look like more or less the same regression, so someone should verify that the patch doesn't break 421062, then we should be good here (or not).

Georg’s picture

Status: Needs review » Reviewed & tested by the community

I read through all of #421062: Maintenance theme is ugly and cannot be altered but I'm not sure, what the main issue is.

I've tested the patch in #15 again. It works fabulously.
On a completely installed Drupal, the maintenance page is always displayed with the default theme of the page. I guess that was the main issue of 421062. This also works for bartik and corolla.

When I set the maintenance theme in the settings.php before starting the installation process, the installation begins with the defined theme. BUT starting with "Install profile" all further pages only use the seven theme. This is an inconsistency I did not expect.

Ok, I figured it out. If I also change the maintenance theme in default.settings.php, then the installation is completely done in the specified theme. (settings.php is probably overridden)

But that's probably not the only way to specify a specific installation theme, because it influences the maintenance theme later on. Thus This should not be a problem. I have to find out, how to do that, it's fun!

I say this issue is solved with patch in #15. I set it to RTBC again, since we had it already RTBC before and nothing in the patch changed, and there is no regression to 421062.

Dries’s picture

Status: Reviewed & tested by the community » Fixed

I decided to commit #15. Inspecting the code, I think we should be good. If not, we'll find out quickly. :)

Committed to CVS HEAD. Thanks!

Georg’s picture

yippee! 8-)

Status: Fixed » Closed (fixed)

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