Problem/Motivation

Postponed on: #3037156: Modernize locale history functions

locale.module scans the file system for all po files, even if the filename is already known. All items from the KV store are fetched in locale_translation_get_status even if most of the time one project is relevant. PHP spends a huge amount of time on IO and serialize/unserialize

Its reproduceable on my site with over 200 modules and 38 languages. I have attached the benchmark script. Before: 5 minutes, After: ~7 seconds.

Steps to reproduce

Enable 200 modules
Enable 38 languages
Check for updated languages

Proposed resolution

Skip the directory check
#3586654-14: Avoid scanning the file system for local po files has details on where this functionality was lost years ago.

Remaining tasks

Review

User interface changes

N/A

Introduced terminology

N/A

API changes

N/A

Data model changes

N/A

Release notes snippet

TBD

CommentFileSizeAuthor
#2 3586654.patch1.24 KBwebflo

Issue fork drupal-3586654

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

webflo created an issue. See original summary.

webflo’s picture

webflo commented
StatusFileSize
new 3586654.patch1.24 KB

ishani patel made their first commit to this issue’s fork.

ishani patel opened merge request !15563

ishani patel’s picture

ishani patel commented

I've raised MR from the patch.
Thank you!

nicxvan’s picture

nicxvan commented
Issue tags: +Needs issue summary update

Please share the benchmarks you did before and after.

webflo’s picture

webflo commented
Issue summary: View changes
Status: Active » Needs review
Issue tags: -Needs issue summary update
andypost’s picture

andypost
he/him
Primary language Russian
commented
Issue tags: +DevDaysAthens2026
andypost’s picture

andypost
he/him
Primary language Russian
commented
Parent issue: » #1577902: [META] Remove all usages of drupal_static() & drupal_static_reset()
andypost’s picture

andypost
he/him
Primary language Russian
commented
Status: Needs review » Reviewed & tested by the community
Related issues: +#3037156: Modernize locale history functions

Looks ready as the static will be fixed in related

nicxvan’s picture

nicxvan commented
Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs change record

I think we need a way to enable the old behavior, some sites may rely on that. Maybe a setting?

I'm not sure if this change needs subsystem maintainer review or not.

webflo’s picture

webflo commented

I looked into it and tried to understand how the old behaviour was supposed to work.

- \Drupal\locale\LocaleSource::buildServerPattern builds the pattern/replaces %project %langcode etc.
- \Drupal\locale\LocaleSource::sourceCheckFile runs preg_quote on the pattern and calls \Drupal\Core\File\FileSystemInterface::scanDirectory with a string escaped string from preg_quote
- Afaik, all special chars are escaped

To me it looks like a bug and just inefficient. Even if the regex would return multiple files, \Drupal\locale\LocaleSource::sourceCheckFile would continue only with one.

// from \Drupal\locale\LocaleSource::sourceCheckFile

if ($files = $this->fileSystem->scanDirectory($directory, $filename, ['key' => 'name', 'recurse' => FALSE])) {
  $file = current($files);
  ...
  return $file
}

I don't think it's worth preserving the old behaviour. I'm looking forward to getting some insights from the subsystem maintainers.

webflo’s picture

webflo commented

Benchmark command for drush.

time drush php-eval '\Drupal::moduleHandler()->loadInclude("locale", "inc", "locale.compare"); locale_translation_check_projects_local();'

penyaskito’s picture

penyaskito
he/him
Primary language Spanish
Location 🧑🏽‍🌾 Seville 💃 Andalusia, UTC+2 🇪🇺
commented

Spent 1.5 hours doing archeology.

See #1804688: Download and import interface translations where it was introduced.

If I'm right, we were tracking all the versions of the files in locale_file db (see $source passed by reference, and setting ->files, which would end up in the locale_very_long_name_yada_yada_save() method)

In #1998056: Automatically update interface translations using cron this behavior disappeared very early on patch #7, without further discussion. If I'm understanding the code right, we started tracking only the last download of the po file, but still doing the expensive scan (this was cached on state, so not that expensive, moved later to key value store, then I lost track if we still do for 11.x).

So IMHO we are fine to do this. If we accidentally removed some functionality, it disappeared in 8.8.0, and no-one screamed about it.

nicxvan’s picture

nicxvan commented

I think that covers it for me I think, though I need to read the linked issues myself in depth too.

Since this branch was already not working and this is strictly a performance change now so we need a CR?

We should update the issue summary with the approach taken though.

nicxvan’s picture

nicxvan commented
Title: Avoid scanning the file system for local po files » [pp-1] Avoid scanning the file system for local po files
Status: Needs work » Postponed
Related issues: +#3037031: Convert locale.compare.inc to a service

I actually think we need to postpone this on #3037031: Convert locale.compare.inc to a service.

Honestly I came across the same bug when trying to fix the LocaleSource object.

If I'm being honest this fix will likely be rolled into the other issue since we got signoff on scope and the other issue has to get in in for 11.4 in order to prevent a lot of rework and deprecations.

Once we confirm the other issue gets in I'll close this out and ensure everyone gets credit according to the Drupal core policies.

Thanks for the work here everyone! This is fantastic.

nicxvan’s picture

nicxvan commented
Issue summary: View changes

This is getting closer, but now should be postponed on: #3037156: Modernize locale history functions

We're doing the status stuff next so I'm not sure we should do those changes here.
If the update history changes are not needed here this doesn't need to be postponed at all.

berdir’s picture

berdir
Primary language German
Location Switzerland
commented
Title: [pp-1] Avoid scanning the file system for local po files » Avoid scanning the file system for local po files
Status: Postponed » Needs work

The changes to the other functions make sense, but we're already handling that in #3037156: Modernize locale history functions for file history and will completely redesign the translation status API to support single lookup and probably memory caching in #3590050: [pp-1] Deprecate and replace locale_status related functions

Let's focus on sourceCheckFile here, then this is not blocked and can happen now. Just needs a rebase to remove the extra changes.

nicxvan’s picture

nicxvan commented
Status: Needs work » Needs review

Ok I pushed up changes reflecting 18, as mentioned this issue now only focuses on sourceCheckFile the other two optimizations will be addressed as part of the broader refactoring going on in locale.

nicxvan’s picture

nicxvan commented
Issue summary: View changes
Parent issue: #1577902: [META] Remove all usages of drupal_static() & drupal_static_reset() »
Related issues: +#3590050: Deprecate and replace locale_status related functions

This isn't technically a behavior change so I don't think it requires a CR.

It's still a pretty big performance improvement so a release note might be nice.

berdir’s picture

berdir
Primary language German
Location Switzerland
commented
Status: Needs review » Reviewed & tested by the community

I think this is ready, already confirmed by others, we just simplified it since then and focused on this specific change. Similar to #14, I saw this hasn't been changed much since it got into core in 2012.

It's challenging to look at anything in locale without urges to refactor everything, fix up docs and so on, translation.inc deprecation didn't get the same close reviews that others issues later on got, but parts of the unrelated issues with that function will be cleaned up with #3590050: [pp-1] Deprecate and replace locale_status related functions

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Version: main » 11.x-dev
Status: Reviewed & tested by the community » Fixed
Issue tags: -Needs change record

Committed and pushed 1d9e127ea48 to main and 8e59a7a3dab to 11.x. Thanks!

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.

  • alexpott committed 8e59a7a3 on 11.x 
    perf: #3586654 Avoid scanning the file system for local po files

By:...

  • alexpott committed 1d9e127e on main 
    perf: #3586654 Avoid scanning the file system for local po files

By:...

Status: Fixed » Closed (fixed)

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