Provide a proper API for updating translations

Looks good to me. Makes sense to API-ify this.

1. +++ b/core/modules/locale/src/TranslationUpdater.php
   @@ -0,0 +1,92 @@
   +    locale_translation_flush_projects();
   

   This could use the locale.project service.

2. +++ b/core/modules/locale/src/TranslationUpdater.php
   @@ -0,0 +1,92 @@
   +    $this->moduleHandler->loadInclude('locale', 'inc', 'locale.compare');
   

   Also this is quite sad (and the includes in ::update() below) - is there an issue we can link to for moving that functionality to a class too?

Address #6.1
#6.2 looks like there is already one. is this is one #1834298 ?

Thanks for the patch! I reviewed the code and noticed a few minor things.

1. +++ b/core/modules/locale/src/TranslationUpdater.php
   @@ -0,0 +1,100 @@
   +    // Check translation status of all translatable project in all languages.
   

   Plural change: project => projects.

2. +++ b/core/modules/locale/src/TranslationUpdater.php
   @@ -0,0 +1,100 @@
   +    // translation updates. If the status is expired we clear it an run a batch to
   

   Typo: an => and.

   Nitpick: Over 80 characters.

3. +++ b/core/modules/locale/src/TranslationUpdater.php
   @@ -0,0 +1,100 @@
   +
   

   Nitpick: Extra line.

4. +++ b/core/modules/locale/src/TranslationUpdaterInterface.php
   @@ -0,0 +1,59 @@
   +   *   $translation_update->check()
   

   Missing semi-colon?

5. +++ b/core/modules/locale/src/TranslationUpdaterInterface.php
   @@ -0,0 +1,59 @@
   +   *   $translation_update->update()
   

   Missing semi-colon?

What is the best way to test this?

I tested the patch on a local site. The check is fine. I'm having a hard time testing the manual update since there aren't new translations since the last update.

One thing I noticed when testing is that if I choose:

Overwrite existing translations.

for:

Import behavior

it does not affect doing a manual update. The description does say it's for "automatic" updates:

How to treat existing translations when automatically updating the interface translations.

but that doesn't seem very intuitive to me. Why should a manual update behave differently than an automatic update?

I was hoping that I could check the update by:

1. Add language (it runs the update)
2. Manually remove some translations
3. Choose "Overwrite existing translations." in the settings
4. Run a manual update
5. The translations I removed would be added back

But since manual vs automatic updates behave differently then I can't test this way. I'm unclear how else I can test without mucking with the database directly since I don't have an D8 site that is old enough to have some missing translations but not old enough that it's not on a way older version of D8.

Thoughts?

Which URL did you run the manual updates from? Which admin page?

I have found the link I used. Go to:

/admin/config/regional/translate/settings

and it has a link with text:

Check updates now

and if you click that it does the check and you end up on:

/admin/reports/translations

which also has a:

Check manually

link that does the same thing. The link is linked to:

/admin/reports/translations/check

I'll see if I can reproduce the issue of manual vs automatic update difference and open a new issue if I can reproduce.

Also, it's possible that this is because there are no updates so it's not changing back my custom translations. If so, then it might apply to both manual and automatic updates. That would be a different issue.

Recap: There appear to be 2 possibilities:

1) The custom translations aren't wiped out for manual updates but are for automatic.

or

2) The custom translations aren't wiped out for manual or automatic updates when there are no new translations from localize.drupal.org.

My feeling now is it's the latter. I'll be doing some testing locally to see if I can figure that out.

+++ b/core/modules/locale/src/TranslationUpdater.php
@@ -0,0 +1,100 @@
+    $options = _locale_translation_default_update_options();
...
+      $batch = locale_translation_batch_update_build(array(), $langcodes, $options);
...
+      $batch = locale_translation_batch_fetch_build($projects, $langcodes, $options);
+      batch_set($batch);
...
+      if ($batch = locale_config_batch_update_components($options, $langcodes)) {
+        batch_set($batch);

a lot of procedures without wrappers

@Kristen: yeah I can imagine it sees the .po files don't need updating and it does not reimport them since they were not updated, therefore not overwriting your customizations even though you wanted that. That sounds very plausible :)

I have confirmed that it was scenario 2 in #17 as I added a bunch of languages locally yesterday, changed the same translations for 3 strings for all languages, and just now ran a manual update. There was an update for Italian and that updated the custom Italian strings but the other languages are unchanged. I guess that kind of makes sense unless you want force getting the latest strings. But, then I guess you can do the manual po files though that's kind of a pain.

The good thing is that it worked so the patch is fine from that perspective. But, now I see #18 so maybe it needs to be updated?

When I had to inject the translation updater into locale's import form I wondered whether that is actually a good place to put the getDefaultOptions() method. It seems there could be a separate translation importer, that the form should use and that the translation updater should inject. What do people think

I love the idea, but probably worth to do it in a follow-up issue.
I would love to have this in core, which we can iterate later.

Nitpicks:

1. +++ b/core/modules/locale/src/TranslationUpdater.php
   @@ -0,0 +1,125 @@
   +/**
   + * @file
   + * Contains \Drupal\locale\TranslationUpdater.
   + */
   
   +++ b/core/modules/locale/src/TranslationUpdaterInterface.php
   @@ -0,0 +1,127 @@
   +/**
   + * @file
   + * Contains \Drupal\locale\TranslationUpdaterInterface.
   + */
   

   We don't want those anymore.

2. +++ b/core/modules/locale/src/TranslationUpdater.php
   @@ -0,0 +1,125 @@
   +   * @param \Drupal\Core\Config\ConfigFactoryInterface $config_factory
   +   *   The configuration factory.
   +   *
   

   Coder complains about the extra empty line in this docblock.

I tested the patch from #26 using the steps from #16 and got no errors. I checked the interdiffs from #26 and #24 and do not see anything wrong and see that #24 interdiff addresses items from #23. Marking RTBC.

+++ b/core/modules/locale/locale.module
@@ -83,6 +84,9 @@
+ * @deprecated Will be removed in Drupal 9. Use

@@ -91,6 +95,10 @@
+ * @deprecated Will be removed in Drupal 9. Use

@@ -98,28 +106,45 @@
+ * @deprecated Will be removed in Drupal 9. Use
...
+ * @deprecated Will be removed in Drupal 9. Use
...
+ * @deprecated Will be removed in Drupal 9. Use
...
+ * @deprecated Will be removed in Drupal 9. Use
...
+ * @deprecated Will be removed in Drupal 9. Use

+++ b/core/modules/locale/locale.translation.inc
@@ -426,16 +426,11 @@ function _locale_translation_source_compare($source1, $source2) {
+ * @deprecated Will be removed in Drupal 9. Use

Should be properly deprecated
@deprecated in Drupal 8.2.0, will be removed before Drupal 9.0.0.

Thanks for the review. I have updated the patch with the changes from #28.

Thanks! Patch looks good, but 8.2.0 was already released (sorry for the late review). We should change this. Otherwise looks good to me. We will need a change record too.

+++ b/core/modules/locale/locale.module
@@ -83,6 +84,9 @@
+ * @deprecated in Drupal 8.2.0, will be removed before Drupal 9.0.0. Use

Anyone planning to pick this up today for Global Sprint Weekend?

Re-rolled the patch. It did no apply due to the array syntax change.

- Changed the version in the deprecation info to 8.5.0
- Changed the deprecated REQUEST_TIME to time::getRequestTime(). Don't think it is good to put deprecated stuff in the new TranslationUpdater class.

Rerolled the patch.

Oh, sorry to spoil the party, but I had a pending review I just couldn't finish this morning.

Checked that deprecated constants and function are not used anywhere on core.
Checked comments of new class, interface, constants, methods.

Some things to work on though...

1. +++ b/core/modules/locale/locale.module
   @@ -98,28 +106,45 @@
   + *
   + * @deprecated in Drupal 8.5.0, will be removed before Drupal 9.0.0. Use
   + *   \Drupal\locale\TranslationUpdaterInterface::DEFAULT_SERVER_PATTERN instead.
     */
    const LOCALE_TRANSLATION_DEFAULT_SERVER_PATTERN = 'https://ftp.drupal.org/files/translations/%core/%project/%project-%version.%language.po';
    
   

   This should not be modified in this patch. It is not used in the TranslationUpdater server and it is not part of the high level update proces but it is used in the check/fetch process.

2. +++ b/core/modules/locale/locale.module
   @@ -98,28 +106,45 @@
   + *
   + * @deprecated in Drupal 8.2.0, will be removed before Drupal 9.0.0. Use
   + *   \Drupal\locale\TranslationUpdaterInterface::STATUS_TTL instead.
     */
   

   Version in deprecation text is not correct.

3. +++ b/core/modules/locale/src/TranslationUpdaterInterface.php
   @@ -0,0 +1,122 @@
   +  /**
   +   * Imports the available translation updates.
   +   *
   +   * Note that this does not actually check whether translation updates are
   +   * available, TranslationUpdateInterface::check() needs to be used for that.
   +   *
   

   Comment incorrect. It _does_ check whether translation updates are available. But only if previous check was executec more than 10 minutes ago (TranslationUpdaterInterface::STATUS_TTL).

#46 comments processed.

I found that 'instead' word is duplicated on local.module

The patch needs a reroll if this issue is still relevant (I asked in the #multilingual Slack channel but no one chimed in).

Re-roll patch created, Please have a look.

Take a look at the "15 coding standards messages" noted here:

https://www.drupal.org/pift-ci-job/1769855

@Kristen Pol Fixed the "coding standards messages", Please have a look.

Thanks for the update. Sorry, I should have looked at the patch prior to comment #57.

I only scanned the patch for obvious issues and did not review for the approach as it was reviewed above by @tstoeckler and @Sutharsan. The only obvious thing that that needs changing are the versions:

1. +++ b/core/modules/locale/locale.module
   @@ -88,6 +89,11 @@
   + * @deprecated in drupal:8.5.0 and is removed from drupal:9.0.0. Use
   

   Change versions to 9.1.0 and 10.0.0, e.g. @deprecated in drupal:9.1.0 and is removed from drupal:10.0.0.

2. +++ b/core/modules/locale/locale.module
   @@ -96,6 +102,12 @@
   + * @deprecated in drupal:8.5.0 and is removed from drupal:9.0.0. Use
   

   Same as above.

3. +++ b/core/modules/locale/locale.module
   @@ -108,23 +120,44 @@
   + * @deprecated in drupal:8.5.0 and is removed from drupal:9.0.0. Use
   

   Same as above.

4. +++ b/core/modules/locale/locale.module
   @@ -108,23 +120,44 @@
   + * @deprecated in drupal:8.5.0 and is removed from drupal:9.0.0. Use
   

   Same as above.

5. +++ b/core/modules/locale/locale.module
   @@ -108,23 +120,44 @@
   + * @deprecated in drupal:8.5.0 and is removed from drupal:9.0.0. Use
   

   Same as above.

6. +++ b/core/modules/locale/locale.module
   @@ -108,23 +120,44 @@
   + * @deprecated in drupal:8.5.0 and is removed from drupal:9.0.0. Use
   

   Same as above.

7. +++ b/core/modules/locale/locale.translation.inc
   @@ -431,16 +431,13 @@ function _locale_translation_source_compare($source1, $source2) {
   + * @deprecated in drupal:8.5.0 and is removed from drupal:9.0.0. Use
   

   Same as above.

Also, it would be good to double check that I didn't miss any of these.

@Kristen Pol changes has been done according to #59, Please have a look.

Thanks for the update. The changes address #59.

Now we need to see if tests come back green.

@Kristen Pol, Fixed the test, Please review.

Thanks for the update.

1) I was confused why the interdiff was so large and then I went back through the old patches and see that the class and interface you added were there back in #48 and then got accidentally removed in #56. I compared the new patch with #48 and the differences seem ok:

a) Changed @deprecated in Drupal 8.5.0, will be removed before Drupal 9.0.0. to @deprecated in drupal:9.1.0 and is removed from drupal:10.0.0. as requested in #59.

b) Added @see https://www.drupal.org/node/2847622 which is the change record that was created in 2017 (!). This was added starting in patch #58.

Note that I just updated the change record to reference the 9.1.x branch.

c) drupal_set_message is switched to $this->messenger()->addStatus due to deprecation.

d) $this->url was changed to Url::fromRoute due to deprecation.

2) Tests pass.

3) Code was reviewed previously by @tstoeckler and @Sutharsan.

4) I've updated the issue summary to strike out what I know has been done. What I'm unclear about is if additional tests need to be added as the issue summary says to write tests yet the "Needs tests" tag was never added and there aren't any comments about writing tests.

5) Tagging for manual testing as it's probably good to double check this even though automated tests passed.

I tested this as following. At first I was going to try to wait for awhile to see if translations were added to test the update part but I found a way to cheat.

1) Add a language but very quickly use the browser back button so that it doesn't pull over the translations

2) You see a message the language was added

3) Go to /admin/reports/translations

4) Click Check manually link

5) You will see that there are translations available

6) Click Update translations button

7) Translation will be added

8) Go to /admin/config/regional/translate/settings

9) Click Check updates now. link

10) You will see that the projects were checked

This worked as expected. Moving back to needs work for tests in case more need to be added.

When, if, this lands: we will need phpstan-drupal to be explicitly updated to check for these constants in https://github.com/mglaman/phpstan-drupal/blob/main/src/Rules/Deprecatio...