VichFileType : Fix translation for 'delete_label' when 'download_label' is a callable #1573

[theredled]

In VichFileType, when download_label is either a callable or true, delete_label translation domain is affected, breaking the label. Added separate 'translation_domain' options for each label (download_label_translation_domain, delete_label_translation_domain)
#1573

[garak]

You can rebase to get the PHP-CS-Fixer errors fixed.
The rest of the failures are up to you.

[theredled]

Should be ok!
I also added 'translation_domain' option back in VichFileType for full backwards-compatibility

[theredled]

Sorry, changes got lost between repos locally, I should have re-run tests before pushing.

Fixed.

[Copilot]

Pull request overview

Fixes translation-domain interference in VichFileType when download_label is callable/true by separating translation-domain handling for the download link vs. the delete checkbox label (issue #1573).

Changes:

• Added per-label translation domain options: download_label_translation_domain and delete_label_translation_domain.
• Updated download-label resolution to avoid clobbering the form’s translation_domain.
• Updated the form theme and adjusted tests to expect the new download-label domain key.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 4 comments.

File	Description
src/Form/Type/VichFileType.php	Introduces new options and updates download/delete label translation-domain wiring.
templates/Form/fields.html.twig	Switches download link translation to use download_label_translation_domain.
tests/Form/Type/VichFileTypeTest.php	Updates expectations for the renamed download-label translation domain key.

Comments suppressed due to low confidence (1)

tests/Form/Type/VichFileTypeTest.php:214

• The updated expectations cover cases where download_label_translation_domain is returned by resolveDownloadLabel, but there’s no test for the new explicit option (download_label_translation_domain in the field options) being propagated to the view/template when download_label is a plain string. Adding such a case would help catch the current regression where the widget may ignore the configured domain/false value unless it comes from the callable/true branches.

                $object,
                [
                    'download_label' => true,
                    'download_uri' => true,
                    'asset_helper' => false,
                ],
                [
                    'object' => $object,
                    'download_label' => 'image.jpeg',
                    'download_label_translation_domain' => false,
                    'download_uri' => 'resolved-uri',
                    'value' => null,
                    'attr' => [],
                    'asset_helper' => false,
                ],

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[theredled]

Note: added a $options parameter to signature of VichFileType::resolveDownloadLabel (required to access the configuration)

[Copilot]

Pull request overview

Copilot reviewed 5 out of 5 changed files in this pull request and generated 3 comments.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

In the callable download_label branch, $result can be a plain string (as covered by VichFileTypeTest), but the code still attempts $result['download_label'] / $result['download_label_translation_domain'] / $result['translation_domain']. Accessing array offsets on a string triggers PHP warnings (and can be a TypeError depending on PHP version/settings). Consider branching on is_array($result) (or instanceof \ArrayAccess) and: (1) for scalar results, set download_label to the scalar and download_label_translation_domain to $options['download_label_translation_domain'] ?? $options['translation_domain']; (2) for array results, read keys safely and fall back to the same options when the callable doesn’t specify a domain.

[theredled]

Wrong : isset / ?? never trigger warnings for non-existent variables, and also it's not a regression, as I only adopted the same codestyle as existing code.
@garak : agree?

[garak]

@copilot do you agree with the comment above?

[theredled]

There seems to be an issue with PHPStan when you create a mock from a final class: phpstan/phpstan#8901

For some reason that issue was present but not detected in master branch
Only thing I've done was moving the methods that were creating mocks from final classes (PropertyMapping, PropertyMappingFactory, MetadataReader, UploadHandler) from Vich\UploaderBundle\Tests\TestCase to new trait Vich\UploaderBundle\Tests\TestCaseTrait, and they're now triggering errors from PHPStan

Don't know what to do about it.

[theredled]

Ok I got it, the error was already happening but it was manually excluded in phpstan-baseline.neon for TestCase.php.

Doing the same for TestCaseTrait.php.

[theredled]

Thanks!