mindkomm / timmy

Advanced image handling for Timber.
MIT License
171 stars 13 forks source link

make_content_images_responsive regex is too greedy #37

Closed drzraf closed 3 years ago

drzraf commented 3 years ago

It would include trailing "><..." after the class name.

Example string and resulting $block_images variable: $str = '\n<figure class="wp-block-image size-large"><a href="https://foo"><img src="/uploads/2020/02/hello-1024x0-c-default.jpg" alt="" class="wp-image-22361"/></a></figure>\n';

Before, notice the large"><a

array(3) {
  [0] =>
  array(1) {
    [0] =>
    string(163) "<figure class="wp-block-image size-large"><a href="https://foo"><img src="/uploads/2020/02/hello-1024x0-c-default.jpg" alt="" class="wp-image-22361"/></a></figure>"
  }
  [1] =>
  array(1) {
    [0] =>
    string(35) "wp-block-image size-large"><a href="
  }
  [2] =>
  array(1) {
    [0] =>
    string(9) "large"><a"
  }
}

After

array(3) {
  [0] =>
  array(1) {
    [0] =>
    string(163) "<figure class="wp-block-image size-large"><a href="https://foo"><img src="/uploads/2020/02/hello-1024x0-c-default.jpg" alt="" class="wp-image-22361"/></a></figure>"
  }
  [1] =>
  array(1) {
    [0] =>
    string(25) "wp-block-image size-large"
  }
  [2] =>
  array(1) {
    [0] =>
    string(5) "large"
  }
}

This warning: PHP Warning: array_merge(): Expected parameter 1 to be an array, bool given in /vendor/mindkomm/timmy/functions-images.php on line 244 was seen during Gutenberg editing and due to the call to get_timber_image_attributes_responsive($timber_image = 22361, $size = 'large"><a', $args = *uninitialized*) at/vendor/mindkomm/timmy/lib/Responsive_Content_Images.php:271`

Note: I didn't heavily tested the regexp so you'll probably want to add a couple of testcases before before merging ;)

gchtr commented 3 years ago

Hey @drzraf, thanks for your pull request! I always find regexes quite hard to get right.

I had to iterate over your solution in https://github.com/mindkomm/timmy/commit/3d622cd2d4ea8ad510176feb9b7ecd8ffd1102d2, because it wouldn’t work with images sizes with special characters like dashes and underscores in it. I should probably iterate over that regex once more to add characters like . or / in case somebody decides to use those. But then again, I think there might be a better regex solution than listing all the characters people might use.

I released a new version with the fix.