tractorcow-farm / silverstripe-fluent

Multi-language translate module for Silverstripe, without having to manage separate site trees.
BSD 3-Clause "New" or "Revised" License
91 stars 109 forks source link

FIX allows i18nTextCollector to concatenate keys #824

Closed davejtoews closed 6 months ago

davejtoews commented 7 months ago

Description

fixes #823 - The magic constant TRAIT was inserting namespaces into localization keys, containing slashes that could not be parsed by the TextCollector task. These have been replaced w/ the Trait name as a string

Manual testing steps

Run /dev/tasks/i18nTextCollectorTask with the environment set to Dev or error reporting otherwise configured to display warnings.

Issues

823

davejtoews commented 7 months ago

I wasn't 100% sure which branch to submit a PR to patch version 6. Let me know if this belongs on another branch.

tractorcow commented 7 months ago

That's not actually correct, textcollector has always been able to support namespaces with slashes in them. Here is the currently extracted strings.

Are you running an older or outdated version of text collector?

  TractorCow\Fluent\Extension\Traits\FluentAdminTrait:
    ClearAllNotice: 'All localisations have been cleared for ''{title}''.'
    CopyNotice: 'Copied ''{title}'' to all other locales.'
    DeleteNotice: 'Deleted ''{title}'' and all of its localisations.'
    UnpublishNotice: 'Unpublished ''{title}'' from all locales.'
    ArchiveNotice: 'Archived ''{title}'' and all of its localisations.'
    PublishNotice: 'Published ''{title}'' across all locales.'
tractorcow commented 7 months ago

It could also matter maybe what PHP version you're testing it on.

davejtoews commented 7 months ago

Yeah, I after a bit more digging it does appear to not be about the namespaces but something about the __TRAIT magic constant itself. Passing the slashes in as a string works, but not w/ __TRAIT

I'm running Silverstripe Framework 4.13.29 on PHP 8.1

tractorcow commented 7 months ago

Ok, we should be fine to replace __TRAIT__ with the literal, but we probably should keep the slashes otherwise all the existing localisations will need to be migrated. :) Hope that's a reasonable resolution.

davejtoews commented 7 months ago

Yes, I was coming to the conclusion that was the correct solution already but hadn't yet updated the PR. Thanks.

davejtoews commented 7 months ago

Strings updated to include full namespace. Presumably this change needs doing on Fluent v7 as well but I am not running Silverstripe 5 and haven't tested it.

GuySartorelli commented 6 months ago

I honestly think this is being addressed in the wrong place - the textcollector should instead be updated to correctly handle __TRAIT__.

tractorcow commented 6 months ago

Can be merged when tests pass.

davejtoews commented 6 months ago

@GuySartorelli It looks like this may have been fixed in framework 5 but not applied to 4? https://github.com/silverstripe/silverstripe-framework/commit/f4e0c768bdeeb3c2a379320eb219a6020c80ce08

GuySartorelli commented 6 months ago

@davejtoews oh, nice find! __TRAIT__ is well within our support commitments for CMS 4 so if you want to cherry pick that on top of framework 4.13 and raise a PR that'll be an easy merge.