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

Drush - Import multiple custom translation po files #4251

Open
wants to merge 1 commit into
base: 10.x
Choose a base branch
from

Conversation

Sergey-Orlov
Copy link
Contributor

This adds new drush command to import multiple custom translation po files from directory, as described in #4235

@Sergey-Orlov Sergey-Orlov force-pushed the import-multiple-translation-files-4235 branch 3 times, most recently from ba3d5d8 to c2d5761 Compare November 14, 2019 05:57
@Sergey-Orlov Sergey-Orlov force-pushed the import-multiple-translation-files-4235 branch from c2d5761 to 5b52a55 Compare November 14, 2019 06:01
@weitzman
Copy link
Member

weitzman commented Dec 3, 2019

We are lacking a locale maintainer at the moment so I apologize that nobody from Drush core team is reviewing this. We need a review from outside the team, as next step.

@alexkras
Copy link

Reviewed and tested on real project, all works fine.
Can be merged as-is.

@alexkras
Copy link

@weitzman what need to do to speed up merge of this changes? thx.

@weitzman
Copy link
Member

Thanks for the review.

I'm hesitant to add more locale functionality while we have no maintainer for it. Ideally someone maintains these commands outside of Drush core for now.

@andypost
Copy link
Contributor

I'd say it needs tests and then it could be a locale console command patch in core

@greg-1-anderson greg-1-anderson changed the base branch from master to 10.x August 2, 2020 22:57
@davidferlay
Copy link

FYI we have been using this patch in production for years now without any issue

@weitzman weitzman requested a review from Sutharsan April 9, 2021 10:30
@DieterHolvoet
Copy link
Contributor

Maybe we can consider making the directory argument an option? That way, you could set a sitewide default directory through a Drush config file. If you guys are on board with this idea I wouldn't mind implementing it.

@davidferlay
Copy link

Hi @DieterHolvoet ,
Making the directory argument an option would require to have a default value as fallback (because user can then omit this input, as it is optional). So i'd say, why not providing we figure out a meaningful default value for it.

WDYT ?

@DieterHolvoet
Copy link
Contributor

Or we could add an extra check in code to make sure the option is present? I would probably use ../translations, relative to the Drupal docroot. In that case we should also support passing relative paths.

@davidferlay
Copy link

Just made a bit of research and discovered that "option" may not necessarily be "optional" (https://pubs.opengroup.org/onlinepubs/9699919799/basedefs/V1_chap12.html#tag_12_02)

We could add an extra check in code to make sure the option is present

++ as long the helper reflects it

We should also support passing relative paths

I agree that this would be a welcome addition

@weitzman
Copy link
Member

The the decision about merging and maintaining this falls to the locale maintainer, @Sutharsan. This PR would need tests and needs to target 11.x branch.

@DieterHolvoet
Copy link
Contributor

Okay, before I continue work on this PR I'll wait for their go ahead.

andypost added a commit to skilld-labs/drush that referenced this pull request May 17, 2023
andypost added a commit to skilld-labs/drush that referenced this pull request May 17, 2023
Sutharsan added a commit that referenced this pull request Aug 23, 2023
…5569)

* Add locale:import:all - imports multiple custom translation po files

Supersedes #4251

* Fix feedbacks

* Fix feedbacks

* Apply suggestions from code review

Co-authored-by: Erik Stielstra <info@erikstielstra.nl>

---------

Co-authored-by: Sergey-Orlov <sergey.kam.orlov@gmail.com>
Co-authored-by: Erik Stielstra <info@erikstielstra.nl>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants