NEW File converter API #595

GuySartorelli · 2024-04-04T04:26:23Z

Description

Provides:

An API for file converters, so the community can provide their own file converters and expect them to work consistently with anything we build ourselves
A file converter that uses Intervention Image for converting from one image format to another
A manager for file conversion so you can just try a conversion and if there's any converter which supports it, your conversion will be successful
A method on File and DBFile to leverage available converters

Note that one AC is self-fulfilling:

Converters can validate the presence of required libraries, binaries, PHP modules or dependencies.

Converters can validate whatever the heck they want. If they don't have the stuff they need, they should just return false from the method that checks if they support a given conversion.

Issue

API/Service for registering "converters" that will create file variant under a different extensions #489

src/Conversion/FileConversionManager.php

GuySartorelli · 2024-04-08T23:41:17Z

src/Conversion/FileConversionException.php

+/**
+ * An exception that represents a failure to convert a file in a FileConverter class.
+ */
+class FileConversionException extends LogicException


This allows us and developers to catch exceptions that were intentionally thrown for conversion failures - while still correctly getting errors in their faces if there are unexpected exceptions.

GuySartorelli · 2024-04-08T23:42:59Z

src/Conversion/FileConversionManager.php

+     * Note that if a converter supports the conversion generally but doesn't support these options, that converter will not be used.
+     * @throws FileConversionException if the conversion failed or there were no converters available.
+     */
+    public function convert(DBFile $from, string $toFormat, array $options = []): DBFile


Providing $options here gives a higher level of control for developers (and converters) which might require it.

GuySartorelli · 2024-04-08T23:43:42Z

src/Conversion/InterventionImageFileConverter.php

+        return $problems;
+    }
+
+    private function supportedByIntervention(string $from, string $to, string $driver): bool


Intervention Image doesn't currently tell you what it supports. It might in the future but for now we have to check for ourselves.

src/ImageManipulation.php

GuySartorelli · 2024-04-08T23:49:28Z

src/ImageManipulation.php

+     * exceptions for end users.
+     * @throws FileConversionException If the conversion fails and $returnNullOnFailure is false.
+     */
+    public function convert(string $toFormat, bool $returnNullOnFailure = false): ?AssetContainer


I originally had $returnOriginalOnFailure instead of $returnNullOnFailure, and it would return $this if the conversion failed. But null is more consistent with how other methods in this file behave, and in many cases it would be more desirable to display nothing in the template instead of the wrong thing.

We could optionally remove that parameter and just always return null on failure. I'll leave it as is for now pending review.

The main benefit of removing the parameter is that it is more consistent with other methods here. We'd probably want to at least log the failure though if we did that.
The main benefit of keeping it is that silent failing is an explicit decision developers have to make, which makes it safer.

I think remove the param and just return null

You have written this "This is useful if calling this method from a template, for example, where you don't want to be throwing for end users."

That sounds like the main scenario this method will be called in? In which case let's not force the developer to have to put in a param, which they don't have to add in for any other template methods.

I don't like that personally but I don't feel strongly enough about it to have peer review ping pong about it.

That sounds like the main scenario this method will be called in?

There are all PHP methods and can be called from PHP. Any time you're dealing with an image (or file in this case) in PHP for any reason, you'll call these methods. File conversion will I suspect be just as common in PHP as it will in templates, for things like converting files before attaching to emails, etc.

That said, I've made the requested change and logged the error as a compromise.

GuySartorelli · 2024-04-09T02:19:01Z

src/Conversion/InterventionImageFileConverter.php

+        } catch (ImageException $e) {
+            throw new FileConversionException('Failed to convert: ' . $e->getMessage(), $e->getCode(), $e);
+        }
+        // This is very unlikely but the API for `manipulateExtension()` allows for it
+        if ($result === null) {
+            throw new FileConversionException('File conversion resulted in null. Check whether original file actually exists.');
+        }


I can't find a way to test either of these - I think all of the scenarios that would normally result in these are being explicitly validated against already. This is just here as some defensive programming.

GuySartorelli · 2024-04-09T02:49:18Z

src/ImageManipulation.php

+     * exceptions for end users.
+     * @throws FileConversionException If the conversion fails and $returnNullOnFailure is false.
+     */
+    public function convert(string $toFormat, bool $returnNullOnFailure = false): ?AssetContainer


Note that this is in the ImageManipulation trait because this trait is shared in both the File and DBFile class. The issue originally had some comments about this that I wanted to have as part of the ACs but it was decided that they were not relevant - so we're stuck with putting a file-thing into an image trait.

src/Conversion/InterventionImageFileConverter.php

michalkleiner · 2024-04-09T05:27:54Z

✅ I haven't done any functional testing so won't do a full PR approval but in general this looks good to me.

emteknetnz

As usual my main thing is naming :-)

Change FileConversionManager to FileConverterManager
Change FileConversionException to FileConverterException
Change $fromFormat to $fromExtension
Change $toFormat to $toExtension
convert() should be Convert() to match convention when used from templates

Also make corresponding naming changes in silverstripe/developer-docs#491

Also, I had this in my template: $MyFile.convert('webp') - however the html rendered had the original png file as the alt tag. I would have expected a webp file in the alt tag.

emteknetnz · 2024-04-10T03:21:02Z

src/ImageManipulation.php

+     * exceptions for end users.
+     * @throws FileConversionException If the conversion fails and $returnNullOnFailure is false.
+     */
+    public function convert(string $toFormat, bool $returnNullOnFailure = false): ?AssetContainer


I think remove the param and just return null

You have written this "This is useful if calling this method from a template, for example, where you don't want to be throwing for end users."

That sounds like the main scenario this method will be called in? In which case let's not force the developer to have to put in a param, which they don't have to add in for any other template methods.

emteknetnz · 2024-04-10T03:22:42Z

src/Conversion/InterventionImageFileConverter.php

+                if ($driver === 'gd' && !function_exists('imagewebp')) {
+                    return false;
+                }
+                if ($driver === 'imagick' && !\Imagick::queryFormats('WEBP')) {


Suggested change

if ($driver === 'imagick' && !\Imagick::queryFormats('WEBP')) {

if ($driver === 'imagick' && !Imagick::queryFormats('WEBP')) {

Import Imagick up the top. Remove the trailing slash for other examples in this file

src/Conversion/InterventionImageFileConverter.php

GuySartorelli · 2024-04-10T04:11:10Z

Change FileConversionManager to FileConverterManager
Change FileConversionException to FileConverterException
Change $fromFormat to $fromExtension
Change $toFormat to $toExtension

Done, though I think the first two are not as good as the original names. Naming is always subjective though so 🤷‍♀️

convert() should be Convert() to match convention when used from templates

This was suggested in #595 (comment) and I provided reasoning why I didn't do that in that comment thread. Please read that - and if you still want this change after you have that context, I'll make the change at that stage to avoid additional ping pong.

Also, I had this in my template: $MyFile.convert('webp') - however the html rendered had the original png file as the alt tag. I would have expected a webp file in the alt tag.

This is unrelated to this API - that's a bug with the low-level API and will need to be logged as a separate issue.

emteknetnz · 2024-04-10T05:39:07Z

convert() should be Convert() to match convention when used from templates

Yes it's not following our own best practice for PHP methods ... however when we wrote those we probably didn't consider methods intended to called from template. The convention there very much is initial caps for method names unfortunately and we should follow the existing convention. ImageManipulation is full of public methods named like that. End users will be expecting to use $MyFile.Convert('webp') rather than $MyFile.convert('webp') because they're already calling $MyFile.ThumbnailURL(300, 200)

GuySartorelli · 2024-04-10T06:02:47Z

Methods are just methods. Some methods can be called from templates. Especially in this case, it is not exclusively intended to be used in templates.

Nevertheless, I will rename this method to avoid additional unnecessary ping pong - but in the future I will be more resistant to this sort of change because it does go against our best practices (which do not and should not make exceptions for methods which can be called from templates).

Done.

GuySartorelli marked this pull request as draft April 4, 2024 04:26

GuySartorelli mentioned this pull request Apr 4, 2024

API/Service for registering "converters" that will create file variant under a different extensions #489

Closed

6 tasks

GuySartorelli force-pushed the pulls/2/file-converter-api branch 6 times, most recently from 1f18d36 to 57bc27d Compare April 8, 2024 03:34

GuySartorelli commented Apr 8, 2024

View reviewed changes

src/Conversion/FileConversionManager.php Outdated Show resolved Hide resolved

GuySartorelli force-pushed the pulls/2/file-converter-api branch 2 times, most recently from 20539db to 2a8a715 Compare April 8, 2024 23:48

GuySartorelli commented Apr 8, 2024

View reviewed changes

GuySartorelli force-pushed the pulls/2/file-converter-api branch 2 times, most recently from cb08b83 to bf86a3f Compare April 9, 2024 02:18

GuySartorelli commented Apr 9, 2024

View reviewed changes

GuySartorelli force-pushed the pulls/2/file-converter-api branch from bf86a3f to 047acbd Compare April 9, 2024 03:33

GuySartorelli marked this pull request as ready for review April 9, 2024 03:39

michalkleiner reviewed Apr 9, 2024

View reviewed changes

src/Conversion/InterventionImageFileConverter.php Show resolved Hide resolved

GuySartorelli mentioned this pull request Apr 9, 2024

DOC Document new file conversion API silverstripe/developer-docs#491

Merged

emteknetnz requested changes Apr 10, 2024

View reviewed changes

GuySartorelli force-pushed the pulls/2/file-converter-api branch from 047acbd to a5574b5 Compare April 10, 2024 04:10

NEW File converter API

145e9a8

GuySartorelli force-pushed the pulls/2/file-converter-api branch from a5574b5 to 145e9a8 Compare April 10, 2024 06:02

emteknetnz approved these changes Apr 10, 2024

View reviewed changes

emteknetnz merged commit ff5e8a7 into silverstripe:2 Apr 10, 2024
9 checks passed

emteknetnz deleted the pulls/2/file-converter-api branch April 10, 2024 06:48

GuySartorelli added a commit to creative-commoners/silverstripe-assets that referenced this pull request Jul 17, 2024

NEW File converter API (silverstripe#595)

34ee570

GuySartorelli mentioned this pull request Nov 12, 2024

Image::Convert() shows error in frontend instead of returning null #656

Closed

2 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

NEW File converter API #595

NEW File converter API #595

GuySartorelli commented Apr 4, 2024 •

edited

Loading

GuySartorelli Apr 8, 2024

GuySartorelli Apr 8, 2024

GuySartorelli Apr 8, 2024

GuySartorelli Apr 8, 2024

GuySartorelli Apr 8, 2024 •

edited

Loading

emteknetnz Apr 10, 2024

GuySartorelli Apr 10, 2024

GuySartorelli Apr 9, 2024

GuySartorelli Apr 9, 2024

michalkleiner commented Apr 9, 2024

emteknetnz left a comment •

edited

Loading

emteknetnz Apr 10, 2024

emteknetnz Apr 10, 2024

GuySartorelli Apr 10, 2024

GuySartorelli commented Apr 10, 2024

emteknetnz commented Apr 10, 2024 •

edited

Loading

GuySartorelli commented Apr 10, 2024

	if ($driver === 'imagick' && !\Imagick::queryFormats('WEBP')) {
	if ($driver === 'imagick' && !Imagick::queryFormats('WEBP')) {

NEW File converter API #595

NEW File converter API #595

Conversation

GuySartorelli commented Apr 4, 2024 • edited Loading

Description

Issue

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

GuySartorelli Apr 8, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

michalkleiner commented Apr 9, 2024

emteknetnz left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

GuySartorelli commented Apr 10, 2024

emteknetnz commented Apr 10, 2024 • edited Loading

GuySartorelli commented Apr 10, 2024

GuySartorelli commented Apr 4, 2024 •

edited

Loading

GuySartorelli Apr 8, 2024 •

edited

Loading

emteknetnz left a comment •

edited

Loading

emteknetnz commented Apr 10, 2024 •

edited

Loading