Skip to content

[Translator] Revert Fix changing dump directory using AssetMapper #1913

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Conversation

maelanleborgne
Copy link
Contributor

@maelanleborgne maelanleborgne commented Jun 12, 2024

Q A
Bug fix? yes
New feature? no
Issues #1904 (comment)
License MIT

This PR introduced a bug : when using 'var/translations' in the importmap.php file instead of a relative path, it would break the twig importmap() function.

This PR aims to rectify this regression by keeping a 'var/translations' namespace pointing to the dump_directory. If this is not accepted, the previous PR should be reverted to prevent users using 'var/translations' in there importmap.php to face this issue, but the documentation should still be updated to display an example using the relative path instead of a misleading namespace.

This PR reverts the change. A word in the doc to say that the dump_directory conf is not compatible with asset_mapper would be nice.

@carsonbot carsonbot added Bug Bug Fix Status: Needs Review Needs to be reviewed labels Jun 12, 2024
@smnandre smnandre requested a review from kbond June 12, 2024 14:02
Comment on lines 51 to 52
$config['dump_directory'] => 'var/translations', // Keeping for BC, see https://github.com/symfony/ux/pull/1904#issuecomment-2162066941
$config['dump_directory'] => '@app/translations',
Copy link
Member

@Kocal Kocal Jun 12, 2024

Choose a reason for hiding this comment

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

You will only have 'var/translations' or '@app/translations' in the array, since the key is the same for both values

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You're right, when we tested it worked because we had it the other way around.
There's no way to fix this because even with

asset_mapper' => [
                'paths' => [
                    __DIR__.'/../../assets/dist' => '@symfony/ux-translator',
                    $config['dump_directory'] => '@app/translations',
                    '%kernel.project_dir%/var/translations' => 'var/translations',
                ],
            ],

the keys will be the same when running the default conf.

@maelanleborgne maelanleborgne force-pushed the feature/translator-allow-custom-dump-dir-asset-mapper branch from 4373448 to e551cff Compare June 12, 2024 15:30
@maelanleborgne maelanleborgne changed the title [Translator] Fix breaking link to hardcoded namespace 'var/translations' [Translator] Revert Fix changing dump directory using AssetMapper Jun 12, 2024
Copy link
Member

@smnandre smnandre left a comment

Choose a reason for hiding this comment

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

Yep and it happens don't worry...

(some even say only the best of the best do this 😇 😆 )

... and we should have been more attentive.

Thank you :)

@carsonbot carsonbot added Status: Reviewed Has been reviewed by a maintainer and removed Status: Needs Review Needs to be reviewed labels Jun 12, 2024
@kbond
Copy link
Member

kbond commented Jun 12, 2024

Thank you @maelanleborgne.

@kbond kbond merged commit 18a18d1 into symfony:2.x Jun 12, 2024
36 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Bug Fix Status: Reviewed Has been reviewed by a maintainer
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants