Problem/Motivation

After upgrading to version 8.x-2.4 the error message below started printing on any page with a carousel:

Warning: file_get_contents(/REDACTED/docroot/libraries/slick/package.json): failed to open stream: No such file or directory in Drupal\slick\SlickSkinManager->isBreaking() (line 432 of modules/contrib/slick/src/SlickSkinManager.php).

From what I can tell the module is trying to read the version of the JS library from the package.json file.

This is a problem because in order to prevent supply chain attacks we automatically remove any package.json files from JS libraries during our deployment process.

Steps to reproduce

1. Enable the module and install the JS library.
2. Remove the package.json file form the JS library folder.
3. Add a Slick carousel to a page.
4. Clear cache.
5. The error should be visible.

Proposed resolution

Add a fallback catch to not throw an error if the package.json file cannot be read. Only throw the rror if the file is read, but the version is not correct.

Remaining tasks

Decide on fix and implement.

User interface changes

None

API changes

None

Data model changes

None

CommentFileSizeAuthor
#6 3246544-6-error-package-json.patch1.24 KBtanmayk

Comments

PCate created an issue. See original summary.

gausarts’s picture

Thank you.

The second by-product reported issue of the new awesomess: #3196529: Support Accessible Slick

The responsible method:
https://git.drupalcode.org/project/slick/-/blob/8.x-2.4/src/SlickSkinMan...

With the @todo to remove it:

// The master reverted from 1.8.1 - 1.9.0 to 1.8.0. This is for old
        // downloads. See https://github.com/kenwheeler/slick/pull/3688
        // @todo Remove after another check.

So, the best solution is we should remove it.
We had enough babysitting codes around. This new questionable one should be gotten rid.

Instead we had provided relevant warnings at project home requirements section.

Patches are welcome. Thanks.

joelpittet’s picture

Version: 8.x-2.4 » 8.x-2.x-dev

Yeah that is very unfortunate, npm is still reporting 1.8.1 as the latest in the ^1.0.0 releases. We are copying the library with npm's copy-files-from-to package but were excluding the top directory contents because none of it was executable code. Thanks for reporting this, the proposed solution is good, but won't help with this issue with the 1.8.1. Can we parse the slick.js file comment of the version number or is that silly?

joelpittet’s picture

I like the removing the code entirely as @gausarts suggests in #2.

gausarts’s picture

> Can we parse the slick.js file comment of the version number
Unfortunately, unlike package.json, versions at this file are inconsistent, so no:
https://github.com/kenwheeler/slick/blob/1.8.0/slick/slick.js
https://github.com/kenwheeler/slick/blob/v1.8.1/slick/slick.js

These two files have 1.8.0 as version, likely due to being hard-coded.. The package.json is strictly correct.

The problem is identified here as per deletion: https://github.com/kenwheeler/slick/pull/3688 and still present at v1.8.1.
Perhaps another reason why the recommended release has no v.1.8.1:
https://github.com/kenwheeler/slick/releases
The Latest marked as release is 1.8.0, not v.1.8.1.

It is starting to become a concern when #3196529: Support Accessible Slick was in, it inherits the troublesome v1.8.1 which cause confusing breakages.

>removing the code entirely
Only removing the else branch:
https://git.drupalcode.org/project/slick/-/blob/8.x-2.4/src/SlickSkinMan...
as OP reported.

The breaking method is still kept to minimize sudden breakages as mentioned above. This particular babysitter is still needed to minimize extra works due to issues between 1.8.0 vs 1.8.1 which severely break displays.
Unless we had time to re-update each optionset manually when changing from Accessible to regular Slick, and vice-versa.

tanmayk’s picture

Status: Active » Needs review
StatusFileSize
new1.24 KB

Patch against 2.x branch as per comments from @gausarts.

pcate’s picture

Status: Needs review » Reviewed & tested by the community

Patch #6 applied cleanly and fixed issue for me.

  • gausarts committed 7bc30a7 on 8.x-2.x authored by tanmayk
    Issue #3246544 by tanmayk, PCate, joelpittet: Error displayed if package...
gausarts’s picture

Status: Reviewed & tested by the community » Fixed

Committed. Thank you for contribution and patience.

Status: Fixed » Closed (fixed)

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