silverstripe / silverstripe-assets

Silverstripe Assets component
BSD 3-Clause "New" or "Revised" License
9 stars 65 forks source link

NEW File converter API #595

Closed GuySartorelli closed 2 months ago

GuySartorelli commented 3 months ago

Description

Provides:

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

michalkleiner commented 2 months ago

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

GuySartorelli commented 2 months ago

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 https://github.com/silverstripe/silverstripe-assets/pull/595#discussion_r1556894605 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 commented 2 months ago

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 commented 2 months ago

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.