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

typhonius’s picture

Status: Active » Needs review
FileSize
1.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...

Wim Leers’s picture

Status: Needs review » Needs work

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

Wim Leers’s picture

Issue tags: +Novice, +Needs reroll

.

Tim Bozeman’s picture

Assigned: Unassigned » Tim Bozeman
Tim Bozeman’s picture

Assigned: Tim Bozeman » Unassigned
Issue tags: -Needs reroll
FileSize
861 bytes
1.32 KB
PASSED: [[SimpleTest]]: [MySQL] 58,531 pass(es). View

I emulated #44.

Cottser’s picture

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.

Cottser’s picture

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.

Tim Bozeman’s picture

Assigned: Unassigned » Tim Bozeman

I will add the comment.

Tim Bozeman’s picture

FileSize
408 bytes
1.45 KB
PASSED: [[SimpleTest]]: [MySQL] 58,553 pass(es). View

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

Tim Bozeman’s picture

Assigned: Tim Bozeman » Unassigned
Status: Needs work » Needs review
Cottser’s picture

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…

Tim Bozeman’s picture

Assigned: Unassigned » Tim Bozeman
Tim Bozeman’s picture

Status: Needs work » Needs review
FileSize
722 bytes
1.45 KB
PASSED: [[SimpleTest]]: [MySQL] 58,155 pass(es). View

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

Cottser’s picture

FileSize
796 bytes
1.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" />
Tim Bozeman’s picture

Assigned: Tim Bozeman » Unassigned
Wim Leers’s picture

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.

Cottser’s picture

Issue tags: +Needs reroll

Tagging for reroll.

RunePhilosof’s picture

Assigned: Unassigned » RunePhilosof

I'll reroll it

RunePhilosof’s picture

Assigned: RunePhilosof » Unassigned
Status: Needs work » Needs review
Issue tags: -Needs reroll
FileSize
185 bytes
1.7 KB
FAILED: [[SimpleTest]]: [MySQL] 58,582 pass(es), 0 fail(s), and 2 exception(s). View
RunePhilosof’s picture

FileSize
689 bytes

Interdiff in unified format.

RunePhilosof’s picture

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.

rteijeiro’s picture

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.

RunePhilosof’s picture

Status: Needs work » Needs review
FileSize
695 bytes
1.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).

RunePhilosof’s picture

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

kgoel’s picture

Status: Needs review » Reviewed & tested by the community

Patch applied cleanly and looks good.

rteijeiro’s picture

Ok, it seems the patch works well:

BEFORE

maintenance-before.png

AFTER

maintenance-after.png

Go RTBC, go!!

kgoel’s picture

Issue summary: View changes

Updated issue summary.

kgoel’s picture

Issue summary: View changes

Updated issue summary.

kgoel’s picture

Issue summary: View changes

Updated issue summary.

kgoel’s picture

Issue summary: View changes

Updated issue summary.

kgoel’s picture

Issue summary: View changes

Updated issue summary.

kgoel’s picture

Issue summary: View changes

Updated issue summary.

kgoel’s picture

Issue summary: View changes

Updated issue summary.

rteijeiro’s picture

Issue summary: View changes

Updated issue summary.

kgoel’s picture

Issue summary: View changes

Updated issue summary.

kgoel’s picture

Issue summary: View changes

Updated issue summary.

kgoel’s picture

Issue summary: View changes

Updated issue summary.

alexpott’s picture

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.

Anonymous’s picture

Issue summary: View changes

Updated issue summary.