[Image] Add the UX Image package - #3768
Conversation
carsonbot
added
Documentation
There was a problem hiding this comment.
Reviewed by checking the branch out locally: ran the full suite (504 tests / 1425 assertions pass on PHP 8.4 with gd + exif + imagick), ran php-cs-fixer and twig-cs-fixer, and probed the processing pipeline with throwaway scripts to confirm the behavioural findings rather than guess at them.
The design holds up well. The metadata-only rendering boundary, the immutable generation keys, StoragePath confinement and the bounded-provider regeneration contract are all genuinely well thought out, and the scope section says no to the right things. Most of what follows is about the GD encoding path and render-time attribute handling.
Detailed notes are inline on the diff. The three I would consider blocking:
- Transparent PNG produces JPEG variants with a black background. Verified: the corner pixel of a generated variant is
#000000. Nothing in the suite covers alpha, so it stays invisible until production. class,altandsizesare not in the reserved-attribute list, so passing them throughattributesemits duplicate attributes and the caller's value is silently dropped.htmlspecialchars($v, \ENT_QUOTES)drops the defaultENT_SUBSTITUTE, so any invalid UTF-8 blanks the whole attribute. Verified:alt: "Caf\xE9 crème"rendersalt="".
The lossy encoding intermediate, the Twig runtime wiring, the dead DBAL 3 methods, the unreachable non-stream storage path and the dropped TwigComponent attributes bag are API and behaviour decisions that get expensive to change once the package ships, so they are worth settling now rather than after. Everything else is polish.
Really nice work overall. The architecture survives a close read, the security posture is substantive rather than decorative, and the test suite is serious. Its one blind spot is alpha and transparency, which is exactly where the top issue lives.
carsonbot
added
Status: Needs Work
Kocal
left a comment
Kocal
left a comment
There was a problem hiding this comment.
Line-by-line notes, following up on the summary review above.
| 'webp' => imagewebp($image, $outputPath, $quality), | ||
| 'avif' => \function_exists('imageavif') ? imageavif($image, $outputPath, $quality) : throw ImageProcessingException::unsupportedFormat('avif'), | ||
| 'png' => imagepng($image, $outputPath, (int) (9 - ($quality * 9 / 100))), | ||
| 'jpeg', 'jpg' => imagejpeg($image, $outputPath, $quality), |
There was a problem hiding this comment.
Blocking: transparent PNG produces JPEG variants with a black background.
resize() fills the canvas with imagecolorallocatealpha($dst, 0, 0, 0, 127) (L312-317), then this line hands that image to imagejpeg(). JPEG has no alpha channel, so GD flattens the transparency onto black rather than white.
I ran a transparent PNG through a jpeg-only profile and read back the generated variant: the corner pixel is #000000.
A transparent logo comes out with a black background in every JPEG variant, and fill mode letterboxes in black too. Nothing in the suite covers alpha, so this stays invisible until someone looks at a rendered page. Most libraries flatten onto white; whichever colour you pick it should be a deliberate one, and ideally configurable.
| match ($type) { | ||
| \IMAGETYPE_PNG => imagepng($image, $path), | ||
| \IMAGETYPE_WEBP => imagewebp($image, $path), | ||
| default => imagejpeg($image, $path, 90), |
There was a problem hiding this comment.
saveImage() picks the encoder from the source image type, which makes the shared resized file a lossy intermediate whenever the upload is a JPEG: every variant goes through an extra JPEG q90 generation before the real encode runs.
Measured on a synthetic high-frequency source: mean absolute deviation of 2.64/255 per channel versus encoding straight from the resampled pixels. A WebP or AVIF variant of a JPEG upload can never be better than a recompressed q90 JPEG.
A lossless intermediate (PNG) fixes the quality side. Better still, see the note on L195: the disk round trip is avoidable entirely.
|
|
||
| try { | ||
| foreach ($plan->variants as $plannedVariant) { | ||
| $resizedPath = $workspace->path(\sprintf('resized-%d.%s', ++$index, pathinfo($originalPath, \PATHINFO_EXTENSION) ?: 'jpeg')); |
There was a problem hiding this comment.
The intermediate inherits the source extension, and resize() then encodes it with the source's codec (see L382-389).
The round trip through disk also buys nothing here: $encodingImage on L208 immediately decodes the file back into memory to loop over $plan->formats. Having resize() expose a variant that returns the GdImage would remove one encode and one decode per variant, and would make the lossy-intermediate problem disappear along the way.
| throw new \Symfony\UX\Image\Exception\InvalidArgumentException(\sprintf('Unsafe image attribute name "%s".', $name)); | ||
| } | ||
| if (\in_array(strtolower($name), ['src', 'srcset', 'width', 'height', 'loading', 'fetchpriority', 'decoding'], true)) { | ||
| throw new \Symfony\UX\Image\Exception\InvalidArgumentException(\sprintf('Image attribute "%s" is managed by ImageRenderOptions.', $name)); |
There was a problem hiding this comment.
Blocking: class, alt and sizes are renderer-owned but not reserved here.
The list covers src, srcset, width, height, loading, fetchpriority and decoding, but the renderer also emits class, alt and sizes itself. Passing any of those through attributes produces duplicate attributes:
<img src="/up/a_sm.jpg" alt="x" loading="lazy" ... class="rounded" class="shadow" id="hero" />
<img src="/up/a_sm.jpg" alt="x" ... sizes="100vw" ... alt="other" sizes="50vw" />That is invalid HTML, and the browser keeps the first occurrence, so the caller's value is silently dropped. The PR description promises "safe application attributes without allowing overrides of renderer-owned attributes", which currently holds for 7 attributes out of 10.
| public function toHtml(): string | ||
| { | ||
| $sizes = null !== $this->options->sizes && '' !== $this->options->sizes | ||
| ? \sprintf(' sizes="%s"', htmlspecialchars($this->options->sizes, \ENT_QUOTES)) |
There was a problem hiding this comment.
Blocking: passing \ENT_QUOTES explicitly drops ENT_SUBSTITUTE.
Since PHP 8.1 the default flags are ENT_QUOTES | ENT_SUBSTITUTE | ENT_HTML401. Passing ENT_QUOTES alone removes the substitution, and htmlspecialchars() then returns an empty string on invalid UTF-8.
Verified: rendering with alt: "Caf\xE9 crème" emits alt="". Not an injection, but the accessibility text disappears with no error and no log, and mis-encoded strings are common when the value comes from a legacy import or a latin1 column.
\ENT_QUOTES | \ENT_SUBSTITUTE restores the intended behaviour. This applies to every htmlspecialchars() call in the file.
| sizes: $this->sizes, | ||
| alt: $this->alt ?? '', | ||
| lazy: $this->lazy, | ||
| fetchPriority: $this->fetchpriority ?? ($this->lazy ? 'auto' : 'high'), |
There was a problem hiding this comment.
Suggestion: two entry points, two defaults. The component sets fetchpriority: high when lazy: false, while ux_image() / ux_picture() stay on auto in the same situation (ImageRuntime::renderConfigured()).
The implicit behaviour is defensible, it just should not depend on which API the user picked.
|
|
||
| $errors = []; | ||
| foreach (array_unique(['default_public', ...array_keys($this->storages)]) as $storageName) { | ||
| $path = new StoragePath('.ux-image-validation/'.bin2hex(random_bytes(12)).'.probe'); |
There was a problem hiding this comment.
Suggestion: worth documenting that ux:image:validate writes real objects into every configured storage, including a production bucket, then deletes them.
The .ux-image-validation/ directory is also left behind after the probe file is removed.
| The documentation is organized as a navigable, task-oriented corpus instead of | ||
| one monolithic page: | ||
|
|
||
| * `Installation and quick start <installation.md>`_ |
There was a problem hiding this comment.
These use RST external-link syntax pointing at .md files, and .symfony.bundle.yaml declares doc_dir: doc, so symfony.com would build an index of dead links.
You already flagged the Markdown to RST conversion as pending, just worth gating the docs publication on it.
| return null; | ||
| } | ||
|
|
||
| foreach (['avif', 'webp', 'jpeg', 'jpg', 'png'] as $preferred) { |
There was a problem hiding this comment.
Suggestion: this hardcodes [avif, webp, jpeg, jpg, png] and ignores the configurable ux_image.preferred_formats (and the per-profile override). An application that changes the preference order does not affect this method, which DefaultImageRenderer::resolveDimensions() relies on.
| ...$attributes, | ||
| \sprintf('alt="%s"', htmlspecialchars($this->options->alt, \ENT_QUOTES)), | ||
| \sprintf('loading="%s"', $this->loadingAttribute()), | ||
| \sprintf('fetchpriority="%s"', $this->fetchPriorityAttribute()), |
There was a problem hiding this comment.
Suggestion: fetchpriority="auto" and decoding="async" are emitted on every image even at their default values. Both are the browser default, so it is bytes on every page for no behavioural change.
| if (null !== $position && '' === trim($position)) { | ||
| throw new Exception\InvalidArgumentException('Image variant position must not be empty.'); | ||
| } | ||
| if (null !== $density && 1 !== preg_match('/^(?:[1-9]\d*(?:\.\d+)?x)$/', $density)) { |
There was a problem hiding this comment.
The density regex [1-9]\d*... rejects values below 1 like 0.5x, which are valid per the HTML spec (§ 4.8.4.2.1: "a valid floating-point number giving a number greater than zero").
| return $this->render($asset, new ImageRenderOptions( | ||
| sizes: \is_string($sizes) ? $sizes : null, | ||
| alt: \is_string($alt) ? $alt : '', | ||
| lazy: \is_bool($lazy) ? $lazy : true, | ||
| fetchPriority: \is_string($fetchPriority) ? $fetchPriority : 'auto', | ||
| class: \is_string($class) ? $class : '', | ||
| decoding: \is_string($decoding) ? $decoding : 'async', | ||
| variant: \is_string($variant) ? $variant : null, | ||
| )); |
There was a problem hiding this comment.
renderConfigured() extracts 7 of the 9 ImageRenderOptions parameters but doesn't forward srcset or attributes. The Twig functions ux_image() / ux_picture() silently ignore those options with no error, while the Twig component <twig:ux:image> does support srcset. Either forward them or document the limitation explicitly.
4 participants
So I had the intention to open the Upload PR first ... but #3765 kinda changed my plan :)
I don't think it's a question of "one or the other one", and let's see what feature, DX, etc both PR bring to the table, and what we can build from there.
I'll write a longer description tomorrow, but I didn't want to let too much time pass before opening this.
/!\ Review warning: this is a big one...
TL;DR;
UX Image turns one uploaded raster image into durable responsive-image metadata, then renders native
<picture>and<img>markup without storage I/O or JavaScript.Applications keep ownership of uploads and entities while the bundle provides one explicit processing, storage and rendering contract.
UX Image
Process the image once:
Render the persisted value anywhere:
{{ ux_picture(product.image, { alt: product.name, lazy: false, fetchpriority: 'high', }) }}The browser receives native responsive markup and selects the appropriate format, width and density. Rendering uses the persisted
ImageAssetmetadata only: it does not reopen the source image or contact storage.Objectives
UX Image provides a Symfony-native image pipeline with a deliberately small application boundary:
ImageAssetvalue that remains independent from entities and storage implementations;Pipeline
flowchart LR A["UploadedFile"] --> B["Inspect real bytes"] B --> C["Named profile"] C --> D["Generate variants"] D --> E["Local or Flysystem storage"] E --> F["Immutable ImageAsset"] F --> G["Application persistence"] G --> H["ux_picture() or ux_image()"] H --> I["Native responsive HTML"]Processing and rendering are separate operations. The expensive work happens when the application accepts the image; ordinary page rendering reads only persisted metadata.
Features
ImageAsset, JSON serialization contract and optional Doctrine DBAL type;ux_picture(),ux_image()and an optional Twig Component producesrcset,sizes, intrinsic dimensions, media conditions and loading hints;Out of Scope
These boundaries are intentional. UX Upload, Symfony Forms or application code can provide the input; UX Image starts when the application has an authorized
UploadedFileto process.Requirements
Required Dependencies
The package also uses Symfony Config, Console, DependencyInjection, Filesystem, HttpFoundation and HttpKernel, plus PSR-6 cache contracts.
Optional Dependencies
league/flysystem ^3.0doctrine/dbal ^4.0image_assetJSON persistence typeintervention/image ^3.0intervention/image-driver-vipssymfony/ux-twig-component ^3.0<twig:ux:image>componentOptional integrations are registered only when their concrete dependency is available. The core value objects, renderer contracts and local storage do not require them.
Usage
Define the outputs required by the layout:
Process the real uploaded file after application authorization:
The application persists the returned
ImageAssetwith its owning model. It can use its own mapping or the optional Doctrine DBAL type.Configuration
The default configuration works with local storage and GD. Applications add named profiles and storages as their layouts and deployment require:
Profiles select transformation behavior. Storage is selected at processing time and recorded in the resulting asset, so the same profile can publish to different storage backends.
Processing modes are explicit:
immediatewrites the original and variants before returning;deferredstores an asset that can be completed later;asyncdelegates dispatch to an application implementation ofImageProcessingDispatcherInterface.The bundle does not invent an application message, owner identifier or persistence transaction for asynchronous work.
Rendering
ux_picture()performs format negotiation through<source>elements and keeps a JPEG, PNG or original fallback in<img>.ux_image()renders a single native<img>when format negotiation is unnecessary.Both functions support:
srcset, profile-levelsizesand density descriptors;The optional
<twig:ux:image>component delegates to the same renderer. There is one rendering contract, not a second component-specific implementation.Storage and Persistence
ImageAssetstores paths, dimensions, MIME information, variants, profile name and profile revision. It stores no resolved public URL. This keeps persisted data portable when a CDN hostname or URL strategy changes.Storage publication uses immutable generation keys. A failed processing run removes only the new objects it created; an existing published generation is never overwritten. The application makes the database update durable before deleting an older generation.
Rendering never checks storage existence. The persisted asset and its stored objects therefore form an application consistency boundary.
Security
Images are untrusted binary input. Security is applied before processing, while writing outputs and when generating public markup.
Input Files
Processing and Storage
Authorization and Delivery
The application authorizes the owner and tenant before calling
process(). It must never accept anImageAssetJSON document or storage path directly from a client.Public URL prefixes and CDN builders provide addressing, not access control. Private originals require private storage and an application-owned signed URL adapter or controller.
Regeneration
Regeneration starts from application persistence, never from a storage scan. Applications implement:
ImageAssetProviderInterfaceto expose bounded, stably ordered batches;ImageAssetPersisterInterfaceto publish each replacement with application-level compare-and-swap semantics.php bin/console ux:image:regenerate product \ --storage=default_public \ --batch-size=100The command supports dry runs, current-revision skipping, forced regeneration and resumable opaque cursors. The provider and persister keep entity knowledge and transactions where they belong: in the application.
Tests
Documentation
The documentation follows the image lifecycle from profile design to production operation:
overview.mdinstallation.mdprocessing.mdimage-asset.mdrendering.mdstorage.mdregeneration.mdintegrations.mdsecurity.mdconfiguration.mdtesting.mddebugging.mdarchitecture.mdThe Markdown pages will be converted to reStructuredText before the documentation is submitted for publication.
Coming Next
Separate pull requests can provide: