imagehelper - filename (towebp, tojpg) - #2556

Thanks so much @EpicKau ! It looks like some tests need to be updated to match with this new naming convention. Could you give it a look?

@jarednova So what I see is that most time the test failed because of "assertFileExists" check. ("Failed asserting that file "XYZ" exists.")
That makes sense because the output file has another name than the input file.
E.g. XYZ-tojpg.jpg instead of XYZ.jpg.

Exactly @EpicKau (at least, that's what it looks like) — just need to reflect the new expected naming convention

Coverage increased (+0.007%) to 92.301% when pulling ffc3b39 on EpicKau:master into 68bb27b on timber:master.

So I ran into this issue #2748 as well and found this PR after some digging. #2535 seems to be related as well.

I did some local tests and this pr fixes both problems for me. both the webp and and jpg generated images are removed.

Only files that are not accounted for are when running towebp and tojpg on crops that will result to filenames like these.

test-400x267-towebp.webp
test-400x267-tojpg.jpg

This is still an issue we run into every now and then. Is there an ETA for this fix?

@gchtr @pascalknecht Any chance we could get this merged? The installations are getting bigger and bigger. I could also bring some 🍻 to your office. 😊

@EpicKau I tested it with WebP images generated with Timber\ImageHelper::img_to_webp and it doesn't seem to add the "-towebp" suffix. So these generated versions wouldn't be deleted by this addition. Also I found another issue that seems to come up, if the pictures have the WordPress native suffix "-scaled". That would need to be accounted aswell, since these would not be deleted either.

self::processDeleteGeneratedFiles($filename, 'webp', $dir, '-[0-9999999]*', '-[0-9]*x[0-9]*-c-[a-z]*.');
self::processDeleteGeneratedFiles($filename, 'webp', $dir, '-lbox-[0-9999999]*', '-lbox-[0-9]*x[0-9]*-[a-zA-Z0-9]*.');

// If scaled image is deleted, delete the original image webp as well
$filename_without_scaled = str_replace('-scaled', '', $filename);

self::processDeleteGeneratedFiles($filename_without_scaled, 'webp', $dir, '-[0-9999999]*', '-[0-9]*x[0-9]*-c-[a-z]*.');
self::processDeleteGeneratedFiles($filename_without_scaled, 'webp', $dir, '-lbox-[0-9999999]*', '-lbox-[0-9]*x[0-9]*-[a-zA-Z0-9]*.');

With these addition we seem to catch all files. Would be great to see this implemented by default, if you need any help on this, I am happy to help with the update of the pull request.

@svenvonarx

I tested it with WebP images generated with Timber\ImageHelper::img_to_webp and it doesn't seem to add the "-towebp" suffix.

The -towebp and -tojpg suffix have been added in this PR. See the lib/Image/Operation/ToJpg.php and lib/Image/Operation/ToWebp.php in the changed files.

@svenvonarx

I tested it with WebP images generated with Timber\ImageHelper::img_to_webp and it doesn't seem to add the "-towebp" suffix.

The -towebp and -tojpg suffix have been added in this PR. See the lib/Image/Operation/ToJpg.php and lib/Image/Operation/ToWebp.php in the changed files.

Yes, source files that are changed to webp, but not crops that are changed to webp. Those still have to be addressed.

There’s no ETA on this fix yet. Because there’s more involved here than simply merging this PR. But it is definitely something we want to work on next.

As mentioned in #2556 (review), this "fix" would cause many websites to generate a lot of images, while existing images will not be deleted automatically. If we don’t classify this as a breaking change, many websites could run into problems.

I wrote an answer to a similar issue in #2876 (comment), which outlines the problems we have to solve:

As with all the PR involving file name changes for images, I wonder how we should handle this properly. Because this will cause a lot of files to be regenerated. It could cause lots of max execution errors and could quickly fill up space on some hosts and leave the uploads folder with lots of unused files.

Could we somehow add a migration path so that developers have a way to fix this on their sites?

• Provide a way to delete all deprecated (or even all) WebP files using the WP CLI.
• But then again, not all hosts allow you to run WP CLI. So maybe we could add integrations for popular image regeneration plugins that could handle the migration as well.
• Maybe save generated WebP files to a different folder, as envisioned in Big ticket about Image handling #2866

I wonder whether we even should put this change behind a filter hook. The filter could be applied for all new Timber installations. For existing installations, we could maybe work with a flag that is saved in the wp_options database and is deleted once the change is through. Depending on the flag, we could show a notification in the admin dashboard.

So, for this issue to be fixed more quickly, I think the most straight-forward way to go would be to add the fixes as an opt-in feature through a WordPress filter hook in a new PR against the 2.x branch.

Something like:

add_filter('timber/image/advanced_file_names', '__return_true');

@svenvonarx @marcoluzi @enrikberisha Does any one of you have the capacity to work on a basic PR for this? If we had the base code, this would surely speed up a fix for this. If you can’t include tests, that would be fine as well. We could work on those.

@enrikberisha Thanks for the offer for the 🍻😄. Let’s have on at WordCamp Europe in Basel with the whole Timber team, if you can make it 😉.

@gchtr I can try to come up with something. 👍🏻