Updated: Comment #N

Problem/Motivation

Main meta issue: #2073819: [META] Remove direct calls to drupal_add_css()

Proposed resolution

Add a hook_library_info() in the theme and use #attached/drupal_render() instead of drupal_add_js/css.

Uses libraries in the same way #1969916: Remove drupal_add_js/css from seven.theme

The reason we are using hook_library_info() in this issue because replace hook_library_info() by *.library.yml file is an active issue and we will continue to use hook_library_info until https://drupal.org/node/1996238 is resolved.

Remaining tasks

None

User interface changes

None

API changes

None

#1996238: Replace hook_library_info() by *.libraries.yml file
#1969916: Remove drupal_add_js/css from seven.theme
#2073819: [META] Remove direct calls to drupal_add_css()

Manual testing - #29 Done
Technical review - #28, #24Done

Main meta issue: #1839338: [meta] Remove drupal_set_*() drupal_add_*() in favour of #attached/response for out-of-band stuff

Original report by @typhonius

Files: 
CommentFileSizeAuthor
#29 maintenance-before.png65.75 KBrteijeiro
#29 maintenance-after.png65.54 KBrteijeiro
#26 2040269-26.patch1.7 KBRunePhilosof
PASSED: [[SimpleTest]]: [MySQL] 58,630 pass(es).
[ View ]
#26 interdiff-21-26.txt695 bytesRunePhilosof
#22 interdiff-15-21.txt689 bytesRunePhilosof
#21 2040269-21.patch1.7 KBRunePhilosof
FAILED: [[SimpleTest]]: [MySQL] 58,582 pass(es), 0 fail(s), and 2 exception(s).
[ View ]
#21 interdiff-15-21.txt185 bytesRunePhilosof
#15 2040269-15.patch1.69 KBCottser
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 2040269-15.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#15 interdiff.txt796 bytesCottser
#14 2040269_14_remove-drupal_add_css-from-bartik.patch1.45 KBTim Bozeman
PASSED: [[SimpleTest]]: [MySQL] 58,155 pass(es).
[ View ]
#14 interdiff-10-14.txt722 bytesTim Bozeman
#10 2040269_10_remove-drupal_add_css-from-bartik.patch1.45 KBTim Bozeman
PASSED: [[SimpleTest]]: [MySQL] 58,553 pass(es).
[ View ]
#10 interdiff-6-10.txt408 bytesTim Bozeman
#6 2040269_6_remove-drupal_add_css-from-bartik.patch1.32 KBTim Bozeman
PASSED: [[SimpleTest]]: [MySQL] 58,531 pass(es).
[ View ]
#6 interdiff-1-6.txt861 bytesTim Bozeman
#1 2040269_1_remove-drupal_add_css-from-bartik.patch1.11 KBtyphonius
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 2040269_1_remove-drupal_add_css-from-bartik.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

Comments

Status:Active» Needs review
StatusFileSize
new1.11 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 2040269_1_remove-drupal_add_css-from-bartik.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

Patch attached...

Status:Needs review» Needs work

The last submitted patch, 2040269_1_remove-drupal_add_css-from-bartik.patch, failed testing.

Issue tags:+Novice, +Needs reroll

.

Assigned:Unassigned» Tim Bozeman

Assigned:Tim Bozeman» Unassigned
Issue tags:-Needs reroll
StatusFileSize
new861 bytes
new1.32 KB
PASSED: [[SimpleTest]]: [MySQL] 58,531 pass(es).
[ View ]

I emulated #44.

Status:Needs work» Needs review

Thanks Tim! There's no 'Needs review' tag, removing the tag and setting the status to 'needs review' so that the patch gets tested.

Status:Needs review» Needs work

Also I think we should be including a comment to explain the approach, so CNW to add that. Comment can be copied from the other patch.

Assigned:Unassigned» Tim Bozeman

I will add the comment.

StatusFileSize
new408 bytes
new1.45 KB
PASSED: [[SimpleTest]]: [MySQL] 58,553 pass(es).
[ View ]

I copied the comment from patch #44 and included it here.

Assigned:Tim Bozeman» Unassigned
Status:Needs work» Needs review

Status:Needs review» Needs work

The comment should be in bartik_preprocess_maintenance_page above the render array/drupal_render(), not in hook_library_info…

Assigned:Unassigned» Tim Bozeman

Status:Needs work» Needs review
StatusFileSize
new722 bytes
new1.45 KB
PASSED: [[SimpleTest]]: [MySQL] 58,155 pass(es).
[ View ]

Thank you for walking me through these issues Cottser!
I moved the comment.

StatusFileSize
new796 bytes
new1.69 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 2040269-15.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

Thanks for your work on this @Tim Bozeman! I wanted to RTBC this but it did not pass manual testing (No bartik/css/maintenance-page.css showed up on the maintenance page). This revised patch does include the file, the only change in the resulting markup is the weight of maintenance-page.css.

Using RenderWrapper based on similar work done in seven.theme for #2004286: Defer calls to drupal_get_* functions until printed inside a template by adding a RenderWrapper class, in this case the result is the same as moving drupal_get_css() after the drupal_render().

Before:

<link rel="stylesheet" href="http://d8.dev/core/themes/bartik/css/maintenance-page.css?msble0" media="all" />
<link rel="stylesheet" href="http://d8.dev/core/themes/bartik/css/layout.css?msble0" media="all" />
<link rel="stylesheet" href="http://d8.dev/core/themes/bartik/css/style.css?msble0" media="all" />
<link rel="stylesheet" href="http://d8.dev/core/themes/bartik/css/colors.css?msble0" media="all" />
<link rel="stylesheet" href="http://d8.dev/core/themes/bartik/css/print.css?msble0" media="print" />

After:

<link rel="stylesheet" href="http://d8.dev/core/themes/bartik/css/layout.css?msble0" media="all" />
<link rel="stylesheet" href="http://d8.dev/core/themes/bartik/css/style.css?msble0" media="all" />
<link rel="stylesheet" href="http://d8.dev/core/themes/bartik/css/colors.css?msble0" media="all" />
<link rel="stylesheet" href="http://d8.dev/core/themes/bartik/css/print.css?msble0" media="print" />
<link rel="stylesheet" href="http://d8.dev/core/themes/bartik/css/maintenance-page.css?msble0" media="all" />

Assigned:Tim Bozeman» Unassigned

Issue tags:-Novice

#15: 2040269-15.patch queued for re-testing.

Status:Needs review» Needs work
Issue tags:+Novice

The last submitted patch, 2040269-15.patch, failed testing.

Issue tags:+Needs reroll

Tagging for reroll.

Assigned:Unassigned» RunePhilosof

I'll reroll it

Assigned:RunePhilosof» Unassigned
Status:Needs work» Needs review
Issue tags:-Needs reroll
StatusFileSize
new185 bytes
new1.7 KB
FAILED: [[SimpleTest]]: [MySQL] 58,582 pass(es), 0 fail(s), and 2 exception(s).
[ View ]

StatusFileSize
new689 bytes

Interdiff in unified format.

In #10 it was stated that the solution used is from https://drupal.org/node/1969916#comment-7797531.
In that issue catch commented https://drupal.org/node/1969916#comment-7797647 that the solution is only valid because it was the maintenance theme.
I don't know if the correction in #15 solved that, or if it isn't an issue anymore.

Status:Needs review» Reviewed & tested by the community

Patch applies well and seems to be right so it's a RTBC for me ;)

Status:Reviewed & tested by the community» Needs work

The last submitted patch, 2040269-21.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new695 bytes
new1.7 KB
PASSED: [[SimpleTest]]: [MySQL] 58,630 pass(es).
[ View ]

Catching up with core: Issue #2067017: Remove usage of DRUPAL_CORE_COMPATIBILITY/VERSION constants moved VERSION to \Drupal::VERSION.

I can see that the question I raised in #23 was a bit off.
It is fine because it is the maintenance page theming (inserted page there).

I did a manual testing with simplytest.me and found that the maintenance-page.css file is sent to the browser.

Status:Needs review» Reviewed & tested by the community

Patch applied cleanly and looks good.

StatusFileSize
new65.54 KB
new65.75 KB

Ok, it seems the patch works well:

BEFORE

maintenance-before.png

AFTER

maintenance-after.png

Go RTBC, go!!

Issue summary:View changes

Updated issue summary.

Issue summary:View changes

Updated issue summary.

Issue summary:View changes

Updated issue summary.

Issue summary:View changes

Updated issue summary.

Issue summary:View changes

Updated issue summary.

Issue summary:View changes

Updated issue summary.

Issue summary:View changes

Updated issue summary.

Issue summary:View changes

Updated issue summary.

Issue summary:View changes

Updated issue summary.

Issue summary:View changes

Updated issue summary.

Issue summary:View changes

Updated issue summary.

Status:Reviewed & tested by the community» Fixed

Committed ded8dc7 and pushed to 8.x. Thanks!

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

Issue summary:View changes

Updated issue summary.