Problem/Motivation

jQuery Cookie 1.4.1 is no longer in development, has vulnerabilities and widespread usage in D7 community as a core requirement. JS-Cookie is the replacement used by Drupal 9 with its a shim to translate functions between the two. Lets enable JS-Cookie using this module by providing the shim and admin updates to supply the JS-Cookie path when desired.

Steps to reproduce

Without this patch you will discover console errors with "$.cookie is not a function", thus stopping JS from continuing.

Proposed resolution

Add a JS-Cookie shim to this module that is used when a js.cookie url is used. Also admin updates to help get JS-Cookie visible as an option.

Remaining tasks

  • Testing of Shim usage as this was stolen directly from Drupal 9.5.10

User interface changes

  • Admin descriptions that include jQuery Cookie and JS-Cookie URLs

API changes

  • JS-Cookie version checking and URL's in descriptions
  • Autoloading of Drupal 9.5.10 js.cookie.shim.js

Data model changes

CommentFileSizeAuthor
#5 js-cookie-shim-3382000-5.patch11 KBwhthat
#2 js-cookie-shim-3382000.patch8.65 KBwhthat

Issue fork jquery_update-3382000

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

About issue forks

Comments

whthat created an issue. See original summary.

whthat’s picture

whthat commented
Status: Active » Needs review
StatusFileSize
new js-cookie-shim-3382000.patch8.65 KB

Status: Needs review » Needs work

The last submitted patch, 2: js-cookie-shim-3382000.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

whthat’s picture

whthat commented
Issue summary: View changes
whthat’s picture

whthat commented
StatusFileSize
new js-cookie-shim-3382000-5.patch11 KB

With updated tests as descriptions were changed on Cookie path

whthat’s picture

whthat commented
Status: Needs work » Needs review
whthat’s picture

whthat commented
Issue summary: View changes

whthat opened merge request !7

mcdruid’s picture

mcdruid commented
Status: Needs review » Needs work

Interesting suggestion, thanks.

I think this looks good.

I'd like to see a couple of tweaks:

'#description' => t('Example: %url or %path or in early testing js.cookie and D9 shim with %jscookie', array(
  • Let's drop "early testing"; I'd be more comfortable saying "experimental"?
  • Can we provide a link to explain more about the shim - perhaps to: https://www.drupal.org/node/3104677
  • I am a bit nervous about this:
    if (strpos( $custom_path,  'js-cookie')){
      // if js.cookie is being used to replace jquery.cookie 1.4.1, then use this shim from a Drupal 9.5.10 environment
      $libraries['cookie']['js']['misc/jquery.cookie.shim.js']['data'] = $path . '/replace/ui/external/jquery.cookie.shim.js';
    }

(nit - there's an extra space before the variable within the if condition.)

It's probably okay to check for this string in the custom path, but that could easily go wrong e.g. if someone stores a local copy of the library and renames it without that exact pattern.

If we're going to rely on the presence of that string, let's be explicit about it (in the admin UI), or let's add a checkbox instead to indicate that the shim should be included... that might also give us some space in the admin UI to better explain what this is all about.

Finally, it'd be great to see some additional testing of this option being used.

prabuela’s picture

prabuela commented
Assigned: Unassigned » prabuela
prabuela’s picture

prabuela commented
Assigned: prabuela » Unassigned
whthat’s picture

whthat commented
Status: Needs work » Needs review

Quick update based on @mcdruid #9. Further explanation of this experimental option, path requirements and link to Drupal 9 Shim. Happy to go deeper with a checkbox in admin interface, if necessary.

Agreed please test! I have only Drupal core based cookies needs, so any custom/module based cookie code would be ideal.

poker10’s picture

poker10 commented
Status: Needs review » Needs work

Currently the MR is failing tests, so I think we need to fix that failure first. Thanks!

whthat’s picture

whthat commented

I need help modifying that test...changed description of field and tried updating, but something else is needed.

mcdruid’s picture

mcdruid commented

The MR still has the "in early testing" wording in the admin UI (see #9) and IMHO putting *** into the admin text for emphasis looks quite messy. Let's try to keep everything as clean and simple as possible.

We also mentioned adding a checkbox for the shim rather than relying on detecting a string in the custom path.

Finally... should the shim still be based on Drupal 9.5? Is it still in D11? If so, can we update the shim and any references, or explain why it's specifically a D9 shim we're including if there's a reason for it to be "locked" etc..

mcdruid’s picture

mcdruid commented
Issue tags: +Needs tests

There are a few things to address in the comments, plus needs tests.

mcdruid’s picture

mcdruid commented

I had a bit of time to tidy this up, but I'd still like to see some new tests added to verify that e.g. the checkbox / variable for the shim works as it's supposed to.

So leaving at NW + Needs tests for now.