Comments

catch’s picture

Priority: Normal » Critical
Wim Leers’s picture

nod_’s picture

Issue tags: +JavaScript

tag

hass’s picture

I've found an issue in Google Analytics and I have no clue how to solve if drupal_add_js() is going to be removed.

Are the designers of this idea able to help here, please?

/**
 * Implements hook_preprocess_search_results().
 *
 * Collects and adds the number of search results to the head.
 */
function google_analytics_preprocess_search_results(&$variables) {
  // There is no search result $variable available that hold the number of items
  // found. But the pager item mumber can tell the number of search results.
  global $pager_total_items;

  drupal_add_js('window.googleanalytics_search_results = ' . intval($pager_total_items[0]) . ';', array('type' => 'inline', 'group' => JS_LIBRARY-1));
  // Don't work:
  $variables['#attached']['js']['window.googleanalytics_search_results = ' . intval($pager_total_items[0]) . ';'] = array('type' => 'inline', 'group' => JS_LIBRARY-1);
}
hass’s picture

Adding Contributed project blocker as a removed drupal_add_js() will block a Google Analytics release.

xjm’s picture

Issue tags: +Prague Hard Problems
nod_’s picture

$variables['#attached']['js']['window.googleanalytics_search_results = ' . intval($pager_total_items[0]) . ';'] = array('type' => 'inline', 'group' => JS_LIBRARY-1);

should be

$variables['#attached']['js'][] = array(
  'data' => 'window.googleanalytics_search_results = ' . intval($pager_total_items[0]) . ';',
  'type' => 'inline', 
  'group' => JS_LIBRARY-1,
);

And that should work. We have to keep a way to do this because that's how drupalSettings is added to the page. So I wouldn't worry about this use-case.

and if it doesn't, then it means $variables isn't a render array, and #attached is used on a render array so it might be the wrong hook to put that in.

hass’s picture

That is the same like your array...

and if it doesn't, then it means $variables isn't a render array, and #attached is used on a render array so it might be the wrong hook to put that in.

This is the only workaround-hook where I'm able to grab the required information I need as the required hooks are missing in core. If you like it or not... this will not work as this $variables is not a render array as I know. It looks like core need to add the appropiate search hooks or drupal_add_js() cannot removed.

catch’s picture

Adding more search hooks is fine, please open an issue for that.

As a workaround in preprocess you can set up an #attached array with the js/css/library and call drupal_render() on it. That doesn't fix the root problem (still relies on global state) but gives us at least a central place to fix things. instead of five.

Wim Leers’s picture

As nod_ said in #7, you're using #attached incorrectly in #4, so this is not true:

That is the same like your array...

hass’s picture

@nod_: thx. Gave you the credit for 3 other changes at http://drupalcode.org/project/google_analytics.git/commit/1701fa1

@catch: Cannot get it working...

  drupal_render($variables['#attached']['js'][] = array(
    'data' => 'window.googleanalytics_search_results = ' . intval($pager_total_items[0]) . ';',
    'type' => 'inline',
    'group' => JS_LIBRARY-1,
  ));

gives me:

Strict warning: Only variables should be passed by reference in google_analytics_preprocess_search_results() (line 421 of modules\google_analytics\google_analytics.module).
User error: "data" is an invalid render array key in element_children() (line 4535 of core\includes\common.inc).
User error: "type" is an invalid render array key in element_children() (line 4535 of core\includes\common.inc).
User error: "group" is an invalid render array key in element_children() (line 4535 of core\includes\common.inc).
catch’s picture

That's a normal PHP error. Set up the variable first, then pass it to drupal_render(). Also you don't need to use $variables here can just be its own thing.

nod_’s picture

Thanks :þ

Attach first, then do a drupal_render($variables). drupal_render will look for an #attached key.

(edit) yeah like catch says.

hass’s picture

Finally done. http://drupalcode.org/project/google_analytics.git/commit/6798d24 thank you all.

Opened two cases to get rid of this wonky code at all:

Maybe you like to join :-)

hass’s picture

Issue summary: View changes

Updated issue summary.

xjm’s picture

Issue summary: View changes
Issue tags: +beta blocker
catch’s picture

Issue summary: View changes
joelpittet’s picture

Issue summary: View changes
joelpittet’s picture

Issue summary: View changes
vijaycs85’s picture

Issue summary: View changes
MustangGB’s picture

Issue summary: View changes
Wim Leers’s picture

Issue summary: View changes

The work that needed to be done in #2160577: Remove drupal_add_js() + drupal_add_library() from batch.inc was done in another issue, so that's already taken care of.

I reviewed all other issues; most have been RTBC'd.

Much thanks to vijaycs85 for tackling most of them!

Wim Leers’s picture

Issue summary: View changes
Wim Leers’s picture

Wim Leers’s picture

Issue summary: View changes

#2160581: Remove drupal_add_js() + drupal_add_library() from install.core.inc was also committed just now. That leaves just one: #2160593: Remove drupal_add_js() from menu.module/remove inline JavaScript from menu.module Assigned to: Wim Leers, which is blocked on approval by nod_. Almost there!

webchick’s picture

Title: [META] Remove drupal_add_js() » [META] Remove drupal_add_js() calls

I just committed the last of these. :) WOOHOO!!

Now we need to do something to signal to people it's for internal use only. Prefix with an underscore, mark @deprecated?

catch’s picture

We're in the same position with drupal_add_css(). There's a bit of discussion on what to do on https://drupal.org/comment/8311981#comment-8311981 downwards.

Might want to open a new issue to do whatever we end up doing to both of these functions, and close these metas as fixed?

Wim Leers’s picture

#27: that sounds good :)

catch’s picture

vijaycs85’s picture

Issue summary: View changes

Status: Fixed » Closed (fixed)

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

webchick’s picture

Here is some developer feedback from a themer actively trying to use this to add in an external JS library to a theme: #2298551: Documentation updates adding/adjusting inline/external JS in themes Would love suggestions there on how we could've made this not an hours-long task, but still retain the benefits of what these initiatives are trying to do.

corbacho’s picture

Asking also for best practices here, maybe it's helpful for others.

I'm wondering why this stopped working:

$js_attached['#attached']['drupalSettings']['color']['logo'] = theme_get_setting('logo.url', 'bartik');
drupal_render($js_attached);

Replacing drupal_render for drupal_process_attached works, but is the best approach? #2396983: Header Logo with Bartik won't change in settings preview