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

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

adammalone’s picture

Status: Active » Needs review
FileSize
1.11 KB

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

I emulated #44.

star-szr’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.

star-szr’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

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

Tim Bozeman’s picture

Assigned: Tim Bozeman » Unassigned
Status: Needs work » Needs review
star-szr’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

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

star-szr’s picture

FileSize
796 bytes
1.69 KB

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.

star-szr’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
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

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.