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

andypost · 2023-05-17T16:22:49Z

Supersedes #4251

Supersedes drush-ops#4251

Sutharsan

Initial review of the code. I did not test the command yet.

~~Note: Performing the same import with locale:import is possible but requires bash scripting.~~

src/Commands/core/LocaleCommands.php

weitzman · 2023-06-02T14:49:12Z

@Sutharsan Is all feedback addressed?

Sutharsan · 2023-06-08T20:54:36Z

src/Commands/core/LocaleCommands.php

+        $files = [];
+        foreach ($poFiles as $file) {
+            // Ensure we have the file intended for upload.
+            if (file_exists($file)) {


The other if conditions in this foreach loop use continue to skip processing the current $file. I agree with using this code style (early return). I'd like to use it consistently and apply it to this condition too, and remove these nested if conditions. With this being done, the comment 'Ensure we have the file ...' can go away. It is pretty obvious what is going on here.

Sutharsan · 2023-06-11T12:08:20Z

I have tested the code and all works as expected.

@weitzman If you provide an array of command aliases, drush shows all aliases except the first. Is this intended behavior? (I see the same behavior with existing commands)
Since this is a new command why should we provide aliases? No BC required, no short code.

The command code:

#[CLI\Command(name: self::IMPORT_ALL, aliases: ['locale-import-all', 'locale:import:all'])]

The output of drush list:

locale:                                                                                                                        
  locale:check                                 Checks for available translation updates.                                       
  locale:clear-status                          Clears the translation status.                                                  
  locale:export                                Exports to a gettext translation file.                                          
  locale:import                                Imports to a gettext translation file.                                          
  locale:import-all (locale:import:all)        Imports multiple translation files from the defined directory.                  
  locale:update                                Imports the available translation updates.

weitzman

Yeah, we should omit "dash" aliases for new commands.

weitzman · 2023-06-12T00:17:34Z

src/Commands/core/LocaleCommands.php

+    #[CLI\Usage(name: 'drush locale:import-all /var/www/translations', description: 'Import all translations from the defined directory (non-recursively). Supported filename patterns are: {project}-{version}.{langcode}.po, {prefix}.{langcode}.po or {langcode}.po.')]
+    #[CLI\Usage(name: 'drush locale:import-all /var/www/translations/custom --types=customized --override=all', description: 'Import all custom translations from the defined directory (non-recursively) and override any existing translation. Supported filename patterns are: {project}-{version}.{langcode}.po, {prefix}.{langcode}.po or {langcode}.po.')]
+    #[CLI\ValidateModulesEnabled(modules: ['locale'])]
+    public function importAll($directory, $options = ['type' => self::OPT, 'override' => self::OPT])


Suggested change

public function importAll($directory, $options = ['type' => self::OPT, 'override' => self::OPT])

public function importAll($directory, $options = ['type' => self::REQ, 'override' => self::REQ])

The meaning of REQ is that a value must be supplied when the option is used.

weitzman · 2023-06-12T00:19:29Z

src/Commands/core/LocaleCommands.php

+     */
+    #[CLI\Command(name: self::IMPORT_ALL, aliases: ['locale-import-all', 'locale:import:all'])]
+    #[CLI\Argument(name: 'directory', description: 'The path to directory with translation files to import.')]
+    #[CLI\Option(name: 'type', description: 'String types to include, defaults to <info>not-customized</info>. Recognized values: <info>not-customized</info>, <info>customized</info>')]


Use suggestionValues param as per

drush/src/Commands/config/ConfigCommands.php

Line 146 in 166af7a

#[CLI\Option(name: 'input-format', description: 'Format to parse the object. Recognized values: <info>string</info>, <info>yaml</info>. Since JSON is a subset of YAML, $value may be in JSON format.', suggestedValues: ['string', 'json'])]

https://github.com/weitzman/drush/blob/166af7acb72ad256cbc8c33fba58f5981683cd5a/src/Commands/config/ConfigCommands.php#L146. Helps with shell completion.

done, for override option as well

weitzman · 2023-06-12T11:26:03Z

If you provide an array of command aliases, drush shows all aliases except the first. Is this intended behavior?

Looks like a bug. I think we should show only the first alias. Otherwise the first column gets too wide.

weitzman · 2023-06-12T11:50:52Z

Actually ListCommands hides 'dash' aliases.

weitzman

LGTM. @Sutharsan will merge when satisfied.

Sergey-Orlov · 2023-08-21T07:28:34Z

rebased forked branch, so all checks should be fine now

Sutharsan

I found two sentences that need some improvement.

src/Commands/core/LocaleCommands.php

Sutharsan · 2023-08-21T19:39:33Z

Once this is fixed, I will merge the PR.

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

Sergey-Orlov · 2023-08-22T03:12:10Z

Once this is fixed, I will merge the PR.

Fixed and rebased again

Sutharsan · 2023-08-23T06:10:12Z

Thanks all for working on this.

andypost requested review from weitzman and greg-1-anderson as code owners May 17, 2023 16:22

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

a7908d9

Supersedes drush-ops#4251

andypost force-pushed the locale-drush-12 branch from dfe1215 to a7908d9 Compare May 17, 2023 16:34

Sutharsan requested changes May 17, 2023

View reviewed changes

Sergey-Orlov added 2 commits May 19, 2023 13:07

Merge branch 'drush-ops:12.x' into locale-drush-12

e8d31b5

Fix feedbacks

422394d

weitzman added the area-locale label Jun 2, 2023

Sutharsan reviewed Jun 8, 2023

View reviewed changes

weitzman reviewed Jun 12, 2023

View reviewed changes

Fix feedbacks

c1cce04

Sergey-Orlov force-pushed the locale-drush-12 branch from 293a922 to c1cce04 Compare June 15, 2023 05:07

weitzman approved these changes Jul 6, 2023

View reviewed changes

Merge branch 'drush-ops:12.x' into locale-drush-12

692118e

Sutharsan requested changes Aug 21, 2023

View reviewed changes

src/Commands/core/LocaleCommands.php Outdated Show resolved Hide resolved

src/Commands/core/LocaleCommands.php Outdated Show resolved Hide resolved

Apply suggestions from code review

8d8b60a

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

Sergey-Orlov added 2 commits August 22, 2023 18:22

Merge branch 'drush-ops:12.x' into locale-drush-12

a450cad

Merge branch '12.x' into locale-drush-12

ea935dd

Sutharsan approved these changes Aug 23, 2023

View reviewed changes

Sutharsan merged commit 6ab5d13 into drush-ops:12.x Aug 23, 2023
2 checks passed

andypost deleted the locale-drush-12 branch December 17, 2023 14:44

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

andypost commented May 17, 2023

Sutharsan left a comment •

edited

weitzman commented Jun 2, 2023

Sutharsan Jun 8, 2023 •

edited

Sergey-Orlov Jun 15, 2023

Sutharsan commented Jun 11, 2023

weitzman left a comment

weitzman Jun 12, 2023

Sergey-Orlov Jun 15, 2023

weitzman Jun 12, 2023

Sergey-Orlov Jun 15, 2023 •

edited

weitzman commented Jun 12, 2023

weitzman commented Jun 12, 2023 •

edited

weitzman left a comment

Sergey-Orlov commented Aug 21, 2023

Sutharsan left a comment

Sutharsan commented Aug 21, 2023

Sergey-Orlov commented Aug 22, 2023 •

edited

Sutharsan commented Aug 23, 2023

	public function importAll($directory, $options = ['type' => self::OPT, 'override' => self::OPT])
	public function importAll($directory, $options = ['type' => self::REQ, 'override' => self::REQ])

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

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

Conversation

andypost commented May 17, 2023

Sutharsan left a comment • edited

Choose a reason for hiding this comment

weitzman commented Jun 2, 2023

Sutharsan Jun 8, 2023 • edited

Choose a reason for hiding this comment

Sergey-Orlov Jun 15, 2023

Choose a reason for hiding this comment

Sutharsan commented Jun 11, 2023

weitzman left a comment

Choose a reason for hiding this comment

weitzman Jun 12, 2023

Choose a reason for hiding this comment

Sergey-Orlov Jun 15, 2023

Choose a reason for hiding this comment

weitzman Jun 12, 2023

Choose a reason for hiding this comment

Sergey-Orlov Jun 15, 2023 • edited

Choose a reason for hiding this comment

weitzman commented Jun 12, 2023

weitzman commented Jun 12, 2023 • edited

weitzman left a comment

Choose a reason for hiding this comment

Sergey-Orlov commented Aug 21, 2023

Sutharsan left a comment

Choose a reason for hiding this comment

Sutharsan commented Aug 21, 2023

Sergey-Orlov commented Aug 22, 2023 • edited

Sutharsan commented Aug 23, 2023

Sutharsan left a comment •

edited

Sutharsan Jun 8, 2023 •

edited

Sergey-Orlov Jun 15, 2023 •

edited

weitzman commented Jun 12, 2023 •

edited

Sergey-Orlov commented Aug 22, 2023 •

edited