backdrop / backdrop-issues

Issue tracker for Backdrop core.
144 stars 38 forks source link

[PS] jQuery and Backdrop JavaScript libraries and settings always output #3669

Open klonos opened 5 years ago

klonos commented 5 years ago

This is the same as https://www.drupal.org/project/drupal/issues/1279226 (change record for 7.36: https://www.drupal.org/node/2462717), which makes this issue a feature regression.

Description: Before Drupal 7.36, Drupal 7 always added jQuery and other related JavaScript libraries to all pages, even when they were not being used. For example, an anonymous user visiting the front page of a fresh Drupal installation would have these files loaded, even though no actual JavaScript runs on the front page.

In Drupal 7.36, an optional method was introduced to improve front-end performance by allowing sites and module authors to not force these libraries to be loaded on pages where they are not going to be used.

New javascript_always_use_jquery variable for sites to opt out of always loading the JavaScript libraries Sites can now set the new javascript_always_use_jquery variable to FALSE if they want to turn on the above-mentioned functionality. There is no user interface for setting this variable currently, but it can be done in various common ways, for example by putting this code in settings.php:

$conf['javascript_always_use_jquery'] = FALSE;

This variable should be used with caution, because it will cause jQuery and related libraries to only be loaded on pages where Drupal's JavaScript API was used to include JavaScript files that require them. This should be safe on sites that always use the JavaScript API to load JavaScript, but if the site includes JavaScript in other ways (for example, hardcoded <script> tags in the theme, or in node bodies in the database) and if the JavaScript included there relies on jQuery but doesn't load it explicitly, setting this variable could cause broken JavaScript on the site.

If the variable is not set, the current behavior of Drupal 7 will not change.

New 'requires_jquery' option in drupal_add_js() which can be used by developers to indicate that a JavaScript file does not require jQuery If you are a module or theme developer adding a JavaScript file which has no dependency on jQuery, it is recommended that you set the new 'requires_jquery' option to FALSE when adding the file via drupal_add_js(), $form['#attached']['js'], or similar methods. For example:

drupal_add_js('path/to/some/standalone-javascript-library.js', array('requires_jquery' => FALSE));

This will indicate to the JavaScript API that this file does not require adding jQuery to the page. (The default is to assume that it does.)

Be aware that if your JavaScript file uses any of Drupal's built-in JavaScript helper methods, such as the Drupal behaviors system or the Drupal.t() function, your code does require jQuery indirectly (even if you are not using jQuery methods directly yourself). This new option is primarily intended for JavaScript code written outside of Drupal, e.g. external libraries that do not rely on jQuery at all.

klonos commented 5 years ago

I've filed a PR and tried to do my best to convert the tests. I have used state_set()/state_del instead of variable_set()/variable_del(), since there is no way that I know of to "dynamically" write to the settings.php file (to check if $conf['javascript_always_use_jquery'] has been set). Please let me know if there is a better way.

I have tried to see if there are examples of $this->assertRaw('Backdrop.settings', 'some text') used in other tests, but could not find any. I hope that this works in Backdrop the same way that $this->assertRaw('Drupal.settings', 'some text') works.

klonos commented 5 years ago

Questions:

  1. Do we need to include $conf['javascript_always_use_jquery'] + respective documentation in our settings.php file? If yes, then should that be optional/commended-out, or set to the default $conf['javascript_always_use_jquery'] = TRUE;?

  2. Do we need an update hook to grab that variable from Drupal sites during upgrade to Backdrop, and convert it to config? Can/should that be handled in #3671 instead?

PS (re point 2 above): In the actual code in core/includes/common.inc, there is a conditional that uses settings_get('javascript_always_use_jquery', TRUE). With #3671, we'll need to add a config_get() for the same setting as well.

herbdool commented 5 years ago

@klonos I see your comments address similar issues as mine.

I think you should just store the variable in config file, system.core.json might be best. On a quick review I didn't see any reason it needs to be in settings.php for now, plus this makes it easier to expose in the UI.

klonos commented 5 months ago

I'll get back to this, since it would make cross-porting #6393 easier.