Convert locale.compare.inc to a service

#12 Patch Re-rolled for 9.1.x
Please review

@kishor_kolekar: The testbot is not happy with your reroll. At least the deprecation messages need to be updated to deprecate in 9.1 and removed in 10.0.

Updating deprecate message and re-rolled the patch.

@Hardik_Patel_12, Thanks for working on this issue. The types of issues being worked on by the Bug Smash Initiative are listed on the Bug Smash Initiative page and does not include Tasks. So, I am removing the tag.

@quietone , sure will keep in mind this. Thanks for your suggestion.

Thank you @daffie for ur suggestion.
Thanks @Hardik_Patel_12 for working on it.

Testbot still not happy.

Solving failed test cases. Kindly follow a new patch.

Fixed the coding standard and testcase issues.

Updated patch.

Patch re-rolled

Both above comments are addressed.

Also added missing 'array' type hints in LocaleProjectStatusInterface::remoteTranslationStatusOperations and LocaleProjectStatus:

-  public function remoteTranslationStatusOperations($projects, $langcodes, $options = []) {
+  public function remoteTranslationStatusOperations(array $projects, array $langcodes, array $options = []) {

Spell check fixes.

Trying to fix the deprecation tests.

I could not find a way to cover the deprecation error triggered at the top of the .inc file. I decided to remove this trigger althogether. Not a nice approach, but the best I could come up with.

Quickly skimed a pacth and last interdiff, will do deeper review later tonight

+++ b/core/modules/locale/locale.compare.inc
@@ -11,9 +11,6 @@
-@trigger_error(__FILE__ . ' is deprecated in Drupal 9.3.0 and is removed from Drupal:10.0.0. See individual methods in the file for individual deprecation notices with upgrade instructions. See https://www.drupal.org/node/3037033.', E_USER_DEPRECATED);

not sure this line need to be removed, as this file no longer loaded... contrib or custom code using to load it should fail

Re #37, I removed this line because I could not find a way to get the test working _with_ this line. As I tried to explain in #36.
Further I searched for other other .inc files which got deprecated to look for a pattern. #2044435: Convert pager.inc to a service deprecate pager.inc, it did not use a deprecation message at file level. I found no deprecated .inc files in core. Further the existing documentation standard on @deprectated only mentions function, method, class. The coding standards issue mentioned in #32 extends this list further (below), but does not mention deprecation of files. Therefore I propose to remove the @deprectated at file level and only use it for functions.

%thing%
    What is being deprecated - for example, the class name, method name, function name, service name or the use or optional status of a parameter

This issue converts _all_ functions in locale.compare.inc into one service where each function gets a method. In the current code of Locale module I found the LocaleConfigManager service which (a.o.) is called by batch functions in locale.bulk.inc.

This made me wonder whether it is proper architecture to include these batch functions into a service like LocaleConfigManager and LocaleProjectStatus (this issue). I am leaning towards separating the batch function from the functions that do the "work". In other words to move the batch building and batch operation/finish functions out of this patch.

AFAIK, batch callbacks can't be on services yet, only functions or static methods. Closest I managed is a batch function "dispatcher" than then calls the service. See _simplenews_batch_dispatcher() in simplenews.module.

@Berdir, Thanks for the tip, _simplenews_batch_dispatcher() is a clever way to call a service method from where only call_user_function_array is used by Drupal's batch API.

Let me break down the batch related parts of batch code: The Batch builder (which returns an array of batch title, operations, finished, etc., the batch definition as required by batch_set()), Batch operation callback (called by the batch API, handles batch administration and calls the Worker) and the Worker (which performs the actual task, but is not batch agnostic).
In my opinion the Worker should not be aware of the batch. Making the worker batch aware makes unit testing and re-use in other contexts harder. I did not check how the current procedural code does or does not mix batch and worker, but I'm painting my ideal picture here. A service is probably the first choice for the worker, and the service may combine multiple workers (called by multiple batch operations).

The question I'm raising here is whether we should (not) combine the Batch operation callback(s) and Batch builder into this Worker service. Worker, operation callbacks and builder in their own methods of course, but combined in one class. I did not find an OO pattern in core for Batch builder and Batch operation callbacks. I only found them in procedural code. As I mentioned above I lean towards _not_ combining them into the worker, but keep them separate (for re-use and unit testing). When choosing for separation, we can keep them procedural for now. But I 'd rather find an OO pattern for this too.

This choice is not only relevant for this issue, but for all of Locale module's .inc files which are subject to refactoring in of #3215707: [META] Modernize Locale module.

@Sutharsan The batch builder already in core https://www.drupal.org/node/2875389 but its adoption a bit staled #2875151: [META] Implement Batch API as a service

There's even #1797526: Make batch a wrapper around the queue system but I'm not sure it has enough chances to get in fast, OTOH locale could be first case to use it

IMHO we should separate if the worker part needs to be reusable, eg. used in multiple batches or by contrib, or should be overridden as for what the worker does vs not how the batch is built (separation of concerns). Testing may be another good reason indeed that I did not think of before.

A couple of nitpicks about the new interface:

1. +++ b/core/modules/locale/src/LocaleProjectStatusInterface.php
   @@ -0,0 +1,78 @@
   +  public function checkProjects(array $projects = [], array $langcodes = []);
   ...
   +  public function checkProjectsLocal(array $projects = [], array $langcodes = []);
   ...
   +  public function resetCache();
   

   Could we add ": void" to indicate that the method does not return anything.

2. +++ b/core/modules/locale/src/LocaleProjectStatusInterface.php
   @@ -0,0 +1,78 @@
   +  public function buildProjects();
   ...
   +  public function remoteTranslationStatusOperations(array $projects, array $langcodes, array $options = []);
   

   Could we add ": array" to indicate that the method returns an array.

3. +++ b/core/modules/locale/src/LocaleProjectStatusInterface.php
   @@ -0,0 +1,78 @@
   +   * @param array $options
   +   *   Batch processing options.
   ...
   +  public function remoteTranslationStatusOperations(array $projects, array $langcodes, array $options = []);
   

   The parameter is optional. Could we add "(optional)" to the parameter documentation.

The Needs Review Queue Bot tested this issue. It either no longer applies to Drupal core, or fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".

Apart from a re-roll or rebase, this issue may need more work to address feedback in the issue or MR comments. To progress an issue, incorporate this feedback as part of the process of updating the issue. This helps other contributors to know what is outstanding.

Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.

#3577671: Modernize locale file handling

I'm going to actually start fresh, the patch does not apply across 13 files and we've changed process, I'll review the patch for locations that functions landed.

Made it about halfway, haven't run CS so I won't do a MR since I know it will fail, but gotta take a break.

Thank you, I'll take that under consideration, but I think we don't want all of that overhead, another value object should be enough.

We do have to figure out how to incorporate projectInfo since this also uses that.

I added a couple of fixes and some comments, I'd like to chat about scope and what you think about the changes in LocaleSource.

One nice thing is that we added LocaleSource in 11.4 so if we need to change something as long as we get it in before 11.4 we can change it a bit.

The Needs Review Queue Bot tested this issue. It fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".

This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.

Ok, let's see how this goes.

I converted the object in LocaleSource to use the LocaleTranslatableProject object.

There are some properties stored on that object that look like just LocaleFile properties but are used elsewhere.
type is overloaded so I created a new property fileType to track.

I created the properties on the class so we could just set them and see what we should do. We could maybe keep them short term, but I am not sure.

There is a fairly complex conflation of LocaleTranslatableProject and LocaleFile between LocaleSource, locale_translation_update_file_history and locale_translation_status_save.

I also deprecated LocaleProjectStorage and LocaleProjectStorageInterface.

Most of the failures are due to using a deprecated service, but I think it's the alias:

Drupal\locale\LocaleProjectStorageInterface:
    alias: 'locale.project'
    deprecated: The "%alias_id%" service is deprecated in drupal:11.4.0 and is removed from drupal:13.0.0. Use the 'Drupal\locale\LocaleProjectRepository' service instead.See https://www.drupal.org/node/3037033

Down to the dreaded functional failures:
Fail locally:
Drupal\FunctionalTests\Installer\InstallerTranslation
Drupal\Tests\search_help\Functional\HelpTopicSearch
Drupal\Tests\locale\Functional\LocaleConfigTranslationImport
Drupal\Tests\locale\Functional\LocaleUpdateCron
Drupal\FunctionalTests\Installer\InstallerExistingConfigSyncDirectoryMultilingual
Drupal\Tests\help\Functional\HelpTopicTranslation
Drupal\FunctionalTests\Installer\InstallerTranslationMultipleLanguageForeign
Drupal\FunctionalTests\Installer\InstallerTranslationMultipleLanguageNonInteractive
Drupal\Tests\locale\Functional\LocaleUpdate
Drupal\FunctionalTests\Installer\InstallerTranslationMultipleLanguageKeepEnglish
Drupal\FunctionalTests\Installer\InstallerTranslationMultipleLanguage
Drupal\FunctionalTests\Installer\InstallerTranslationNonStandardFilenames
Drupal\Tests\locale\Functional\LocaleTranslationChangeProjectVersion

Passes locally:
Drupal\Tests\locale\Functional\LocaleUpdateInterface

Down to just a few, a bunch of the tests failed due to conflated type and file type properties, we probably need to truly clean that up.
I also removed readonly for now from the properties.

Drupal\Tests\locale\Functional\LocaleUpdate
Drupal\FunctionalTests\Installer\InstallerExistingConfigSyncDirectoryMultilingual
Drupal\Tests\locale\Functional\LocaleUpdateInterface

Ok I got this green, this is still kind of rough, but it's ready for a round of reviews.
I imagine we want to try to better decide what to do about the repeated properties on LocaleTranslatableProject. Also a cleaner way to differentiate between the type on LocaleTranslatableProject and LocaleFile (They each have type, but the legacy code seemingly randomly overwrote the project type to the file type, I added a new fileType property to track, but it lead to much confusion.

I also updated the CR.

We also have to not trigger deprecation for LocaleProjectStorageInterface or add the deprecated key to the LocaleProjectStorageInterface service alias due to: #3588483: Deprecating services that are autowired and autoconfigured doesn't quite work

I updated a bunch of other services to help fix some circular dependencies in LocaleFileManager and LocaleSource. I think these changes make this issue crucial to get into 11.4 if possible since those classes are not released yet and we'll avoid double deprecations.

Ok I addressed almost all of the super straightforward stuff on the current MR, I updated the CR with those changes, but will need a new pass to make sure I didn't forget something especially if we go to the new MR.

For the getProject, get, getMultiple etc I created a new MR since it's a behavior change, I think it's good minus a bit of messiness changing getAll behavior and needing to check if we need multiple or all. All of the get calls now populate the cache properly so a few tests don't work, it's also a new object, so the assert same won't work, I deleted some tests there and commented out two that we may want to discuss. Honestly this is probably better, why do we want to have out of sync projects and cache deleting a project?

I think the two big remaining questions not addressed here fully:
1. Do I go back to type and just document it can be extension type or file location type?
2. Do we try to reuse the translatable project passed into LocaleSource or do we need a psuedo clone.

This is starting to really shape up!

I addressed the most recent round of feedback and updated a few things too.

This is ready again! I took a look at the IS and CR with the most recent changes.

We discussed the $source vs project stuff in LocaleSource and I proposed some changes to that, that essentially restore it almost exactly to HEAD and no longer merges those two things together. It's a weird split, they are overlapping and cloned from each other, but each has their own distinct properties and behaviors and I think we should not merge them. This allows us to remove several properties from LocaleTranslatableProject that were only used after being converted to a source. This also solves the type property conflict. Discussed that extensively in Slack.

I will update the old follow-up #1842380: Convert $source object to a LocaleTranslationSource class to add a type for $source, we could revive that for that in a follow-up.

That is and will likely remain my only direct code contribution to this MR, I think I'm still OK RTBC'ing this after it this been cross-reviewed.

We discussed the changes in the previous comment before @berdir pushed them, I also reviewed the commits directly and the flow after.

It's mainly scaling back some of the changes made here that really should be done in a follow up, @berdir already identified the follow up.

I'll sign off on his changes, I think this is ready for review again, I've addressed all feedback!

Refactoring the locale module is really a challenge. There is so much code, complex value objects that are just so slightly different ($source vs $project, which this tried to unify but we undid that again), so many features and functions. The project storage is essentially a cache but comes with an extensive storage/new repository service that we slimmed down but still does a a lot. Sometimes the data is completely deleted and rebuilt (clicking the check manually button on the reports page), sometimes removed/uninstalled projects are just disabled, there is the sorting which I don't know what it's really used for and so many other things.

We tried our best to find a middle ground between refactoring, using typed value objects, reducing the API service but also still somewhat limiting the scope here to be able to finish this. We have a number of follow-ups and I just filled #3589344: Remove the ability to have enabled/disabled projects in the locale project repository for the status stuff.

This isn't perfect, but I think it's a reasonable improvement. and while the diff stats don't show that reduction in complexity/API surface (+ 828 − 320), that is mostly due to the old code only being deprecated. Once we can remove all the code in locale.compare and the old storage service, this should be roughly even again.

Added one more comment and comment suggestion on the MR, but I think that's not holding up an RTBC.

Thank you! I updated the two comments after confirming your question.

As you mentioned we still have a fair bit to do, but this is pretty good progress I think.

We have the two follow ups you created and the plan for .module elimination (all of the batch functions are in the next issue, but based on how these have gone that might be ambitious).

This all looks good to me, but one small question on the MR.

Committed/pushed to main, thanks!

Will need an 11.x backport MR, looks like services.yml and a test so hopefully not too bad.

The backport should be ready.

The additional +2 are from needing autowire: true on the two new services.
The additional -8 are from the deleted test difference (one of the reasons there wasn't a clean backport)

The reasons we needed a backport:
Services autowiring
locale.translation.inc a use statement needed to be added
bootstrap.inc the drupal_static_reset changes

I compared the main commit with the backport and the changes are all the same.

Committed/pushed to 11.x, thanks!

I updated the previous CR.

The only one that changed was https://www.drupal.org/node/3569330 for the locale_translation_get_projects deprecation.

I published the recently reviewed change record.