NEW Allow file variants with different extensions #585

GuySartorelli · 2024-01-16T22:00:45Z

Description

Provides a low-level API for generating a file variant which has a different extension than the original file.
e.g. could be used for:

Generating thumbnails for videos/documents/etc
Converting .png or .jpg to .webp
Converting .docx to .pdf

Manual testing steps

Upload a video file.
Upload a document file.

Use this code for PageController.
Replace /var/www/html/.eddev/samples/snickers.jpg with the absolute path to some arbitrary image file your site can access (or replace that logic with something to actually generate a file from the video - I'm taking some shortcuts in that regard)
Replace /var/www/html/.eddev/samples/client-pdf2.pdf with the absolute path to some arbitrary pdf file your site can access (or replace that logic with something to actually generate a pdf from the document - I'm taking some shortcuts in that regard)

use SilverStripe\Assets\Storage\AssetStore;
use SilverStripe\Assets\File;
use SilverStripe\Assets\Image_Backend;
use SilverStripe\CMS\Controllers\ContentController;
use SilverStripe\Core\Injector\Injector;

class PageController extends ContentController
{
    public function getVideoImage()
    {
        $file = File::get()->find('FileFilename:EndsWith', File::get_category_extensions('video'));
        return $file->manipulateExtension('jpg', function (AssetStore $store, string $filename, string $hash, string $variant) {
            $backend = Injector::inst()->create(Image_Backend::class);
            $backend->loadFrom('/var/www/html/.eddev/samples/snickers.jpg');
            $tuple = $backend->writeToStore($store, $filename, $hash, $variant, ['conflict' => AssetStore::CONFLICT_USE_EXISTING]);
            return [$tuple, $backend];
        });
    }

    public function getPdfFromDoc()
    {
        $file = File::get()->find('FileFilename:EndsWith', File::get_category_extensions('document'));
        return $file->manipulateExtension('pdf', function (AssetStore $store, string $filename, string $hash, string $variant) {
            $tuple = $store->setFromString(
                file_get_contents('/var/www/html/.eddev/samples/client-pdf2.pdf'),
                $filename,
                $hash,
                $variant,
                ['conflict' => AssetStore::CONFLICT_USE_EXISTING]
            );
            return [$tuple, null];
        });
    }
}

Add this to a page template:

<a href="$VideoImage.ScaleMaxHeight(200).CropHeight(50).Link">Link to the resized, cropped thumbnail</a> - ID=$VideoImage.ID
<br>
<a href="$PdfFromDoc.Link">Link to the "converted" pdf</a> - ID=$PdfFromDoc.ID

Visit a page and click on the links.
- For the image, you should see an appropriately scaled and cropped copy of your dummy thumbnail image.
- For the pdf, you should see the pdf file you pointed your code to.
- The URL should be obviously a variant of the original file
- The displayed ID numbers should be the original file IDs

You can also try the documentation example of converting one image to another image (e.g. a .jpg to .webp) and see that it also works correctly, even with chained image manipulations.

Issues

Allow file variant with different extension #488

Pull request checklist

The target branch is correct
- See picking the right version
All commits are relevant to the purpose of the PR (e.g. no debug statements, unrelated refactoring, or arbitrary linting)
- Small amounts of additional linting are usually okay, but if it makes it hard to concentrate on the relevant changes, ask for the unrelated changes to be reverted, and submitted as a separate PR.
The commit messages follow our commit message guidelines
The PR follows our contribution guidelines
Code changes follow our coding conventions
This change is covered with tests (or tests aren't necessary for this change)
Any relevant User Help/Developer documentation is updated; for impactful changes, information is added to the changelog for the intended release
CI is green

src/FilenameParsing/HashFileIDHelper.php

src/FilenameParsing/AbstractFileIDHelper.php

GuySartorelli · 2024-01-17T03:31:31Z

src/ImageBackendFactory.php

+        // If no asset container was passed in, create a new uncached image backend
+        if (!$assetContainer) {
+            return $this->creator->create($service, $params);
+        }


Needed so that developers can get an instance of the injected Image_Backend when they don't have a valid AssetContainer instance that contains an image file. The example in the "manual testing steps" section of the PR description simply won't work without this.

GuySartorelli · 2024-01-17T03:32:26Z

src/ImageBackendFactory.php

+            return $this->creator->create($service, $params);
+        }
+
+        if (!($assetContainer instanceof AssetContainer)) {


This used to be if (!$assetContainer instanceof AssetContainer) - which actually checks if ((!$assetContainer) instanceof AssetContainer). In other words it was checking if a boolean was an instance of a class, which will always evaluate to false.

src/ImageManipulation.php

GuySartorelli · 2024-01-18T04:05:54Z

src/InterventionBackend.php

+            // Make sure we're using the extension of the variant file, which can differ from the original file
+            $url = $assetStore->getAsURL($filename, $hash, $variant, false);
+            $extension = pathinfo($url, PATHINFO_EXTENSION);
            // Save file
-            $extension = pathinfo($filename ?? '', PATHINFO_EXTENSION);


This change is vital if using InterventionImage::writeToStore() to store (and encode) the variant. Without this, it will think it's meant to store it using the original file extension which will be wrong at best and outright fail if the original file wasn't an image.

GuySartorelli · 2024-01-18T04:12:06Z

src/FilenameParsing/AbstractFileIDHelper.php

+     * @param string $filename Original filename without variant
+     * @param int $extIndex One of self::EXT_ORIGINAL or self::EXT_VARIANT
+     */
+    private function swapExtension(string $filename, string $variant, int $extIndex): string


Swapping the extension is necessary so we can correctly identify the file on the filesystem. Without this, the original file extension is used when searching for the variant file in the filesystem.

e.g. you have an original file myfile.mp4, and a variant myfile__extRewriteWyJtcDQiLCJqcGciXQ.jpg.
Without this method, if would look for myfile__extRewriteWyJtcDQiLCJqcGciXQ.mp4 on the filesystem, and obviously it wouldn't find it.

GuySartorelli · 2024-01-18T04:20:44Z

tests/php/ImageTest.php

See the description of c8f0c29 for changes in this file

GuySartorelli · 2024-01-18T04:21:11Z

src/ImageManipulation.php

-        return array_merge([$matches['format']], $args[0]);
+        return array_merge([$matches['format']], $args);


See the description of c8f0c29

emteknetnz

I'm finding this too hard too peer review despite it being split into different commits. I'm jumping between the 'commits' tab and the 'files changed' tab where I want to put my comments and loosing track of things.

Please split this PR into two different PRs, one for refactoring and one for the original issue. In the future please refrain from doing large amount of refactoring in PRs that also contain new functionality / fix a bug .

GuySartorelli · 2024-01-25T02:04:15Z

Moved the refactoring into #587

Marking this as draft because it'll have conflicts once that is merged.

GuySartorelli · 2024-01-25T19:58:17Z

src/FilenameParsing/NaturalFileIDHelper.php

@@ -2,8 +2,6 @@

 namespace SilverStripe\Assets\FilenameParsing;

-use SilverStripe\Core\Injector\Injectable;


Missed this in the refactor PR - it's not used anymore

emteknetnz · 2024-01-25T20:55:53Z

src/FilenameParsing/AbstractFileIDHelper.php

+        $subVariants = explode('_', $variant);
+
+        // Split our filename into a filename and extension part
+        if (!preg_match('/(?<basename>.+)\.(?<ext>[a-z\d]+)$/i', $filename, $matches)) {


Use pathinfo() instead of your own regex, there may be some weird filenames this doesn't work on

emteknetnz · 2024-01-25T21:00:31Z

src/FilenameParsing/NaturalFileIDHelper.php

+        $variant = isset($matches['variant']) ? $matches['variant'] : '';
+
+        if (isset($variant)) {


Suggested change

$variant = isset($matches['variant']) ? $matches['variant'] : '';

if (isset($variant)) {

$variant = $matches['variant'] ?: '';

if ($variant) {

This isn't new code (it's just moved from below) but I'll validate and make a change here to avoid PR ping pong

src/ImageManipulation.php

emteknetnz · 2024-01-25T21:11:19Z

src/FilenameParsing/AbstractFileIDHelper.php

+        }
+
+        // Split variant string in variant list
+        $subVariants = explode('_', $variant);


What happens when the original filename has underscores in it? You'll need to add a unit test for this.

It won't match the variant regex and so it will be ignored.

I've added underscores to file names in the test scenarios.

emteknetnz · 2024-01-25T21:19:13Z

src/FilenameParsing/AbstractFileIDHelper.php

+    /**
+     * A variant type for encoding a variant filename with a different extension than the original.
+     */
+    public const EXTENSION_REWRITE_VARIANT = 'extRewrite';


Suggested change

public const EXTENSION_REWRITE_VARIANT = 'extRewrite';

public const EXTENSION_REWRITE_VARIANT = 'ExtRewrite';

All the other variants types have initial caps

That's just a fluke because the method names start with a capital letter - but I'll change this to avoid PR ping pong

emteknetnz · 2024-01-25T21:21:07Z

src/FilenameParsing/AbstractFileIDHelper.php

+     * @param string $filename Original filename without variant
+     * @param int $extIndex One of self::EXT_ORIGINAL or self::EXT_VARIANT
+     */
+    private function swapExtension(string $filename, string $variant, int $extIndex): string


The naming on these three methods seems really inconsistent, the words they start with are

"rewrite"
"restore"
"swap"

Yet they all do the same thing?

Just get rid of the first two methods just make this method public and also the EXT_* constants public

Just lifted these straight from the POC as they were, since they did what they needed to do.

Done - but made protected since they're only used by subclasses (and used reflection in tests)

emteknetnz · 2024-01-25T21:24:58Z

src/FilenameParsing/AbstractFileIDHelper.php

+    /**
+     * use the original file's extension
+     */
+    private const EXT_ORIGINAL = 0;
+
+    /**
+     * use the variant file's extension
+     */
+    private const EXT_VARIANT = 1;


Suggested change

/**

* use the original file's extension

*/

private const EXT_ORIGINAL = 0;

/**

* use the variant file's extension

*/

private const EXT_VARIANT = 1;

/**

* Use the original file's extension

*/

private const EXTENSION_ORIGINAL = 0;

/**

* Use the variant file's extension

*/

private const EXTENSION_VARIANT = 1;

Make the naming consistent with other contant above and also make it slightly clearer. Also initial caps for "Use"

Will also need to update references to consts

emteknetnz · 2024-01-25T21:25:49Z

src/FilenameParsing/HashFileIDHelper.php

+        $variant = isset($matches['variant']) ? $matches['variant'] : '';
+
+        if (isset($variant)) {


Suggested change

$variant = isset($matches['variant']) ? $matches['variant'] : '';

if (isset($variant)) {

$variant = $matches['variant'] ?: '';

if ($variant) {

#585 (comment)

Done

Without this, trying to pass any variant name into variantParts() will result in `TypeError: array_merge(): Argument silverstripe#2 must be of type array, string given` This wasn't caught until now because `variantParts()` isn't actually used anywhere in our codebase.

emteknetnz

Tested locally, works good

Won't block merging on this, though just a question really - is it expected to have intermediatory files stay on the file system, for instance I followed the DOC PR instructions for creating a webp image $MyFile.format('webp').ScaleWidth(150), though I ended up with this in the filesystem

Ideally we wouldn't have the unused webp image persisted instead only have the webp+ScaleWidth image, is it viable to have the intermediatory image not persist? OK to make this a new card if it's viable though complex

GuySartorelli · 2024-01-28T21:08:25Z

I'm not even sure if that's viable, to be honest. Each variant is a file. Here you have one variant for the file conversion, and another variant for scaling the image - and because those are discrete steps, they create discrete files.

I'm pretty sure the same thing would happen if you scaled and then rotated an image - you'd have one variant (and therefore one file) for the scaling operation and one for the rotation operation.

I don't think changing that is really feasible - or at least with my current limited understanding of the assets system I can't think of a way to tackle it.

GuySartorelli marked this pull request as draft January 16, 2024 22:00

This was referenced Jan 16, 2024

Allow file variant with different extensions POC #411

Closed

Allow file variant with different extension #488

Closed

GuySartorelli force-pushed the pulls/2/file-conversion-variants branch 3 times, most recently from f75efd4 to 473f384 Compare January 17, 2024 03:36

GuySartorelli commented Jan 17, 2024

View reviewed changes

GuySartorelli mentioned this pull request Jan 17, 2024

DOC Document making variants with different file extensions silverstripe/developer-docs#441

Merged

7 tasks

GuySartorelli commented Jan 18, 2024

View reviewed changes

GuySartorelli force-pushed the pulls/2/file-conversion-variants branch from 473f384 to a5580a2 Compare January 18, 2024 04:19

GuySartorelli marked this pull request as ready for review January 18, 2024 04:19

GuySartorelli commented Jan 18, 2024

View reviewed changes

tests/php/ImageTest.php Outdated

Copy link

Member Author

GuySartorelli Jan 18, 2024 •

edited

Loading

Choose a reason for hiding this comment

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

See the description of c8f0c29 for changes in this file

GuySartorelli commented Jan 18, 2024

View reviewed changes

emteknetnz requested changes Jan 25, 2024

View reviewed changes

GuySartorelli marked this pull request as draft January 25, 2024 02:03

GuySartorelli mentioned this pull request Jan 25, 2024

ENH Refactor FileIDHelper classes to reduce code duplication #587

Merged

GuySartorelli force-pushed the pulls/2/file-conversion-variants branch from a5580a2 to c8f0c29 Compare January 25, 2024 19:56

GuySartorelli commented Jan 25, 2024

View reviewed changes

GuySartorelli marked this pull request as ready for review January 25, 2024 19:58

emteknetnz requested changes Jan 25, 2024

View reviewed changes

GuySartorelli force-pushed the pulls/2/file-conversion-variants branch from c8f0c29 to 1bfa91e Compare January 25, 2024 22:21

GuySartorelli added 2 commits January 26, 2024 11:23

NEW Allow file variants with different extensions

a40ab50

GuySartorelli force-pushed the pulls/2/file-conversion-variants branch from 1bfa91e to d9d5dac Compare January 25, 2024 22:23

emteknetnz requested changes Jan 28, 2024

View reviewed changes

emteknetnz approved these changes Jan 28, 2024

View reviewed changes

emteknetnz merged commit 530b57f into silverstripe:2 Jan 28, 2024
9 checks passed

emteknetnz deleted the pulls/2/file-conversion-variants branch January 28, 2024 21:28

lerni mentioned this pull request Feb 18, 2024

use clone not chained methods to work with InterventionBackend & vips jonom/silverstripe-focuspoint#120

Draft

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

NEW Allow file variants with different extensions #585

NEW Allow file variants with different extensions #585

GuySartorelli commented Jan 16, 2024 •

edited

Loading

GuySartorelli Jan 17, 2024

GuySartorelli Jan 17, 2024

GuySartorelli Jan 18, 2024

GuySartorelli Jan 18, 2024 •

edited

Loading

GuySartorelli Jan 18, 2024 •

edited

Loading

GuySartorelli Jan 18, 2024 •

edited

Loading

emteknetnz left a comment

GuySartorelli commented Jan 25, 2024

GuySartorelli Jan 25, 2024

emteknetnz Jan 25, 2024

GuySartorelli Jan 25, 2024

emteknetnz Jan 25, 2024

GuySartorelli Jan 25, 2024 •

edited

Loading

GuySartorelli Jan 25, 2024

emteknetnz Jan 25, 2024

GuySartorelli Jan 25, 2024

GuySartorelli Jan 25, 2024

emteknetnz Jan 25, 2024

GuySartorelli Jan 25, 2024

GuySartorelli Jan 25, 2024

emteknetnz Jan 25, 2024

GuySartorelli Jan 25, 2024

emteknetnz Jan 25, 2024

GuySartorelli Jan 25, 2024

emteknetnz Jan 25, 2024

GuySartorelli Jan 25, 2024

emteknetnz left a comment •

edited

Loading

GuySartorelli commented Jan 28, 2024 •

edited

Loading

		return array_merge([$matches['format']], $args[0]);
		return array_merge([$matches['format']], $args);

		@@ -2,8 +2,6 @@

		namespace SilverStripe\Assets\FilenameParsing;

		use SilverStripe\Core\Injector\Injectable;

		$variant = isset($matches['variant']) ? $matches['variant'] : '';

		if (isset($variant)) {

	public const EXTENSION_REWRITE_VARIANT = 'extRewrite';
	public const EXTENSION_REWRITE_VARIANT = 'ExtRewrite';

NEW Allow file variants with different extensions #585

NEW Allow file variants with different extensions #585

Conversation

GuySartorelli commented Jan 16, 2024 • edited Loading

Description

Manual testing steps

Issues

Pull request checklist

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

GuySartorelli Jan 18, 2024 • edited Loading

Choose a reason for hiding this comment

GuySartorelli Jan 18, 2024 • edited Loading

Choose a reason for hiding this comment

GuySartorelli Jan 18, 2024 • edited Loading

Choose a reason for hiding this comment

emteknetnz left a comment

Choose a reason for hiding this comment

GuySartorelli commented Jan 25, 2024

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 Jan 25, 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

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

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

emteknetnz left a comment • edited Loading

Choose a reason for hiding this comment

GuySartorelli commented Jan 28, 2024 • edited Loading

GuySartorelli commented Jan 16, 2024 •

edited

Loading

GuySartorelli Jan 18, 2024 •

edited

Loading

GuySartorelli Jan 18, 2024 •

edited

Loading

GuySartorelli Jan 18, 2024 •

edited

Loading

GuySartorelli Jan 25, 2024 •

edited

Loading

emteknetnz left a comment •

edited

Loading

GuySartorelli commented Jan 28, 2024 •

edited

Loading