Modernize locale history functions

1. +++ b/core/modules/locale/src/LocaleImportHistory.php
   @@ -0,0 +1,90 @@
   +   * Memory cache of translations import history.
   ...
   +   * @var array
   +   */
   +  protected $history;
   ...
   +  public function resetCache() {
   +    unset($this->history);
   

   maybe use [] instead of null?

2. +++ b/core/modules/locale/src/LocaleImportHistory.php
   @@ -0,0 +1,90 @@
   +      $result = $this->database->query("SELECT project, langcode, filename, version, uri, timestamp, last_checked FROM {locale_file}");
   +      foreach ($result as $file) {
   +        $file->type = $file->timestamp ? LOCALE_TRANSLATION_CURRENT : '';
   

   looks usage of constant makes this service non-unit testable, maybe just add this to query to return required value?

3. +++ b/core/modules/locale/tests/src/Functional/LocaleUpdateCronTest.php
   @@ -104,8 +107,8 @@ public function testUpdateCron() {
   +    $import_history->resetCache();
   +    $history = $import_history->getHistory();
   
   +++ b/core/modules/locale/tests/src/Functional/LocaleUpdateTest.php
   @@ -153,8 +153,10 @@ public function testUpdateImportSourceRemote() {
   +    $import_history->resetCache();
   +    $history = $import_history->getHistory();
   
   @@ -207,8 +209,10 @@ public function testUpdateImportSourceLocal() {
   +    $import_history->resetCache();
   +    $history = $import_history->getHistory();
   

   looks reset used only in tests, so does it need to be public api?

It makes sense to keep scope creep
otoh While we factoring service makes sense to file follow-up and discus proper implementation

Over time this history needs clean-up for outdated translations and same time mostly all entities in core are revisionable
Also static usage in history service stuck on storage for any core entity

The LOCALE_TRANSLATION_CURRENT sounds like duplicate of FIELD_LOAD_CURRENT so Makes sense to move this constant to new history service

Re-rolling against 9.1.x-dev.

Fixed failed test case.

1. +++ b/core/modules/locale/tests/src/Unit/LocaleImportHistoryTest.php
   @@ -0,0 +1,55 @@
   +   * @expectedDeprecation locale_translation_get_file_history() is deprecated in Drupal 8.7.0 and will be removed before Drupal 9.0.0. Use \Drupal\locale\LocaleImportHistoryInterface::getHistory(). See https://www.drupal.org/node/3037162.
   ...
   +   * @expectedDeprecation locale_translation_update_file_history() is deprecated in Drupal 8.7.0 and will be removed before Drupal 9.0.0. Use \Drupal\locale\LocaleImportHistoryInterface::updateHistory(). See https://www.drupal.org/node/3037162.
   ...
   +   * @expectedDeprecation locale_translation_file_history_delete() is deprecated in Drupal 8.7.0 and will be removed before Drupal 9.0.0. Use \Drupal\locale\LocaleImportHistoryInterface::deleteHistory(). See https://www.drupal.org/node/3037162.
   

   This is now done with the use of the method $this->expectDeprecation().

2. +++ b/core/modules/locale/tests/src/Unit/LocaleImportHistoryTest.php
   @@ -0,0 +1,55 @@
   +   * @group legacy
   ...
   +   * @group legacy
   ...
   +   * @group legacy
   

   All tests in this class are legacy. Therefor the tag @group legacy can be moved to the docblock of the class and the ones in the docblocks of the tests can be removed.

3. +++ b/core/modules/locale/tests/src/Unit/LocaleImportHistoryTest.php
   @@ -0,0 +1,55 @@
   +  public function testLocaleTranslationGetFileHistoryDeprecation() {
   ...
   +  public function testLocaleTranslationUpdateFileHistoryDeprecation() {
   ...
   +  public function testLocaleTranslationFileHistoryDeleteDeprecation() {
   

   Every test needs an assertion. My recommendation would be to test the return value of each function call with an assertion.

4. The deprecation will now happen in 9.3.0 and not in 8.7.0. The deprecation messages need to be updated.

5. +++ b/core/modules/locale/src/LocaleImportHistoryInterface.php
   @@ -0,0 +1,54 @@
   +  /**
   +   * Resets the service memory cache.
   +   */
   +  public function resetCache();
   

   Can we remove this method from the interface. See the comment #5.3 from @andypost.

6. +++ b/core/modules/locale/src/LocaleImportHistory.php
   @@ -0,0 +1,90 @@
   +  public function resetCache() {
   

   Can we add to the docblock that this method is @internal.

7. +++ b/core/modules/locale/locale.module
   @@ -814,25 +810,14 @@ function locale_translation_get_file_history() {
   - * @return int
   - *   FALSE on failure. Otherwise SAVED_NEW or SAVED_UPDATED.
   

   I think it is ok to remove return value. The function is not used in contrib. See: http://grep.xnddx.ru/search?text=locale_translation_update_file_history%... Only it is a API change and it needs to be documented in the IS and the CR.

8. +++ b/core/modules/locale/src/LocaleImportHistoryInterface.php
   @@ -0,0 +1,54 @@
   +   *   The import history as an associative array keyed by project ID (e.g.
   +   *   'drupal', 'pathauto', etc). Each value is an associative array having the
   +   *   language code as key and the file represented as an \stdClass object with
   +   *   the following keys:
   

   Can we change the text to: "The import history as an associative array that is keyed twice. First by the project ID (e.g. 'drupal', 'pathauto', etc). The associative array is keyed then by the language code. The value is the file as an \stdClass object with the following keys:"

Re-rolled the patch manually for 9.3.x as it was not applied automatically

There were various code style fails and a spelling check fail (LocaleImportHistory.php:30:12 - Unknown word (databse)) , see https://www.drupal.org/pift-ci-job/2123269

Fixed the fails. Please check.

The points from comment #18 still needs to be addressed.

The last few patches have dropped the interface and implementation of LocaleImportHistory. NW to resolve this and the feedback in #18.

I'm hoping the conversion here will help with performance. We're currently using an install profile that adds multiple contrib modules and languages. We see calls to these functions around 96% of the execution time and taking almost 2 hours to run with 8 or 9 languages.

After reviewing the code, my suggestion is to add an optional argument to get/update/delete to include the project name. It would significantly reduce the thrashing on install. If that is out of scope, maybe it should be part of scope. Since we are already changing the API a fair bit here. And deprecate calls to these services that don't provide them. If they do provide them, limit the query scope. If they don't, trigger a deprecation as of drupal 10 or 11 that they will be required.

Thank you for working on this!

I have a bunch of comments on the MR, I'm happy to answer any questions you may have.

Please don't get overwhelmed, there are just a bunch of conventions we have worked out in the .module initiative we need to apply here.

Normally I would provide MR suggestions you could apply, but I'm on my phone and some would be cross file and multi line.

Thanks for providing some feedback. I'll check this out and work on it today

Great! #2907780: Add a field purger service has examples for most of the comments I made.

Addressed the feedback. Please review.

I noticed that there's an error in PHPUnit Functional Javascript 2/3 job but it looks like it's unrelated to the current issue changes.

Would be happy to finalize this issue if something is needed.

The Functional Javascript is a known random failure, you can rerun the job manually.

There are a couple more minor notes.

Unfortunately there is one larger issue that I think means we need to postpone this on #3577671: Modernize locale file handling

Edit: quick note that the referenced issue is RTBC and is pretty close, so this should not be postponed too long.

Addressed remaining minor notes.

Concerns in #25 and #26 remains to verify as performance metrics of install is viable

I don't think that is in scope, and it won't really help since get gets the full history.

We could maybe create a new get for a single project that would help, but that would be a follow up for sure.

Update already only updates a single file so no change would be needed there.
Delete history let's you define projects already.

The other issue is in, this can be updated to use LocaleFile.

Unfortunately I think we need to postpone this again, the compare issue is touching a bunch of things, I think the plan now is to do that issue, then the batch functions then either this or the constant ones.

I'm only going to postpone this on one for now since this may be better to address earlier once I start the batch one.
#3037031: Convert locale.compare.inc to a service

Once this is ready we need to use the LocaleFile and a memoryCache instead of a static cache.

Both #3037031: Convert locale.compare.inc to a service and #3581303: Convert locale batch callbacks is in, I think this is the best next step.

I'm working on this.

Ok I recreated the MR since there were so many conflicts, I pulled the information from existing MR.

I'm going to work on the LocaleFile bit then look a the memory cache for individual calls.

Ok I've converted the database call to a value object.

If we want to convert updateHistory to use a value object we would need to do #1842380: Convert $source object to a LocaleTranslationSource class here.

It would be nice to have one normally scoped issue though for locale, so I'm not sure we do this here.

I took a quick look at splitting up the memory cache, I think the only way to really do this is to add a single get call, if there is a miss we then load all to populate, and we have no way to get all projects. Does that seem right?

Ok I added a v3 version that replaces the get all with an individual project and language, I'm curious if we should add a get all as well I think this will be way more performant and I'm not sure we need a get all at all.

There are no usages of locale_translation_get_file_history in contrib.

Not waiting on tests at this hour so I'm not sure they are passing on v3.

v2 is failing too, I'll track that down later, but the approach would be good to verify.

Same test is failing in both so I just gotta fix it and we can decide on a direction.

I've been trying to a bit of profiling with a bunch of languages on our distribution with a ton of projects. One thing I've confirmed is that if there's no file history info yet on 11.x, then we get exactly into the loop of load all -> update -> invalidate -> load all again. invalidating just the changed one should fix that. That only happened for a language where I aborted the initial batch operation that fetched the translations. not just the updates, but also the get history calls:

You can see that there are only 24 calls to buildSource(), each then updates that info.

A different scenario is when locale_translation_clear_status() happens, because that clears the locale.translation_status key value store. This doesn't happen often, but it can happen. it used to happen when saving the file settings form, it still happens when saving the locale settings form and changing the translation source. What I expected to happen then is a lot of calls to buildSource() and to get file history, but none/few to update. I did get 520 calls, but strangely, I also get 520 update calls. Something about that timestamp logic doesn't seem to add up, I'm not sure what needs to happen for that to be set to trigger that case.

=> There is a possible scenario where a getAll() in theory might be beneficial, but it's really not possible to know when upfront, so I don't think there is a place where we could say, I'm going to load all because I know I'll need them. So I think it's fine to only support single get and update, that will in most cases be a significant improvement. What I think *is* problematic is calls to locale_translation_clear_status(), because on a site with 100 languages and 50 projects, that will result in 5000 sourceBuild() calls after a cache clear. It would probably be better to trigger a rebuild as batch then afterwards and not do it on one request on the reports page. But the current refactoring will not make things worse, it will more likely make several things significantly better, such as not loading all translation status data + file history data on every request when it's not needed.

Locale is the gift that keeps on giving.

Had another long discussion with @berdir on slack about this and came to the conclusion yet again that this is not LocaleFile.

To that end we created another LocaleSourceInfo value object for tracking the info.

I created a map of all of the ways to get to sourceBuild since that is where things get muddy again. It doesn't necessarily show the logic flow entirely but it shows where you can start and end up in sourceBuild.

I created a v4 to show this approach, it is green again so I think it's ready for review.

I also changed update to just update the memory cache directly now that we have the info we need.

I'm going to hide v3 and update the issue summary and change record.

Reviewed. I think v4 makes sense as a concept. translation status is still a mess, that we need yet another value object, Need to think about about the class and method names. And possibly the final question to me is if we want to move the direct query in the cron hook in this issue or a follow-up.

Ok I renamed this, I also moved locale cron to it's own class so we could move the storage interactions to the storage class.

Reviewed, I think we're getting close.

I think it's ready for review again!

Ok, I feel comfortable RTBC'ing this now. I also made a lot of updates to the issue summary now.

Note that we basically pair-programmed this. I didn't write any code (just made a few suggestions on comments) but we both spent hours mapping and understanding the functions calls and data flows around this and then more hours discussing naming, how to isolate and separate this data structure from the others and so on.

Considering the starting point, I'm quite happy with the result. We went from functions that, despite only having 1/3 calls, were so confusing that it took me an entire Saturday morning to have a basic understanding of what they actually do to a storage service that fully encapsulates this with a typed and documented value object, separating it from the various other data objects that are floating around. See the comments on the v3 merge request on my discovery journey: https://git.drupalcode.org/project/drupal/-/merge_requests/15774

We did our best to improve and clarify (previously barely existing) documentation. I'm sure it's not perfect, but I think it's a clear improvement. Keep in mind that absolutely nobody in contrib and presumably custom code directly uses any of this, it's entirely internal and now documented as such.

This looks like a huge improvement, as mentioned there is more to do, but feels like we're getting to a much more organised point very quickly.

Committed/pushed to main - will need a backport MR for 11.x

Those should be ready.

The conflict was the use statements in locale.module.

I also fixed autowire.

I asked berdir to review if we would have issues with readonly properties and I think will be OK.

Committed/pushed to 11.x, thanks!