Remove drupal_add_css() and drupal_add_js() from color.module — use #attached [#2010636]

Comment	File	Size	Author
#12	2010636-12-color_module.patch	1.35 KB	adammalone

#9	2010636-9.patch	1.25 KB	Wim Leers

#2	2010636-remove-drupal_add_.patch	986 bytes	mcjim

Comment #1

mcjim CreditAttribution: mcjim commented 3 June 2013 at 10:44

Title:

Remove drupal_add_css() from color.module — use #attached

» Remove drupal_add_css() and drupal_add_js() from color.module — use #attached

Log in or register to post comments

Comment #2

mcjim CreditAttribution: mcjim commented 3 June 2013 at 10:47

Status:

Active

» Needs review

File	Size
2010636-remove-drupal_add_.patch	986 bytes

There was a comment in the code to use drupal_add_library, but I've just use #attached.

Log in or register to post comments

Comment #3

tstoeckler

he/him

German

Essen, Germany

CreditAttribution: tstoeckler commented 3 June 2013 at 11:35

Status:

Needs review

» Needs work

Please let's follow the best practice and use drupal_add_library().
Implement hook_library_info() and add the JS and CSS there. I can give you more specific instructions, in case you need any help.

Log in or register to post comments

Comment #4

mcjim CreditAttribution: mcjim commented 3 June 2013 at 12:10

Specific instructions would be welcome: the CSS/JS being added seems specific to the theme settings form rather than a general library? I may be misunderstanding something.

Log in or register to post comments

Comment #5

mcjim CreditAttribution: mcjim commented 3 June 2013 at 14:22

Status:

Needs work

» Needs review

This is an odd one for sure.

color_library_info() already adds preview.js. The code here is checking to see if there is a preview_js file in the theme and adding that instead of colour/preview.js…

The CSS added is from the theme, also, so neither of these really count as libraries.

So, while this solution might not be perfect, it does help get rid of another call to drupal_add_css/drupal_add_js for now (see #1839338: [meta] Remove drupal_set_*() drupal_add_*() in favour of #attached/response for out-of-band stuff)

Re-setting to needs review to let the test bot have a go, but feel free to change if there is a better approach.

Log in or register to post comments

Comment #6

tstoeckler

he/him

German

Essen, Germany

CreditAttribution: tstoeckler commented 3 June 2013 at 15:04

Status:

Needs review

» Reviewed & tested by the community

Ahh, you're totally right. That approach should totally be improved in 1000 different ways, but that's not for this issue, and as it stands using a library is not possible/sensible in this case.

Thanks for sticking with this and setting me straight!

Log in or register to post comments

Comment #7

catch

he/him

English

CreditAttribution: catch commented 9 June 2013 at 20:55

#2: 2010636-remove-drupal_add_.patch queued for re-testing.

Log in or register to post comments

Comment #8

9 June 2013 at 20:56

Status:

Reviewed & tested by the community

» Needs work

The last submitted patch, 2010636-remove-drupal_add_.patch, failed testing.

Log in or register to post comments

Comment #9

Wim Leers

Ghent 🇧🇪🇪🇺

CreditAttribution: Wim Leers commented 11 July 2013 at 17:54

Assigned:	Unassigned	» Wim Leers
Status:	Needs work	» Postponed
Issue tags:		+D8 cacheability

File	Size
2010636-9.patch	1.25 KB

Straight reroll.

We can't test this because theme-specific settings are currently broken in D8. #2040037: The requested page "/admin/appearance/settings/bartik" could not be found after installation

Log in or register to post comments

Comment #10

Wim Leers

Ghent 🇧🇪🇪🇺

CreditAttribution: Wim Leers commented 11 July 2013 at 18:21

Status:

Postponed

» Needs review

#9 is broken. The CSS and JS no longer ends up on the page. I strongly suspect the root cause for this are the recent changes to the (pre)process layer being removed/working differently and hence #attached assets in that phase no longer working… but at the same time #2004286: Defer calls to drupal_get_* functions until printed inside a template by adding a RenderWrapper class has not yet been committed, so I'm not entirely sure. Needs debugging.

NR because it *is* possible to test the patch, you just have to clear your caches after installing current D8 head freshly — see #2040037: The requested page "/admin/appearance/settings/bartik" could not be found after installation for details.