Hi all,
without insert_colorbox, when I'm inserting images, then no target links are created, based on my image widget settings.
But with insert_colorbox enabled, target links are always created.
I think in insert_colorbox_insert_variables() there should be check if $vars['style_name'] belongs to the colorbox, then colorbox_insert settings should be applied, or not - then image widget settings should be applied.

Here is my attempt to fix it:

  $styleName = null;

  if (strpos($vars['style_name'], 'colorbox__') !== false) {
      $styleName = $config->get('style');
      if ($styleName == 'image') {
          $vars['url_link'] = $vars['url_original'];
      } else {
          $vars['url_link'] = InsertUtility::buildDerivativeUrl(
              $vars['file'],
              $styleName,
              \Drupal::config('insert.config')->get('absolute')
          );
      }
  } else {
      if ($insertSettings['link_image'] == 0) {
          $vars['url_link'] = '';
      } elseif ($insertSettings['link_image'] == 'image') {
          $vars['url_link'] = $vars['url_original'];
      } else {
          $vars['url_link'] = InsertUtility::buildDerivativeUrl(
              $vars['file'],
              $insertSettings['link_image'],
              \Drupal::config('insert.config')->get('absolute')
          );
      }
  }

 

Also I think it is confusing - at least for me - to see some duplicated settings:

  • in insert_colorbox settings (/admin/config/content/insert) I would remove option 'use “Link image to” widget setting'
  • and in the image widget settings I don't understand why there is "AUTOMATIC" option, because it includes same options which are available by default for insert settings

Thanks

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Bohus Ulrych created an issue. See original summary.

Snater’s picture

Thanks for the issue ticket. Just had a quick look, but I'm gonna have to take a deeper look into the code for that. As far as I remember, this section was more or less ported from the Drupal 7 colorbox module.

Snater’s picture

Category: Bug report » Task
Status: Active » Needs review
FileSize
739 bytes

Thanks again for opening the issue. I had a bit of a look at that. As for the settings: /admin/config/content/insert are global settings. Applying use “Link image to” widget setting allows specifying the Colorbox image style per image/file widget.
The AUTOMATIC option makes, in general, most sense on file widgets when content is inserted according to the content type of the file (i.e. files are inserted as links, images are inserted in the style specified in that drop-down you are confused about. There might still be a little reason to have this option for image fields as well to ensure content editors that, by using the AUTOMATIC option, the image would always be inserted in a proper way—a fallback/failsafe option so to say—no need to worry about the sie or whatever. Maybe documentation could be improved a little bit on that, but since these are administration settings, I am pretty sure, whoever admin is interested in such options, does figure it out.

As for the code change. I don't see it as a bug in general, as having the derivative URLs is not a bad thing per se, since the derivatives are created only when the URL is actually accessed. So, as I see it, nothing is broken in that sense. There could be a bit of performance optimization though. I did not touch the code for a while, but I guess, since this part of the code is only supposed to affect colorbox images, we could probably just skip it for non-colorbox styles as to the attached change set. Regular handling of images is already done in the main insert module.

Bohus Ulrych’s picture

Hi, I'm refreshing this topic.
Thank you @Snater for explanation.

With your patch it works much better. For my image field I set "Link image to: do not link", and in the Insert module config page I set "INSERT COLORBOX >> Image style: Large".
Then in the node when I insert picture with style "Medium", only image is created - no link behind.
And inserting style "Colorbox medium" inserts code for image with link to Large image style.

This means that I can set colorbox image style only globally, because if I choose any image style for the field (Link image to: ...) then this will be used for the images without colorbox as well. But I can imagine that for someone this is ok.
In my ideal world there should be two "Link image to: " options - one for image without colorbox and second one for image with colorbox.
What do you think?

Thanks

  • Snater committed 6329ca6 on 8.x-2.x
    Issue #3079702: Prevent insert_colorbox from overwriting insert settings
    
Snater’s picture

Status: Needs review » Fixed

I get your point. As far as I remember, in Drupal 7, the image style for the Colorbox was only a global setting already and I just left it that way when re-implementing the module for Drupal 8 as it seemed to have been sufficient all along.

Originally, the Insert module was by intention meant to be a simplistic module. By now, its complexity in terms of functionality has moved away from that intention to some extend. Anyway, every new functionality needs consideration in terms of whether it is worth not just the implementation effort (that means spending a developers—most likely my—spare time), but also whether a considerable amount of module users would actually make use of this functionality to justify the increase in complexity. You had noticed already that some set-up functionality of Insert might be misunderstood.

I can totally see that the feature you request would be useful for others as well, I would just feel more confident about adding this functionality by more module users backing the request. That said, I have opened a new ticket for the feature request (#3097787: Specify Colorbox image style per image field). I am closing this ticket since the original issue is resolved as far as I can see. Thanks again for reporting the issue. :)

Status: Fixed » Closed (fixed)

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