Replace images in frontend content without using an additional regular expression #262
Conversation
…y replace images without using the preg_match call.
eugene-manuilov
added
[Type] Enhancement
eugene-manuilov
requested review from
adamsilverstein and
getsource
as code owners
There was a problem hiding this comment.
@eugene-manuilov Great work! One minor thing, but nothing blocking.
modules/images/webp-uploads/load.php
Outdated
| // Replace the full size image if present. | ||
| if ( isset( $metadata['sources'][ $target_mime ]['file'] ) ) { | ||
| $basename = wp_basename( $metadata['file'] ); | ||
| $image = str_replace( $basename, $metadata['sources'][ $target_mime ]['file'], $image ); |
There was a problem hiding this comment.
These values could be the same. Just thinking out loud here, in terms of PHP performance (I know it's very minor but still worth thinking about) not sure whether that's okay or whether it would be better to use an if check around them.
There was a problem hiding this comment.
Added a check to avoid replacing the same images.
felixarntz
changed the title
…ace the same images.
modules/images/webp-uploads/load.php
Outdated
| if ( | ||
| ! empty( $size_data['file'] ) && | ||
| ! empty( $size_data['sources'][ $target_mime ]['file'] ) && | ||
| $size_data['file'] !== $size_data['sources'][ $target_mime ]['file'] | ||
| ) { |
There was a problem hiding this comment.
While technically this is the same group of conditionals as before I would argue that this way it's harder to follow, instead 1 line condition moving on makes for a more natural and simple flow.
There was a problem hiding this comment.
I would agree with it if this condition is written on a single line. IMO having every check on its own line within one condition has the same effect as if we would add three separate conditions for every check. However, if more people think that it's hard to follow, I don't mind to update it to have separate conditions.
There was a problem hiding this comment.
I think this could be considered a personal preference, but my take here is that I would agree with @mitogh the previous way was easier to follow. I also prefer the if not x, then bail early (i.e. continue) approach better than if x, run the logic, as it avoids excessive nesting. Of course it's just a single if clause here, but I think it's easier to read if the replacement remains directly within the foreach loop and the checks happen before as safe guards.
So to summarize my personal take:
- I prefer the way it was before.
- However, my main point would be to reverse the condition and bail early based on that, rather than only running the logic within the if clause.
- I don't have a strong preference on multiple if clauses or a single one, but I tend to agree when split in multiple it's easier to read.
There was a problem hiding this comment.
Sounds good to me then. Updated. Thanks, @mitogh and @felixarntz.
Merge package.json fix back to trunk
Summary
Fixes #238
Relevant technical choices
Checklist
[Focus]orInfrastructurelabel.[Type]label.no milestonelabel.