Modernize locale file handling

I have some type questions I'm not sure if we should update.

I kind of figured we needed more cleanup which is why I didn't add more.

Even though there isn't much usage in contrib I think there is custom usage due to this: #3575678: Locale checks file mtime but this is tricky if the translation files are copied to an image by Docker use a hash of the file instead

I guess technically this should be postponed on that but I want to work out what the api looks like here first before postponing it, and the other issue will likely get in soon anyway.

Thanks for the review I'll work on it.

Obviously got some failures to work through, but I think I have the structure in place.

Also looking through the other files.
I think we should handle
locale_translation_http_check
locale_translation_download_source
here as well, they are the only two non batch related functions in locale.batch.inc and they both relate to files as well.
Compare has a bunch, but there is a separate issue for that. Once this, and the compare are in there are only batch related functions in .inc which can probably be done in one issue.

locale_translation_http_check I think should go on localeFileManager
locale_translation_download_source should also go on localeFileManager and return a LocaleFile, there is a type property on the file set in locale_translation_download_source, but it's not used so I think we drop it.

Happy to keep type if we think it's necessary.

We did need type and a couple more that get added dynamically.

I think there is likely one test failure I could use help with, but I think this is ready for a general architecture review again.

Thinking about this i have a theory that we need to check if project or version were set by the constructor, I suspect it's an order of operations issue.

Fixed most comments and did some cleanup.

core/modules/locale/tests/src/Functional/LocaleUpdateTest.php has a bunch of failures now I can't look at at the moment.

There is a failure on HEAD too and a random as well, but I think this is close.

I suspect the new failures are due to the ImportForm change.

locale_translation_get_file_history is returning the timestamp.
It gets set in locale_translation_update_file_history

This is called in three places:
locale bulk: locale_translate_batch_import_save
locale module: locale_translation_status_save
LocaleSource: sourceBuild

It seems to be set from timestampMedium which is offset by 200.

Seems to be something to do with the default:

    $default = [
      'langcode' => $langcode,
      'uri' => '',
      'timestamp' => $this->timestampMedium,
      'last_checked' => $this->timestampMedium,
    ];

The failing test is: core/modules/locale/tests/src/Functional/LocaleUpdateTest.php
It seems that somehow the history lookup is returning stale information.

I pushed up a debug only branch that adds some checks to make it easier to see what is happening, all line numbers are on that branch

I set a local variable for the two timestamps involved:
$mediumTimestamp = $this->timestampMedium;
$nowTimestamp = $this->timestampNow;

The medium is the default timestamp set on files, the now is what we compare against for updates.
Line 196 is asserting that the updated timestamp is greater than now and fails because the timestamp set is still set to the medium timestamp.

I initially got suspicious of $history = locale_translation_get_file_history(); so I looked at it and extracted the direct db call:

\Drupal::database()->select('locale_file')
      ->fields('locale_file', ['project', 'langcode', 'filename', 'version', 'uri', 'timestamp', 'hash', 'last_checked'])
      ->execute()
      ->fetchAll();

I then checked that 6 separate times in the test to see if the timestamp for the files change.
It does not.

One this branch:

IMPORTANT NOTE you need to check the actual project in the result, since each run the order of the projects is different in results or history.
Make sure you're comparing the same project, they are named similarly.
Example test run checking values for "contrib_module_one"

$mediumTimestamp = 1774534743
$nowTimestamp = 1774534943
$result3[contrib_module_one]->timestamp = "1774534743"
$result4[contrib_module_one]->timestamp = "1774534743"
$history['contrib_module_one']['de']->timestamp = "1774534743"

On main

I then checked out main and copied the modified test in: it passes as expected.
Note you have to be careful to check for the same project since the order of the results can change.
Example test run checking values for "contrib_module_one"

$mediumTimestamp = 1774534495
$nowTimestamp = 1774534695
$result3[contrib_module_one]->timestamp = "1774534495"
$result4[contrib_module_one]->timestamp = "1774534712"
$history['contrib_module_one']['de']->timestamp = "1774534712"

The culprit seems to be somewhere in:

    $this->drupalGet('admin/reports/translations');
    $this->submitForm([], 'Update translations');

Which only submits: core/modules/locale/src/Form/TranslationStatusForm.php

I can't dig further now, but I suspect something with type is getting changed so the logic in prepareUpdateData is not following the same path.

Ok this has been super hard to debug, but I think I'm finally on track: because the test does this $this->drupalGet('admin/reports/translations'); xdebug doesn't stop in the form for the submission, so I added a file put contents and output the medium timestamp and added the type and printed the projects, I see the type is changing on something so I see it's not being set properly somewhere, I'll attach the debug so I can chase it down.

Here is a better version on main with other tests commented out, the number of calls are higher since the test doesn't fail.

Edit: looking closer I think it's another red herring :/

I will move those properties to the constructor and the regex to a method that only gets called in the path it was before. Thank you again!

Assigning to myself to rework.

I think this is ready for review!

I've addressed the concern with the regex being called in scenarios it didn't used to be called by moving it to createFromPath and directly instantiating LocaleFile elsewhere.

Edit: I just pushed up a parameter order change so the constructor and createFromPath share the order since all calls to createFromPath now have both parameters.

I've addressed almost all of your feedback, I really think we should leave checkRemoteFileStatus to a follow up.

We can easily add internal to it and update later.

I took another look and just updated checkRemoteFileStatus, it now uses a value object with an enum for the status.

Everything is green so it's ready for review again.

Thank you for the reviews, it was quite a change, but as you mentioned I think it's a great improvement, and we have a lot of follow ups for the rest of locale.

I updated the IS and CR.

This is ready again!