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

Update i18nTextCollector to collect strings also from themes #10685

Merged
merged 1 commit into from May 9, 2023

Conversation

xini
Copy link
Contributor

@xini xini commented Feb 13, 2023

Fixes #8491

@michalkleiner michalkleiner changed the title fix i18nTextCollector to collect strings in themes, fixes #8491 Fix i18nTextCollector to collect strings in themes Feb 13, 2023
@michalkleiner michalkleiner changed the title Fix i18nTextCollector to collect strings in themes Update i18nTextCollector to collect strings also from themes Feb 13, 2023
michalkleiner
michalkleiner previously approved these changes Feb 13, 2023
Copy link
Contributor

@michalkleiner michalkleiner left a comment

Choose a reason for hiding this comment

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

Looks ok from my perspective, thanks for the tweaks!

I'm not too fussed if this particular PR goes to 4.12 as a bugfix or to 4 as an enhancement, what do you think @GuySartorelli?

P.S. Squash will be needed

@michalkleiner
Copy link
Contributor

I'm not too fussed if this particular PR goes to 4.12 as a bugfix or to 4 as an enhancement, what do you think @GuySartorelli?

Based on Guy's comment on #10686 I suggest this also targets 4. Then we can get both in as enhancements for 4.13 which is not too far away.

@xini xini changed the base branch from 4.12 to 4 February 13, 2023 23:30
@xini
Copy link
Contributor Author

xini commented Feb 13, 2023

rebased and squashed.

@xini
Copy link
Contributor Author

xini commented Feb 15, 2023

the tests didn't trigger when I rebased this. can you please trigger them manually?

@GuySartorelli
Copy link
Member

There's no way that I'm aware of to trigger tests manually on a PR. Try pushing to the branch again.

src/i18n/TextCollection/i18nTextCollector.php Outdated Show resolved Hide resolved
src/i18n/TextCollection/i18nTextCollector.php Outdated Show resolved Hide resolved
src/i18n/TextCollection/i18nTextCollector.php Outdated Show resolved Hide resolved
src/i18n/TextCollection/i18nTextCollector.php Outdated Show resolved Hide resolved
@GuySartorelli
Copy link
Member

Sorry, I haven't had time to look at this again - at the very least this will need to target the 5 branch and be rebased. Can you please do that? I'll try make time to look at this properly over the next couple of weeks.

@xini xini changed the base branch from 4 to 5 April 12, 2023 01:18
@xini
Copy link
Contributor Author

xini commented Apr 12, 2023

Thanks @GuySartorelli. This is now rebased and squashed.

Copy link
Member

@GuySartorelli GuySartorelli left a comment

Choose a reason for hiding this comment

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

Please also add some unit tests to validate that this new functionality actually collects translations from themes as expected.

Sorry there are so many changes I didn't mention last time - I should have caught these the first time around.

src/i18n/Messages/YamlWriter.php Outdated Show resolved Hide resolved
src/i18n/TextCollection/i18nTextCollector.php Show resolved Hide resolved
src/i18n/TextCollection/i18nTextCollector.php Outdated Show resolved Hide resolved
src/i18n/TextCollection/i18nTextCollector.php Outdated Show resolved Hide resolved
src/i18n/TextCollection/i18nTextCollector.php Outdated Show resolved Hide resolved
@GuySartorelli
Copy link
Member

I've created silverstripe/developer-docs#263 which will document this change.

@GuySartorelli GuySartorelli dismissed michalkleiner’s stale review May 8, 2023 23:02

There have been a lot of changes since then

@xini xini force-pushed the fix-i18n-collect-themes branch from c8ddf88 to 4d48de3 Compare May 8, 2023 23:03
@xini
Copy link
Contributor Author

xini commented May 9, 2023

Hey @GuySartorelli, I think this is now all cleaned up I have added a test as well. Let me know if you're happy with this and I'll squash the commits. Thank you!

@GuySartorelli
Copy link
Member

That's awesome, thank you! I haven't run it locally yet which I'll do before merging, but given the test is passing I'm pretty confident it works. The code looks good from a visual-inspection. Please go ahead and squash it, I'll try to make time either later today or tomorrow to run it locally before merging.

@GuySartorelli GuySartorelli dismissed their stale review May 9, 2023 01:44

Changes have been made

@xini xini force-pushed the fix-i18n-collect-themes branch from 4759e61 to 04266f7 Compare May 9, 2023 01:57
@xini
Copy link
Contributor Author

xini commented May 9, 2023

Done, thank you!

Copy link
Member

@GuySartorelli GuySartorelli left a comment

Choose a reason for hiding this comment

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

Works great locally, thank you! Between this and the ORM change, this has been a significant contribution.

@GuySartorelli GuySartorelli merged commit 7210ac8 into silverstripe:5 May 9, 2023
9 checks passed
@xini
Copy link
Contributor Author

xini commented May 9, 2023

Thank you! It has been bugging me since 4.0, when the themes functionality was removed... 🙃 And with a few non-english projects over the last few months I finally had enough... 😉

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.

i18nTextCollector not collecting from themes
3 participants