Problem/Motivation

locale.translations.inc is the bottom of the chain of circular dependencies.

Let's convert this to services.

  • locale_translation_get_projects
  • locale_translation_clear_cache_projects
  • locale_translation_load_sources
  • locale_translation_build_sources
  • locale_translation_source_check_file
  • locale_translation_source_build
  • locale_translation_build_server_pattern
  • locale_cron_fill_queue
  • _locale_translation_file_is_remote
  • _locale_translation_source_compare
  • _locale_translation_default_update_options

Steps to reproduce

Open the module

Proposed resolution

Analyze the functions and outline new services.
Constants should be deprecated, there are no uses in contrib and they are only used in one function.

I propose the following organization.
Move to new service: LocaleSources

  • locale_translation_load_sources
  • locale_translation_build_sources
  • locale_translation_source_check_file
  • locale_translation_source_build
  • locale_translation_build_server_pattern
  • make protected and delete _locale_translation_file_is_remote

Move to LocaleProjectStorage

  • locale_translation_get_projects

Replace with static cache

  • locale_translation_clear_cache_projects

Move logic to LocaleHooks and deprecate without replacement

  • locale_cron_fill_queue

Inline and simplify logic in TranslationStatusForm: Delete function

  • _locale_translation_source_compare

Deprecate without replacement now that the logic is inline

  • comparison constants

New api LanguageDefaultOptions

  • _locale_translation_default_update_options

Remaining tasks

Scope discussion:
Since this is both manually included with require_once and included with moduleHandler->loadInclude handling the file at once reduces circular dependencies.
The CR is a great overview of where the code went
Use the command git diff main --color-moved=dimmed_zebra --color-moved-ws=ignore-all-space to compare moved code vs changed code.

User interface changes

Introduced terminology

API changes

New: LanguageDefaultOptions
New: LocaleSources
LocaleProjectStorage->getProjects

Data model changes

Release notes snippet

Issue fork drupal-3569328

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

About issue forks

Comments

nicxvan created an issue. See original summary.

nicxvan’s picture

nicxvan commented
Issue summary: View changes
nicxvan’s picture

nicxvan commented
Related issues: +#3036982: [pp-1] Deprecate locale_translation_get_projects() and locale_translation_clear_cache_projects()

nicxvan opened merge request !14476

nicxvan’s picture

nicxvan commented
Issue summary: View changes
nicxvan’s picture

nicxvan commented
Issue summary: View changes
nicxvan’s picture

nicxvan commented
Issue summary: View changes
nicxvan’s picture

nicxvan commented
Issue summary: View changes
nicxvan’s picture

nicxvan commented
Issue summary: View changes
nicxvan’s picture

nicxvan commented
Issue summary: View changes
nicxvan’s picture

nicxvan commented
Issue summary: View changes
nicxvan’s picture

nicxvan commented
Issue summary: View changes
nicxvan’s picture

nicxvan commented
Status: Active » Needs review
nicxvan’s picture

nicxvan commented
Issue summary: View changes
dcam’s picture

dcam commented

I only found one thing to comment on. If you want to leave the code as close to the original as possible, then this could be delayed to a follow-up. But I feel like it's appropriate to deal with it now due to the addition of the enum.

Everything else looks good. I validated all of the moved code. I still need to try manual testing. I'll keep working on it.

dcam’s picture

dcam commented

I tried to put Locale through its paces to do manual testing. I'm not sure I tested all paths, but so far it's working correctly. It downloaded translations from the server. It checked remotely-hosted translations versus ones I already had downloaded. It downloaded translations when a language was enabled. I was able to trigger manual downloads from the translation status form. As far as I can tell, the module is still working correctly.

I think the only thing to figure out is whether we want to deal with the enum as indicated in my previous comments.

nicxvan’s picture

nicxvan commented

Let me create a separate MR with those suggestions we can see more clearly.

nicxvan opened merge request !14585

nicxvan’s picture

nicxvan commented

That's a great suggestion, please double check my new MR and confirm it's working.

That makes this significantly smaller too.
If you're testing confirms and this looks right I'll update the IS and CR

nicxvan’s picture

nicxvan commented
Issue summary: View changes

nicxvan changed the visibility of the branch 3569328-modernize-locale.translations.inc to hidden.

dcam’s picture

dcam commented
Status: Needs review » Reviewed & tested by the community

Yeah, this looks great to me. I re-reviewed the MR changes, but for the sake of being thorough I also diffed the raw diffs of the two MR so I'd know exactly what changed between them. The constants and function are now deprecated without replacement. The IS and CR were updated to reflect this change. I'm glad this worked out so we could simplify the code.

Actually, I'm amazed there was such an opportunity for simplification. It makes me wonder if this API has changed a lot over time. Two of the three constants basically weren't used. Maybe they were in the past. It isn't worth taking the time to do Git archaeology to check.

claudiu.cristea’s picture

claudiu.cristea
Primary language Romanian
Location Arad 🇷🇴
commented
Related issues: +#3566536: [meta] eliminate core .module files
claudiu.cristea’s picture

claudiu.cristea
Primary language Romanian
Location Arad 🇷🇴
commented

Please collect also the credits from #3036982: [pp-1] Deprecate locale_translation_get_projects() and locale_translation_clear_cache_projects()

nicxvan’s picture

nicxvan commented

All closed issues grant credit now, when I close the other I'll take care of credit.

nicxvan’s picture

nicxvan commented
Status: Reviewed & tested by the community » Needs review

We got new direction that we should stick to the normal BC policy for underscore which is to just delete them.

I pushed up deleting the underscore functions and replacing the remaining deprecated functions to use the new api.

I then updated the deprecation version to 12 since the deletions need to target the next major since we're so close.

nicxvan’s picture

nicxvan commented
Issue summary: View changes
dcam’s picture

dcam commented

I left one question on the MR.

nicxvan’s picture

nicxvan commented

I updated it, I'll keep an eye on tests.

dcam’s picture

dcam commented
Status: Needs review » Reviewed & tested by the community

My feedback on the recent changes was addressed.

nicxvan’s picture

nicxvan commented
Status: Reviewed & tested by the community » Needs review

Sorry for the ping pong, I added the underscore back as the decision is to keep them and deprecate for removal in 12.

There is a mix of removal versions in this MR.
All functions / constants are deprecated in 11.4
Removal is 13 if it is api
Removal is 12 if it is not api

I also addressed @berdir's new feedback around the drupal_static deprecation

dcam’s picture

dcam commented
Status: Needs review » Reviewed & tested by the community

The non-API functions have been restored and deprecated in 11.4 for removal in 12. The additional change to bootstrap.inc looks good. LGTM.

nicxvan’s picture

nicxvan commented

I just updated $timestamp1 to $local_timestamp and $timestamp2 to $remote_timestamp for clarity.

It was just a couple of lines for clarity and tests are still green so I'm leaving RTBC

  • catch committed 5b95ce27 on 11.x 
    task: #3569328 Modernize locale.translations.inc

By: nicxvan
By: dcam...
catch’s picture

catch
he/him
Primary language English
commented
Version: main » 11.x-dev
Status: Reviewed & tested by the community » Fixed

Reviewed this. It's not easy to see what the final result will end up like, but I don't see anything particularly troublesome either.

Committed/pushed to main and 11.x, thanks! Also published the change record.

Now that this issue is closed, review the contribution record.

As a contributor, attribute any organization that helped you, or if you volunteered your own time.

Maintainers, credit people who helped resolve this issue.

  • catch committed b4ee668c on main 
    task: #3569328 Modernize locale.translations.inc

By: nicxvan
By: dcam...

nicxvan closed merge request !14585

nicxvan closed merge request !14476

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.

grimreaper’s picture

grimreaper
Primary language French
Location France 🇫🇷
commented

I have a fatal error with Drush 13.7.3 and Core 11.4.0-RC2

I guess due to the fact that even if marked as deprecated if the file is no more loaded automatically, it provokes fatal errors:

// @todo Combine functions differently in files to avoid unnecessary includes.
// Follow-up issue: https://www.drupal.org/node/1834298.
require_once __DIR__ . '/locale.translation.inc';

PS: I forgot to add the link to the Github issue: https://github.com/drush-ops/drush/issues/6579

nicxvan’s picture

nicxvan commented

Should be fixed in #3601433: Fix local_status deprecation by clearing source in a post update

grimreaper’s picture

grimreaper
Primary language French
Location France 🇫🇷
commented

Sorry, I forgot to post the link to the Github issue I have created.

https://github.com/drush-ops/drush/issues/6579

I have edited my previous comment but you already reacted (thanks!).

I am not sure this is the same problem.

PS: I will test tomorrow

PS2: maybe same problem, I was focused on the post_update hook, but the \Drupal::moduleHandler()->loadInclude('locale', 'inc', 'locale.translation'); may do the trick.

grimreaper’s picture

grimreaper
Primary language French
Location France 🇫🇷
commented

Tested this fixes the problem.

Thanks!