Problem/Motivation

JS Cookie is added as a dependency, but for existing installs it is not enabled during the update process.

Also, somehow the code seems to think it is a theme.

Steps to reproduce

  • Install a version before JS Cookie was introduced as a dependency
  • Update to the latest version
  • Try to open any page on the site
  • See these warnings:
    User warning: The following theme is missing from the file system: js_cookie in Drupal\Core\Extension\ExtensionPathResolver->getPathname() (line 63 of core/lib/Drupal/Core/Extension/ExtensionPathResolver.php).

    Deprecated function: dirname(): Passing null to parameter #1 ($path) of type string is deprecated in Drupal\Core\Extension\ExtensionPathResolver->getPath() (line 85 of core/lib/Drupal/Core/Extension/ExtensionPathResolver.php).

    User warning: The following theme is missing from the file system: js_cookie in Drupal\Core\Extension\ExtensionPathResolver->getPathname() (line 63 of core/lib/Drupal/Core/Extension/ExtensionPathResolver.php).

    Deprecated function: dirname(): Passing null to parameter #1 ($path) of type string is deprecated in Drupal\Core\Extension\ExtensionPathResolver->getPath() (line 85 of core/lib/Drupal/Core/Extension/ExtensionPathResolver.php).

Proposed resolution

Add an update hook that enables the module when it isn't.

Remaining tasks

  • Code changes
  • Create merge request
  • Code review
  • Functional review
  • Merge

User interface changes

None.

API changes

None.

Data model changes

None.

Original report

After upgrading, with php 8.2 I get the following warnings:

User warning: The following theme is missing from the file system: js_cookie in Drupal\Core\Extension\ExtensionPathResolver->getPathname() (line 63 of core/lib/Drupal/Core/Extension/ExtensionPathResolver.php).

Deprecated function: dirname(): Passing null to parameter #1 ($path) of type string is deprecated in Drupal\Core\Extension\ExtensionPathResolver->getPath() (line 85 of core/lib/Drupal/Core/Extension/ExtensionPathResolver.php).

User warning: The following theme is missing from the file system: js_cookie in Drupal\Core\Extension\ExtensionPathResolver->getPathname() (line 63 of core/lib/Drupal/Core/Extension/ExtensionPathResolver.php).

Deprecated function: dirname(): Passing null to parameter #1 ($path) of type string is deprecated in Drupal\Core\Extension\ExtensionPathResolver->getPath() (line 85 of core/lib/Drupal/Core/Extension/ExtensionPathResolver.php).

CommentFileSizeAuthor
#5 multiple_warnings_after_upgrading-3449275-5.patch1.14 KBahmetburkan

Issue fork autologout-3449275

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

doxigo created an issue. See original summary.

ahmetburkan’s picture

ahmetburkan
Primary language Dutch
Location Utrecht
commented

There is a workaround at the moment. Manually enabling the "js_cookie" contrib module.

doxigo’s picture

doxigo commented

Yup that works as a workaround, thanks

ahmetburkan opened merge request !59

ahmetburkan’s picture

ahmetburkan
Primary language Dutch
Location Utrecht
commented
Status: Active » Needs review
StatusFileSize
new multiple_warnings_after_upgrading-3449275-5.patch1.14 KB

Created merge request and patch. This is ready to be reviewed.

eelkeblok’s picture

eelkeblok
he/him
Primary language Dutch
Location Netherlands 🇳🇱
commented
Title: Multiple warnings after upgrading to 8.x-1.5 & 2.x with js_cookie » JS Cookie needs to be enabled automatically when updating
Issue summary: View changes

Updated title and issue summary

eelkeblok’s picture

eelkeblok
he/him
Primary language Dutch
Location Netherlands 🇳🇱
commented
Priority: Normal » Major
Issue summary: View changes

Code looks good. I also upgraded this to major. I haven't functionally tested the changes yet.

eelkeblok’s picture

eelkeblok
he/him
Primary language Dutch
Location Netherlands 🇳🇱
commented
Title: JS Cookie needs to be enabled automatically when updating » Multiple warnings after upgrading to 8.x-1.5 & 2.x with js_cookie
Issue summary: View changes
Status: Needs review » Needs work

I've just deployed the changes with the module enabled, but I still see these errors. One part I did not give much notice, but is really kind of weird, is that the error says that the theme js_cookie is not enabled...

eelkeblok’s picture

eelkeblok
he/him
Primary language Dutch
Location Netherlands 🇳🇱
commented
Title: Multiple warnings after upgrading to 8.x-1.5 & 2.x with js_cookie » JS Cookie needs to be enabled automatically when updating
Issue summary: View changes
Status: Needs work » Needs review

Sorry for the confusion. Something went wrong, errors are resolved after enabling the module. What remains is a functional test of the update hook.

mglaman’s picture

mglaman
Primary language English
Location WI, USA
commented

Came here to mention this. It was added in #3388601: Use GitLab CI for testing without its own specific issue to explain why.

mglaman’s picture

mglaman
Primary language English
Location WI, USA
commented
Status: Needs review » Needs work

Left a comment on the MR.

the_g_bomb made their first commit to this issue’s fork.

the_g_bomb opened merge request !61

the_g_bomb’s picture

the_g_bomb commented
Status: Needs work » Needs review

Create a new PR as the other one looks to be open against the 8.x-1.x branch.

Made the alteration suggested by @ericgsmith

herved’s picture

herved commented

Hi, would it be possible to remove the js-cookie dependency entirely?
I only realized now but the lib uses a CDN (both core/js-cookie and the contrib) and goes against our current CORS policy on several projects.
Looking at the autologout JS it looks rather easy/feasible but I wanted to ask here before creating a new issue.

Thanks

Correction:
core/js-cookie doesn't use a CDN but the contrib does by default. However we can make it local as per #3390616: Document using local library file, with CDN as fallback (Module violates GDPR compliance)
Still, I wonder if it would be worth removing the dependency?

ahmetburkan’s picture

ahmetburkan
Primary language Dutch
Location Utrecht
commented
Status: Needs review » Reviewed & tested by the community

I tested the functionality, and it works as expected! Updating the status.

As for the comment above, I think we should consider a new ticket. Because currently we have a bug, and the suggestion sounds more like a feature.

If we want to fix the way the dependency is loaded in this ticket, feel free to set the status back to "Needs work".

ericgsmith’s picture

ericgsmith commented

If we want to fix the way the dependency is loaded in this ticket, feel free to set the status back to "Needs work".

I think your suggestion to create a new ticket is correct - this is a bug fix for something that is already released and the MR fixes this - IMO lets not delay fixing this based on another option at this point. Sounds like a good discussion to move to a new issue

+1 for RTBC

the_g_bomb’s picture

the_g_bomb commented

+1 to remove the dependency, but I agree it should be a new ticket. Would be good to get this merged.

ahmetburkan’s picture

ahmetburkan
Primary language Dutch
Location Utrecht
commented

Is it possible to get this merged?

japerry made their first commit to this issue’s fork.

  • japerry committed 47213e6b on 2.x authored by the_g_bomb 
    Issue #3449275: JS Cookie needs to be enabled automatically when...
japerry’s picture

japerry
Location KOMK
commented
Status: Reviewed & tested by the community » Fixed

I'll merge this in to fix the immediate problem. the Contrib JS cookie was added because of its removal from core... however I'd need to look back and figure out where that code is being derived from in this module. Agree that its a separate ticket.

https://www.drupal.org/project/js_cookie/

  • japerry committed 47213e6b on 8.x-1.x authored by the_g_bomb 
    Issue #3449275: JS Cookie needs to be enabled automatically when...

Status: Fixed » Closed (fixed)

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

ressa’s picture

ressa
he/him
commented

@herved: +1 to investigate if removing the JS Cookie dependency is possible, and I created #3557620: Remove JS Cookie dependency.