Problem/Motivation

This module depends on the external jQuery Validation library. This library cannot be committed to the Clientside Validation module repository because:

Currently, the Clientside Validation module's README recommends installing the jQuery Validation library by "extract[ing] the zip file to the js/lib of this sub module". However, is not recommended because:

  • Best practices for module upgrades dictate to delete the old module folder before the upgrade. This removes any libraries inside of that folder.
  • Multiple modules can implement the same library.
  • In this particular module, a .gitignore file makes it difficult to commit the jQuery Validation library to the project, which makes it difficult for site builders who use Git to manage files on a website, such as on Acquia Cloud and Pantheon, and some self-hosted setups.

Drupal core / the official Drupal 8 documentation does not specify where modules should be placed. However, it seems to have become convention (see the modules: leaflet, slick, blazy) to locate libraries relative to the Drupal root at /libraries/$library_machine_name. Note, however, this requires a change to the path specified for the library in the module's *.libraries.yml file: paths to library files must begin with /libraries/$library_machine_name/.

For more information, please refer to the *.libraries.yml files of the aforementioned contributed modules, and the documentation for LibraryDiscoveryParser::parseLibraryInfo().

Proposed resolution

Update the jquery.validate definition in clientside_validation_jquery/clientside_validation_jquery.libraries.yml to look like:

jquery.validate:
  js:
    /libraries/jquery-validation/dist/jquery.validate.js: {}

Modify the README to specify where to put the library. See the Leaflet module's README for inspiration.

Update the .gitignore file at the root of the project, and delete the clientside_validation_jquery/js/lib/.gitkeep file.

Remaining tasks

  1. Write a patch
  2. Feedback and review
  3. Feedback and RTBC
  4. Commit and release
  5. Update project page to specify where to put the library

User interface changes

None.

API changes

None.

However, it does change where the module expects the library to be, so it will break people's existing installations. This will almost certainly warrant a note in the module release notes, but there doesn't seem to be a convention on whether this would also warrant a new major or minor version number, a new hook_update_N() that displays a message, a message in the status report if the library is detected at its old location, a message displayed to administrators on every page load until the problem is fixed, or some combination of these. Feedback on the module maintainers' preference would be appreciated so that could be added to the patch!

Data model changes

None.

Comments

mparker17 created an issue. See original summary.

mparker17’s picture

Patch attached. Feedback welcome!

mparker17’s picture

Assigned: mparker17 » Unassigned
Status: Active » Needs review

Oops, forgot to set status and assigned fields after uploading patch.

nikunjkotecha’s picture

Adding condition for library to ensure both CDN and library both are not added.

Status: Needs review » Needs work
nikunjkotecha’s picture

StatusFileSize
new2.36 KB
new5.28 KB

Fixing build error.

nikunjkotecha’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work
nikunjkotecha’s picture

Not sure how to fix this error, will leave it to patch added in #4

nikunjkotecha’s picture

StatusFileSize
new3.79 KB

Use the same name as in current module's composer.json to allow hack in https://www.drupal.org/node/2784069#comment-11922633 work

nikunjkotecha’s picture

Updated even version to match what is there in module.

nikunjkotecha’s picture

nikunjkotecha’s picture

Status: Needs work » Needs review
nikunjkotecha’s picture

@attiks / @Jelle_S

Please confirm the problem and solution for this one.

nikunjkotecha’s picture

StatusFileSize
new3.79 KB

Fix issue in libraries.yml

jelle_s’s picture

Status: Needs review » Needs work
+++ b/clientside_validation_jquery/clientside_validation_jquery.module
@@ -46,7 +46,11 @@ function clientside_validation_jquery_clientside_validation_validator_info_alter
+    // Based on readme instructions for composer support, check in libraries.
+    if (file_exists('libraries/jqueryvalidate/dist/jquery.validate.js')) {
+      return;
+    }
+    elseif (!file_exists(drupal_get_path('module', 'clientside_validation_jquery') . '/js/lib/dist/jquery.validate.js')) {

This might fail on existing installations:

if file_exists('libraries/jqueryvalidate/dist/jquery.validate.js') returns FALSE (which it will for existing installations) and file_exists(drupal_get_path('module', 'clientside_validation_jquery') . '/js/lib/dist/jquery.validate.js') returns TRUE (which it will for existing installations that do not use the CDN), no js paths are altered and we'll try to load 'libraries/jqueryvalidate/dist/jquery.validate.js, which doesn't exist.

We might also want to add a hook_requirements so we can trigger a warning (maybe even error?) so that we can deprecate putting he library inside the module folder in the future?

dsdeiz’s picture

Status: Needs work » Needs review
StatusFileSize
new7.32 KB
new4.91 KB

Here's my take on the requests. Not sure if I got it right though.

nikunjkotecha’s picture

hi @dsdeiz, thanks for the updated patch. There is still a typo and now that we are fixing this, I would love to use the latest version in instructions. Will update patch and push soon.

nikunjkotecha’s picture

Status: Needs review » Closed (duplicate)
Related issues: +#2784069: Composer integration broken

-

nikunjkotecha’s picture

Status: Closed (duplicate) » Needs review
StatusFileSize
new8.2 KB
  • Updated README with latest version
  • Refactored code in hook_library_info_alter to support all scenarios
  • Updated CDN version used to latest one
  • Implemented (updated implementation in previous patch) hook_requirements() to show warning if JS not added in libraries (either using CDN or from module) with reference to readme file to reduce duplication of instructions

  • nikunjkotecha committed 9f5c6d6 on 8.x-1.x
    Issue #2895666 by nikunjkotecha, dsdeiz, mparker17, Jelle_S: Allow...
nikunjkotecha’s picture

Status: Fixed » Closed (fixed)

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