Support from Acquia helps fund testing for Drupal Acquia logo

Comments

mcjim’s picture

Status: Active » Needs review
FileSize
1.93 KB

Patch removes drupal_add_css() from _drupal_maintenance_theme().

CSS files are attached to the content in template_preprocess_maintenance_page(). Not sure if it's the best approach, but it seems to work :-)

tstoeckler’s picture

Looks good. I'm not sure about the type-checking, though. In which cases is $variables['content'] already string, and which cases isn't it?

mcjim’s picture

TBH, it's probably always a string. Think I'm being overly-defensive, there.

tstoeckler’s picture

Status: Needs review » Needs work

OK, can we remove the check then please? Let's just declare $variables['content'] all in one and be done with it.

Whether something has been rendered or not is extremely important information for themers. We always want to render as late as possible to make everything alterable. But for that reason it needs to be 100% clear at which point we have render arrays, and at which point we don't. Therefore - in my humble opinion - these sort of "safety checks", while certainly well intentioned, don't get me wrong, actually hide bugs instead of preventing them.

mcjim’s picture

Status: Needs work » Needs review
FileSize
1.87 KB

Thanks for reviewing, @tstoeckler.
Check removed and amended comment to clarify what it is happening.

tstoeckler’s picture

Status: Needs review » Reviewed & tested by the community

Awesome!

catch’s picture

Status: Reviewed & tested by the community » Needs work
+++ b/core/includes/theme.incundefined
@@ -3029,6 +3029,23 @@ function template_preprocess_maintenance_page(&$variables) {
+  // Attach CSS to content before rendering.
+  $variables['content'] = array(
+    '#markup' => $variables['content'],

It's not immediately clear where $variables['content'] gets set - is that $variables[$region] above? What if a theme doesn't have a region named 'content'?

mcjim’s picture

$variables['content'] gets set in the MaintenanceModeSubscriber with this line:

$content = theme('maintenance_page', array('content' => filter_xss_admin(t(config('system.maintenance')->get('message'), array('@site' => config('system.site')->get('name'))))));

So, hello subscribers. Nice to have met you :-)

So… should I assume we can't depend on this?

tstoeckler’s picture

Re #8: Thanks for figuring that out! For extra credit, could you go ahead and change that to remove the theme() call altogether? I.e. change that line to:

$content = array(
  '#theme' => 'maintenance_page',
  '#content' => filter_xss_admin(t(config('system.maintenance')->get('message'), array('@site' => config('system.site')->get('name')))),
);

That would be super cool!

tstoeckler’s picture

+++ b/core/includes/theme.inc
@@ -3029,6 +3029,23 @@ function template_preprocess_maintenance_page(&$variables) {
+  $variables['content'] = array(
+    '#markup' => $variables['content'],
+  );

Oh, and then we can remove that. I forgot to mention that.

mcjim’s picture

Status: Needs work » Needs review
FileSize
1.74 KB

Suggested approach attached.

Just adding and rendering the CSS within the preprocess.

It doesn't touch MaintenanceModeSubscriber, sorry @tstoeckler, as maybe that should be a separate issue under https://drupal.org/node/2006152? Also: I couldn't make it work for now, but I may be just tired ;-)

Wim Leers’s picture

Status: Needs review » Needs work

The last submitted patch, 2008270-11-remove-drupal_add_css.patch, failed testing.

adammalone’s picture

Status: Needs work » Needs review
FileSize
1.59 KB

Reroll of #11

Wim Leers’s picture

+++ b/core/includes/theme.incundefined
@@ -2962,6 +2962,21 @@ function template_preprocess_maintenance_page(&$variables) {
+  // These are usually added from system_init() -except maintenance.css.

Can you fix the typo in this line? I'll RTBC then — this is good to go. Thanks! :)

adammalone’s picture

Thanks Wim - removed the '-' in the latest patch.

Wim Leers’s picture

Status: Needs review » Reviewed & tested by the community

Great, thanks! :)

catch’s picture

Status: Reviewed & tested by the community » Needs work

Those aren't added from system_init() any more.

adammalone’s picture

Status: Needs work » Needs review
FileSize
1.59 KB

Right - they're added in system_page_build()

Rerolled...

catch’s picture

Status: Needs review » Reviewed & tested by the community
alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Committed 862b4d6 and pushed to 8.x. Thanks!

Status: Fixed » Closed (fixed)

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

JohnAlbin’s picture

Status: Closed (fixed) » Needs review
FileSize
2.93 KB

I was working on #1839318: Replace drupal.base.css library with normalize.css for all themes when I noticed this issue. Unfortunately, when all the CSS files were moved from _drupal_maintenance_theme() to template_preprocess_maintenance_page() (btw, yay!), the drupal.base.css was accidentally left in the former location. It was added just a few days before the last re-roll, so it was probably overlooked.

Also, we need to make the code in template_preprocess_maintenance_page() more closely match system_page_build() because the weights on these system CSS files now still when on regular vs. maintenance pages. That's probably my fault for not adding the weights to the maintenance versions when I added the weights on the normal side. The big chunk of code in this patch after $default_css = array(); is almost an exact copy of the code snippet from system_page_build().

Also, I don't believe system.admin.css should be conditionally added for maintenance pages. We should just add it for all maintenance pages.

Wim Leers’s picture

Oh :( Sorry to have overlooked that!

Are those weights truly necessary? The whole weights system is going away anyway, so if we introduce yet more complex weight handling here, it'll be more to clean up later.

I'll ask nod_ to chime in.

sdboyer’s picture

if those css files truly need to precede other things, they should probably be added as a library. we're really trying to kill weights (i got distracted last week, but i'm rounding the corner on a patch that does so).

this does smell like one of those places where the library/deps system isn't particularly well-suited to managing the css cascade, but still - if we can avoid mucking with weights like that, we should.

Wim Leers’s picture

catch’s picture

Status: Needs review » Needs work

The last submitted patch, 23: 2008270-23-fix-maintenance-css-weights.patch, failed testing.

mr.baileys’s picture

Issue summary: View changes
Status: Needs work » Needs review
FileSize
2.85 KB

Re-rolled and removed an unnecessary $default_css = array();.

Status: Needs review » Needs work

The last submitted patch, 29: 2008270-29-drupal_add_css-maintenance.patch, failed testing.

mr.baileys’s picture

Status: Needs work » Needs review

Test failures seem unrelated to this patch, and pass locally, so requesting a retest.

mr.baileys’s picture

vijaycs85’s picture

Coding standard updates...

Wim Leers’s picture

It looks like all patches since JohnAlbin's resurrection of this issue, including John's, actually try to attach the drupal.base library that was removed, instead of attaching the normalize library that we do have since #1839318: Replace drupal.base.css library with normalize.css for all themes.

The difference is that normalize.css then gets added to e.g. /update.php, whereas otherwise, it's not.

#33 updated the patch to better comply with coding standards. But the thing was that it was a 1:1 copy from system_page_build(). So then we should update the code there too.

Wim Leers’s picture

Title: Remove drupal_add_css() from _drupal_maintenance_theme() — use #attached » Remove drupal_add_css() from template_preprocess_maintenance_page() — use #attached

Function name has changed in the mean time.

andypost’s picture

Any reason to keep system_page_build() at all? Why not move this "attached" into appropriate preprocess functions?

+++ b/core/includes/theme.inc
@@ -2422,21 +2424,31 @@ function template_preprocess_maintenance_page(&$variables) {
+  $default_css['library'][] = array('system', 'normalize');

+++ b/core/modules/system/system.module
@@ -2117,16 +2117,26 @@ function system_filetransfer_info() {
+  // Ensure the same CSS is loaded in template_preprocess_maintenance_page().
   $page['#attached']['library'][] = array('system', 'normalize');

+++ b/core/includes/theme.inc
@@ -2422,21 +2424,31 @@ function template_preprocess_maintenance_page(&$variables) {
   // These are usually added from system_page_build() except maintenance.css.
   // When the database is inactive it's not called so we add it here.

So Any reason to duplicate this? Suppose better to add files once in preprocess

Wim Leers’s picture

#36: because system_page_build() runs for all pages (and only adds system.admin.css on admin paths, and never adds system.maintenance.css), except when maintenance mode is on, in which case template_preprocess_maintenance_page() runs, and hence we must duplicate analogous logic there (but in that case, always add system.(admin|maintenance).css)?

I think you might have missed that subtle detail? :)

nod_’s picture

Status: Needs review » Reviewed & tested by the community

After getting sidetracked for a while I ended up testing the patch. Works great.

andypost’s picture

@Wim no, I mean to separate this into template_preprocess_maintenance_page() and template_preprocess_page() and get rid of system_page_build() because it could not be executed when hooks(database) are not available, so this will allow get rid of the code duplication and this comment

+++ b/core/includes/theme.inc
@@ -2422,21 +2424,31 @@ function template_preprocess_maintenance_page(&$variables) {
   // These are usually added from system_page_build() except maintenance.css.
   // When the database is inactive it's not called so we add it here.
Wim Leers’s picture

#39: Ah, I see! That could work, yes. But: 1) AFAIK the Twig folks are trying to get rid of preprocess functions, 2) that's very much out of scope for this issue :)

Feel free to open a new issue for that though!

benjy’s picture

This looks good to go, +1 for RTBC.

webchick’s picture

Status: Reviewed & tested by the community » Fixed

This looks consistent with what we've done elsewhere for these patches. andypost's point may be worth a follow-up if the Twig maintainers think it's a good idea, but I agree it's not for this issue to solve how preprocess logic is organized.

Committed and pushed to 8.x. Thanks!

vijaycs85’s picture

Status: Fixed » Closed (fixed)

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

Wim Leers’s picture

Issue tags: -sprint