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 i18nTextCollector merge does not work with vendor modules #9720

Merged
merged 2 commits into from Jan 16, 2022
Merged

Fix i18nTextCollector merge does not work with vendor modules #9720

merged 2 commits into from Jan 16, 2022

Conversation

bimthebam
Copy link
Contributor

While running the i18nTextCollector against a vendor-module - with merge option enabled - it won't merge with existing files due to a wrong module-path determination.

i18nTextCollector should use the ModuleLoader to determine the correct module-path.

@dhensby
Copy link
Contributor

dhensby commented Oct 5, 2020

This looks reasonable - is there any existing test coverage that could be modified so that this regression could not happen again (without a test failing)?

@michalkleiner
Copy link
Contributor

Is this somehow related to #10179?

@emteknetnz
Copy link
Member

Related easy PR to merge silverstripe/silverstripe-asset-admin#1249

Copy link
Member

@emteknetnz emteknetnz left a comment

Choose a reason for hiding this comment

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

Have tested locally, this works as intended

Note: the code when this PR was originally raised lacked /vendor/ where $masterFile was defined. This was since fixed here.

This PR is functionally the same as the existing code, just now it's less hardcoded

@emteknetnz emteknetnz merged commit c6ee8c0 into silverstripe:4 Jan 16, 2022
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.

None yet

5 participants