Fix/830 bulk optimisation #832
Conversation
|
I've tried this scenario: 1 - Upload some images, 👍 |
| $result = $this->get_bulk_instance( $context )->get_optimized_media_ids_without_format( $format ); | ||
| $media_ids['ids'] = array_merge( $media_ids['ids'], $result['ids'] ); | ||
| } |
There was a problem hiding this comment.
Why are we looping on the formats and optimized media without formats? This is not the purpose of this method. I think only getting the unoptimized media is enough
There was a problem hiding this comment.
I think it's not needed anymore after the change from latest PR #828
There was a problem hiding this comment.
If I may,
The problem of the method get_optimized_media_ids_without_format($format) is that the SQL query within this method only look for the media that have already been optimized by Imagify but without the format.
So in the following scenario:
1 - Upload some images,
2 - Install Imagify
3 - Enable AVIF
4 - Start a bulk optimization
It won't work, as the uploaded image have never been optimized by Imagify, so in the database we won't have any _imagify_status or _imagify_data within the wp_postmeta table.
While the function get_unoptimized_media_ids doesn't care about these 2 metas values. And in the precedent scenario it will gets the IDs of these images and be able to convert them.
There was a problem hiding this comment.
With get_unoptimized_media_ids(), it will optimize the image and generate any next-gen image too at the same time. For me that's enough, and that is the behaviour that was in previous versions
There was a problem hiding this comment.
Thanks for the catch and details provided here @Tabrisrp.
Since the PR has been tested and the expected behavior is now OK, are you OK with opening a dedicated issue to try and remove the get_optimized_media_ids_without_format loop part?
This for loop might impact performances but that's only when doing bulk optimization so I think it's acceptable.
This would allow to move forward with the PR and 2.2 without having to re-test, and possible regressions for now ; and keep that optimization for the next Imagify minor (which should be soon anyway).
There was a problem hiding this comment.
follow up PR opened here #842
|
Working fine now so if no further changes we can merge it. |
Description
Fixes #830
Documentation
User documentation
This allow users don't to bulk optimise images that aren't optimise.
Technical documentation
Explain how this code works. Diagram & drawings are welcomed.
Type of change
New dependencies
N/A
Risks
List possible performance & security issues or risks, and explain how they have been mitigated.
Checklists
Feature validation
Documentation
Code style