Skip to content
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

FIX allows i18nTextCollector to concatenate keys #824

Merged

Conversation

davejtoews
Copy link
Contributor

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

fixes tractorcow-farm#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
@davejtoews
Copy link
Contributor Author

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
Copy link
Collaborator

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.'

Copy link
Collaborator

@tractorcow tractorcow left a comment

Choose a reason for hiding this comment

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

To investigate possible regression in text collector

@tractorcow
Copy link
Collaborator

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

@davejtoews
Copy link
Contributor Author

davejtoews commented Feb 8, 2024

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
Copy link
Collaborator

tractorcow commented Feb 8, 2024

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
Copy link
Contributor Author

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

@davejtoews
Copy link
Contributor Author

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
Copy link
Contributor

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

@tractorcow
Copy link
Collaborator

Can be merged when tests pass.

@davejtoews
Copy link
Contributor Author

@GuySartorelli It looks like this may have been fixed in framework 5 but not applied to 4? silverstripe/silverstripe-framework@f4e0c76

@GuySartorelli
Copy link
Contributor

@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.

@tractorcow tractorcow merged commit 42ecdc3 into tractorcow-farm:6 Feb 13, 2024
10 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants