Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Feature request: add sync-missing command to wp client. #7

Closed
wants to merge 5 commits into from

Conversation

juliquiron
Copy link

The problem
Some generated images can be removed or deleted for multiple reasons. When this happen the bulk optimizer can not re-optimize it again as long are already registered as optimized and no missing images to be optimized are detected.

Proposed solution
Implement a method to check if all expected images to be in the file system are really there. If not present then update the DB registry to let the bulk optimizer to be able to process it.

This PR implements a new wp command optimus sync-missing that performs this operation. The feature can be exposed also to the management pages, but is not done in this PR.

Copy link

@sun sun left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 Most of the changes are just moving existing code to be able to re-use it in the new command.

I'd recommend to rename the title of this PR to: "Add WP-CLI command to synchronize WebP image attachment optimization status with filesystem."

@@ -99,6 +129,11 @@ public static function add_commands() {
),
),
));

$cmd_syncmissing = function() { self::syncMissing(); };
WP_CLI::add_command( 'optimus sync-missing', $cmd_syncmissing, array(
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
WP_CLI::add_command( 'optimus sync-missing', $cmd_syncmissing, array(
WP_CLI::add_command( 'optimus webp sync', $cmd_syncmissing, array(

I would recommend to rename the (sub)command to wp optimus webp sync, so that the plugin is able to introduce further subcommands related to webp images in the future.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would prefer it like that as well. Would it make sense to align the function syncMissing to syncMissingWebp?

inc/optimus_cli.class.php Outdated Show resolved Hide resolved
@sun
Copy link

sun commented Apr 12, 2019

To be more clear on the actual problem:

In our case, the Optimus bulk image optimizer reported that 100% of all images would have been converted to webp already. But when testing random sample pages, various images were not served as webp, because they were not optimized yet.

It's not clear why the webp versions no longer exist (or whether they ever existed), but that is not the point of this PR. Instead, we want to synchronize the webp status in the database with the actual files in the filesystem.

Another solution would probably have been to implement some code to automatically check and generate missing webp images whenever a post/product is saved in WordPress, so as to ensure that there won't be missing webp images in the future. However, since we need to ensure this for all existing images right now, a bulk operation on the command line seemed more appropriate.

* Note: only works with convert to webp is enabled.
*/
public static function syncMissing () {
$options = Optimus::get_options();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please use 4 spaces for indentation.

@juliquiron
Copy link
Author

Re-pushed with all the feedback integrated on it. Hope now is ok! 👍

Copy link

@sun sun left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@juliquiron Thanks! 👍 Some minor adjustments are remaining, mostly regarding the documentation. Can you quickly fixup those?


$cmd_syncmissing = function() { self::syncMissingWebp(); };
WP_CLI::add_command( 'optimus webp sync', $cmd_syncmissing, array(
'shortdesc' => 'Detects missing optimized assets.',
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
'shortdesc' => 'Detects missing optimized assets.',
'shortdesc' => 'Synchronizes actual webp file optimization status and missing files on disk with database.',

foreach($assets as $asset_path) {
if (stream_resolve_include_path($asset_path) === FALSE) {
$metadata = wp_get_attachment_metadata($post['ID']);
// Unregister the optimus flags when the file is not longer there.
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
// Unregister the optimus flags when the file is not longer there.
// Remove the optimus metadata when the file does not exist.


foreach ($posts as $key => $post) {
$assets = Optimus_Request::get_files_paths($post['ID']);
foreach($assets as $asset_path) {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
foreach($assets as $asset_path) {
foreach ($assets as $asset_path) {

// Unregister the optimus flags when the file is not longer there.
unset($metadata['optimus']);
update_post_meta($post['ID'], '_wp_attachment_metadata', $metadata);
// Registries works at post level.
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
// Registries works at post level.
// No need to check further files as the whole attachment
// will be re-optimized.

WP_CLI::error('sync-missing command only syncs webp missing images. Looks like you don\'t have this setting enabled.', TRUE);
}

// Retrieves the post ids with optimized assets registered in the DB.
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
// Retrieves the post ids with optimized assets registered in the DB.
// Retrieve all post IDs with positive optimization status in database.

* sync-missing command.
*
* Checks if all the registered files really have the optimized version.
* Note: only works with convert to webp is enabled.
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
* Note: only works with convert to webp is enabled.
* Note: Only works if convert to webp is enabled.

* Checks if all the registered files really have the optimized version.
* Note: only works with convert to webp is enabled.
*/
public static function syncMissingWebp () {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
public static function syncMissingWebp () {
public static function webpSync () {

@@ -99,6 +129,11 @@ public static function add_commands() {
),
),
));

$cmd_syncmissing = function() { self::syncMissingWebp(); };
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
$cmd_syncmissing = function() { self::syncMissingWebp(); };
$cmd_syncmissing = function() { self::webpSync(); };

}

/**
* Bulk optimizer collect assets
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here and in PHPDoc blocks below: Wrong indentation of PHPDoc lines after the first.

$files = self::_get_files($metadata, $post_id);
$post_files = array();

foreach( $files as $file ) {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
foreach( $files as $file ) {
foreach ( $files as $file ) {

@juliquiron
Copy link
Author

@svenba all the relevant feedback have been incorporated in c6d7354.

Also in 68dc1e9 I tried to fix the PHPDoc indenting as suggested. Feel free to add or discard it :)

@svenba
Copy link
Contributor

svenba commented Jun 1, 2022

Some formatting issue are not fixed and feedback was not properly implemented (e.g. "public static function webpSync"). We can merge this if you provide a clean PR (single commit).

@svenba svenba closed this Jun 1, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants