In #2625026: Crop widget crop list not following common patterns we introduced the usage of Cropper as a library. We added a lot of code for handling this mostly for error handling. We should review this code an clean it up.

Comments

Lukas von Blarer created an issue. See original summary.

woprrr’s picture

Version: » 8.x-1.x-dev
Status: Active » Needs work
Issue tags: +D8Media

Now we can start the review of this issue :) Thanks @lukas. We can specify all taks we needed on this refactor ?

luksak’s picture

Since @sasanikolic wrote the code for handling libraries, he would be the guy to talk to. The main issue was how the libraries module works currently.

miro_dietiker’s picture

@sasanikolic is just about completing his internship time at MD Systems and needs to focus on other projects issues.

luksak’s picture

Ok. I remember that @sasanikolic wrote a lot of code for handling one CSS and one JS file. This logic should be handled by libraries. Here is the code we are talking about:

image_widget_crop.module:

/**
 * Implements hook_library_info_alter().
 */
function image_widget_crop_library_info_alter(&$libraries, $extension) {
  if ($extension != 'image_widget_crop') {
    return;
  }

  $config = \Drupal::config('image_widget_crop.settings');
  if (!\Drupal::moduleHandler()
      ->moduleExists('libraries') && !$config->get('settings.library_url') && !$config->get('settings.css_url')
  ) {
    $libraries['cropper.integration']['js'] = [];
  }

  // Get the correct path of the Cropper js file (the user needs to manually
  // put the jquery.cropper.min.js in libraries/cropper folder or set the url
  // in the settings).
  if ($library_url = $config->get('settings.library_url')) {
    // Cloud hosted library, use external JavaScript.
    $libraries['cropper']['js'][$library_url] = [
      'type' => 'external',
      'minified' => TRUE,
    ];
  }
  elseif (\Drupal::moduleHandler()->moduleExists('libraries')) {
    $info = libraries_detect('cropper');
    $libraries['cropper'] += [
      'version' => $info['installed'] ? $info['version'] : 'web-hosted',
    ];
    if ($info['installed']) {
      // Because the library is self hosted, use files from library definition.
      if (!empty($info['files']['js'])) {
        foreach ($info['files']['js'] as $data => $option) {

          if (is_numeric($data)) {
            $option = "/{$info['library path']}/{$option}";
          }
          elseif (empty($option['type']) || $option['type'] == 'file') {
            $data = "/{$info['library path']}/{$data}";
          }

          $libraries['cropper']['js'][$data] = $option;
        }
      }
    }
  }

  // Add the local CSS to the libraries.
  if ($css_url = $config->get('settings.css_url')) {
    // Cloud hosted library, use external CSS.
    $libraries['cropper']['css']['component'][$css_url] = [
      'type' => 'external',
      'minified' => TRUE,
    ];
  }
  elseif (\Drupal::moduleHandler()->moduleExists('libraries')) {
    $info = libraries_detect('cropper');
    $libraries['cropper'] += [
      'version' => $info['installed'] ? $info['version'] : 'web-hosted',
    ];
    if ($info['installed']) {
      // Because the library is self hosted, use files from library definition.
      if (!empty($info['files']['css'])) {
        foreach ($info['files']['css'] as $data => $option) {

          if (is_numeric($data)) {
            $option = "/{$info['library path']}/{$option}";
          }
          elseif (empty($option['type']) || $option['type'] == 'file') {
            $data = "/{$info['library path']}/{$data}";
          }

          $libraries['cropper']['css']['theme'][$data] = $option;
        }
      }
    }
  }
}

src/Form/CropWidgetForm.php:

  /**
   * Validation for cropper library.
   *
   * @param array $form
   *   An associative array containing the structure of the form.
   * @param \Drupal\Core\Form\FormStateInterface $form_state
   *   The current state of the form.
   */
  public function validateForm(array &$form, FormStateInterface $form_state) {
    parent::validateForm($form, $form_state);
    // TODO: Change the autogenerated stub.
    if (\Drupal::moduleHandler()->moduleExists('libraries')) {
      $directory = libraries_get_path('cropper') . '/dist/';
      $library = 'cropper.min.js';
      $css = 'cropper.min.css';
      if (!file_exists($directory . $library) || !file_exists($directory . $css)) {
        $form_state->setErrorByName('plugin', t('Either the library file or the CSS file is not present in the directory %directory.', array(
          '%directory' => '/' . $directory,
        )));
      }
    }
    else {
      if (empty($form_state->getValue('library_url')) || empty($form_state->getValue('css_url'))) {
        $form_state->setErrorByName('plugin', t('Either set the library and CSS locally and enable the libraries module or enter the remote URLs below. Check the README.md file for more information.'));
      }
      $cropper_cdn_url = 'https://cdnjs.com/libraries/cropper';
      if (!empty($form_state->getValue('library_url'))) {
        // Check if the name of the library in the remote URL is as expected.
        $library_url = $form_state->getValue('library_url');
        if (parse_url($library_url, PHP_URL_HOST) && parse_url($library_url, PHP_URL_PATH)) {
          $js = pathinfo($library_url, PATHINFO_BASENAME);
          if (!preg_match('/^cropper\.min\.js$/', $js)) {
            $form_state->setErrorByName('plugin', t('The naming of the library is unexpected. Double check that this is the real Cropper library. The URL for the minimized version of the library can be found on <a href="@url">Cropper CDN</a>.', ['@url' => $cropper_cdn_url]), 'warning');
          }
        }
        else {
          $form_state->setErrorByName('plugin', t('The remote URL for the library is unexpected. Please, provide the correct URL to the minimized version of the library found on <a href="@url">Cropper CDN</a>.', ['@url' => $cropper_cdn_url]), 'error');
        }
      }
      elseif (!empty($form_state->getValue('css_url'))) {
        // Check if the name of the library in the remote URL is as expected.
        $css_url = $form_state->getValue('css_url');
        if (parse_url($css_url, PHP_URL_HOST) && parse_url($css_url, PHP_URL_PATH)) {
          $css = pathinfo($css_url, PATHINFO_BASENAME);
          if (!preg_match('/^cropper\.min\.css$/', $css)) {
            $form_state->setErrorByName('plugin', t('The naming of the CSS is unexpected. Double check that this is the real Cropper CSS file. The URL for the minimized version of the CSS fuke can be found on <a href="@url">Cropper CDN</a>.', ['@url' => $cropper_cdn_url]), 'warning');
          }
        }
        else {
          $form_state->setErrorByName('plugin', t('The remote URL for the CSS file is unexpected. Please, provide the correct URL to the minimized version of the CSS file found on <a href="@url">Cropper CDN</a>.', ['@url' => $cropper_cdn_url]), 'error');
        }
      }
    }
  }
ckaotik’s picture

It is also currently only possible to use the files from either a remote url (e.g. CDN) or the libraries module. Using local files without the libraries module, via relative paths such as `/libraries/cropper/cropper.min.js`, is not possible.

Since libraries has no D8 release, this is a dealbreaker for us since we must use local files due to privacy implications and want the config to work on all systems alike.

woprrr’s picture

Yes ckaotik, i aggree with u ! We need to prevent all cases. What you suggest about it ?

"An module interface with checkbox 'CDN & EXTERNAL URL' and 'Local respository & Library module' to choose the method."

miro_dietiker’s picture

IMHO it's not worth adding more complexity to support custom stuff without the libraries module.
Instead we should help make sure the libraries module is provide a stable release.
We could not afford maintaining library code duplication in all the modules where we build of libraries.

But sure, we should minimise code complexity as much as possible as proposed.

ckaotik’s picture

@miro_dietiker I agree on getting the libraries module up and running, but i find it important to either fully depend on libraries being present or retain full compatibility with core (in addition to optionally libraries).
The module does not fully commit to either approach at the moment ;)

@woprrr I don't think additional UI is necessary. Couldn't we just use a priority logic such as this:
1) is `libraries` present? If so, use that to determine files.
2) does `file_exists` work on the url? If so, use the local files.
3) can we `curl` the url? If so, use the remote files.
4) No success? Use the CDN files.

Using a local file path already fails on form validation parse_url($library_url, PHP_URL_HOST) && parse_url($library_url, PHP_URL_PATH) but might otherwise work. (haven't tested that)

miro_dietiker’s picture

@ckaotik We have many cases where we simply provide a trivial fallback if a soft dependency isn't met.

Almost no one is really depending to token. But the UX to browse tokens is greatly limited if you don't have it.

I don't see why we need to depend to libraries when we can provide a minimum fallback easily.
We could though display a warning that we strongly recommend the module in our settings.

ckaotik’s picture

I don't see why we need to depend to libraries when we can provide a minimum fallback easily.

@miro_dietiker This is why I also suggested keeping support for both variants: libraries and core, but add support for local files while we're at it.
I'll try to find some time to propose a patch, though it may be a few days.

Note that it is in general not a good idea to load libraries from a CDN; avoid this if possible. It introduces more points of failure both performance- and security-wise, requires more TCP/IP connections to be set up and usually is not in the browser cache anyway.
-- https://www.drupal.org/theme-guide/8/assets

These (+ privacy concerns) are the reasons why we use local files.

miro_dietiker’s picture

We are fully aware of the CDN disadvantages. We still add it to the modules intentionally.
It lowers the barriers and makes them work out of the box and is convenience for a demo setup and finally makes even testing easier.

Privacy concerns and closed / local networking is why the advanced setting exists.

I'm looking forward to your proposal, but keep in mind, the issue is about simplification of code, not adding more cases and complexity.
If it is going to add complexity, it should go into a separate issue.

ckaotik’s picture

Status: Needs work » Needs review
StatusFileSize
new13.39 KB

I've attached a patch that should both reduce complexity and at the same time clear up the file priorities. This variant also allows to provide paths to local files.

The priorities are as follows (I hope I've correctly identified and retained their order):
1) Explicitly configured files will always be used.
2) If Libraries API is available and the Cropper library is installed, use those files.
3) Otherwise, use the CDN.

There are two unexpected things I ran into:
First, I had to remove the warning in `src/Element/ImageCrop.php` as the changed code will always provide a library that can be used (as long as the last fallback, the CDN, works). This should not create any issues.
Second, the Cropper library registration in `hook_libraries_info` expects the files in the library root (e.g. `libraries/cropper/cropper.min.js`). This does not match the structure of the packaged files, which places the script & style sheets in the `/dist/` sub-folder. As this also effects backward compatibility for users that placed the files in the expected path, I have not yet addressed this issue.

ckaotik’s picture

StatusFileSize
new13.34 KB

Fixed a minor inconsistency in detection of local paths.

ckaotik’s picture

StatusFileSize
new13.33 KB

Damnit, it's too warm here. Third time's the charm.

woprrr’s picture

Thank a lot for that patch :) !! I review it fast when i come back to my mission (thuesday).

miro_dietiker’s picture

How much test coverage do we have for these settings and error message cases?
IMHO it's an important piece to setup with enough complexity to argue all major cases should be covered. Otherwise our tests do not guarantee that all of the relevant cases are working cleanly.

ckaotik’s picture

I would feel a lot safer with accurate tests, as the code changes are far from minor. To my shame I have to admit I've not yet worked with tests, so I can't help with that, sorry.

woprrr’s picture

Assigned: Unassigned » woprrr

Don't worry @ckaotik, i can finish this part (test). It not shame you patch is already an beautifull job ! I m glad to see your help.

@miro it's true DO add test coverage for this part ! I assign to me this part ;)

The last submitted patch, 13: clean_up_libraries-2631732-13.patch, failed testing.

The last submitted patch, 14: clean_up_libraries-2631732-14.patch, failed testing.

The last submitted patch, 14: clean_up_libraries-2631732-14.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 15: clean_up_libraries-2631732-15.patch, failed testing.

The last submitted patch, 15: clean_up_libraries-2631732-15.patch, failed testing.

The last submitted patch, 13: clean_up_libraries-2631732-13.patch, failed testing.

ckaotik’s picture

Status: Needs work » Needs review
StatusFileSize
new13.32 KB

Fixed typo ($csss instead of $css) causing test fails. No interdiff because it's a minor change,

woprrr’s picture

Status: Needs review » Reviewed & tested by the community

I switch to RTBC :) works a charm thanks all. To Tests part i decide to make this in a major issue a part assign to me.

  • woprrr committed 28bc66a on 8.x-1.x authored by ckaotik
    Issue #2631732 by ckaotik: Clean up libraries validation code
    
woprrr’s picture

Status: Reviewed & tested by the community » Fixed

Status: Fixed » Closed (fixed)

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