Libraries registration does not match folder structure

Yes is now my focus to update that :). I purpose a patch fast to solve it thank a lot @ckaotik

I'm changing priority of this feature request because using Composer to download Cropper library the code comes within /dist, so nothing works with actual DEV.

Please, consider this ASAP because Composer workflow to manage D8 websites is largely diffuse now.

Thank you very much!

I change that into bug because that is verry prior issue to purpose an new release of module ! I fix that Today can you stand ready to test it fastly ?

@MXT for using composer to downloading cropper can you purpose a patch ?

This issue is the last blocker before release. @ckaotik have you a small moment to view this with me ? :)

I ve begin tp create composer.json file:

{
  "name": "drupal/image_widget_crop",
  "description": "Provides an interface for using the features of the Crop API.",
  "type": "drupal-module",
  "homepage": "https://www.drupal.org/project/image_widget_crop",
  "support": {
    "issues": "https://www.drupal.org/project/issues/image_widget_crop",
    "source": "https://www.drupal.org/project/image_widget_crop"
  },
  "repositories": [
    {
      "type": "composer",
      "url": "https://packagist.drupal-composer.org"
    }
  ],
  "authors": [
    {
      "name": "Alexandre Mallet",
      "homepage": "https://www.drupal.org/u/woprrr"
    }
  ],
  "require": {
    "drupal/crop": "~8.1"
  }
  "license": "GPL-2.0+"
}

That patch prevent all cases with update_n for old user of libraries. FileSystem component in Symfony basis are powerfull for manipulate files / folders.

I ve process an helper in update_n because two cases possible with libraries for location of files and i prefer an approach more solid and detest code duplication :P

Can you test it and tell me if all is okay for u ? For me all works fine in all cases

I ve just an problem to stop Update_n when exception fired (in extrem cases if we have an files with name modified i think we can't prevent all exotic cases in this update_n because that introduce an too complex logic).

No, this is not OK.

You can't move files, that's not your job in an update function. If it has to change, then you need to document it instead.

Many people today deploy to containers and other read-only file systems. They use tools to download assets and so on.

This seems like a problematic change for a stable update. Maybe there's a way to support both?

@berdir i see your opinion, and I agree that. It better to inform users using libraries to move her files manually. You think is better to add a notice in report page ?

If you take abstraction of .install all work for you when you have /libraries/cropper/dist/* ?

I Re-patch fast when I have your opinion about report notice :)

Thank a lot for you fast feedback berdir I am glad to see you in iwc issues :)

I ve re-patch, but can you confirm me if these start of solution feel like for you ? I add an requirment to verify and prevent user to change her configuration.

I purpose that :) is more precise & clear.

1. +++ b/composer.json
   @@ -0,0 +1,20 @@
   +  "require": {
   +    "drupal/crop": "~8.1"
   +  },
   

   Specifying a version is problematic, see https://www.drupal.org/node/2817789#comment-11730919.

   People using the official d.o packages have version numbers like 1.0.0, not 8.1.0. So this won't install for them. Just put a * there or try specifying boh versions, not sure how well that would work.

2. +++ b/image_widget_crop.install
   @@ -0,0 +1,66 @@
   +  if (\Drupal::moduleHandler()->moduleExists('libraries') && ($info = libraries_detect('cropper'))
   +  ) {
   

   ") {" should be on the same line as the if

3. +++ b/image_widget_crop.install
   @@ -0,0 +1,66 @@
   +    elseif (!$is_local) {
   +      $result = \Drupal::httpClient()->request('GET', $file);
   +      if ($result->getStatusCode() != 200) {
   +        $error[] = t("<b>:type</b> file : The provided CDN path file path not found.", [':type' => strtoupper($type)]);
   +      }
   +    }
   

   I'm not sure if doing this request is worth it. If people put a wrong path there, then they should see a 404 entry in their logs. No strong opinion, fine if you'd prefer to have this. Maybe only do a HEAD request though, to avoid downloading the file?

4. +++ b/image_widget_crop.install
   @@ -0,0 +1,66 @@
   +    $requirements['iwc_libraries']['description'] = t('ImageWidgetCrop libraries files are correctly configured to use <b>:library</b> files', [
   

   I think <strong> should be used instead of <b> these days?

5. +++ b/image_widget_crop.module
   @@ -105,6 +105,7 @@ function image_widget_crop_libraries_info() {
          'download url' => 'https://cdnjs.com/libraries/cropper',
   +      'library path' => 'libraries/cropper/dist',
          'version arguments' => [
   

   why specify this? according to http://www.drupalcontrib.org/api/drupal/contributions!libraries!librarie..., this shouldn't be used, as other locations (like profiles/someting/libraries/cropper) wouldn't work anymore?

   I think you want to specify dist/some_file.. instead later in the code, for the actual files?

That said, I still think you should consider doing a check for the file with and without dist/ for backwards compatible. People don't expect that things change in a stable module update. At the very least, you should put a big warning into the release notes.

Thank a lot again @berdir for your full feedback :) I re-patch with your feedback, yes you right, I think I have too much more complex in the Previous patch to try to cover too many cases as possible. Nothing beats a nice line in the description of the release;). About backward compatibility part, I totally agree with you !! so I leave the options you have to be able to function with the old structure.

For the first point : Yes you right !!! I was still on the old version on packagist-drupal, thanks to you I am now aware

2. Yes !! fixed
3. Yes you right too in this case it's not really revelant to test it and increase complexity, i delete this unused part.
4. I ve found the two cases on core, but i can switch easy to Fixed.
5. Yes ! wrong way for me i ve read more doccumentation about libraries and patch it ! I see to maintain backward compatibility with old / new possible path.