Locale module uses the table {locale_project} to store data about translatable projects. As suggested in #1804688: Download and import interface translations #14 this is cached data and runtime information. It is better stored in a state variable.

Locale module caches all information on available translation status (time stamp of remote and/or local translation in one state() variable. Fetching remote data is a costly operation. Therefore this will scale better for large sites with multiple languages if we can store the data in individual variables and add individual expiration.

This issue will also store the human readable project name and store it with the other project data for use in the user interface. See a follow-up note in #1804702: Display interface translation status.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Berdir’s picture

Note that I would recommend to not use state() but directly call into the keyvaluestore and use your own collection. drupal_container()->get('keyvalue')->get('locale') or something like that. Allows you to do stuff like getAll() if necessary.

And, you can also use the keyvalue.expirable service as well, which allows you to put stuff in there that automatically expires, if needed.

Sutharsan’s picture

Thanks for the pointers! I picked up something, but did not get into it yet.

Sutharsan’s picture

Issue summary: View changes

Updated issue summary.

skipyT’s picture

Assigned: Unassigned » skipyT
Issue summary: View changes
skipyT’s picture

Assigned: skipyT » Unassigned
Status: Active » Needs work
FileSize
10.42 KB

I added a new service called locale.project which stores the projects in a keyvalue collection and removed the table from the schema definition.

TODO:
- write an interface for the LocaleProjectStorage class (needed because it is a service)
- clean up the code
- perhaps we shall rewrite those code parts where the project is used as an stdClass object.
- document what are we storing in the value (needed because schema['locale_project'] was removed

akozma’s picture

Assigned: Unassigned » akozma
Sutharsan’s picture

The setup is pretty straight forward. Some remarks:

  1. +++ b/core/modules/locale/locale.compare.inc
    @@ -102,20 +96,11 @@ function locale_translation_build_projects() {
    +    $data['name'] = $name;
    +    \Drupal::service('locale.project')->set($project->name, $data);
     
    

    Needs more data of course.

  2. +++ b/core/modules/locale/locale.translation.inc
    @@ -64,11 +64,10 @@ function locale_translation_get_projects($project_names = array()) {
    +    $projects = \Drupal::service('locale.project')->getAll();
    +    array_walk($projects, function(&$project) {
    +      $project = (object) $project;
    +    });
       }
    

    getAll() method used, but not declared.

akozma’s picture

FileSize
15.47 KB
6.43 KB

Creating interface and fixing some issues.

skipyT’s picture

  1. +++ b/core/modules/locale/lib/Drupal/locale/LocaleProjectStorage.php
    @@ -0,0 +1,158 @@
    +    return !empty($this->cache) ? count($this->cache) : count($this->keyValueStore->getAll());
    

    we aren't sure here that all the projects are loaded in the cache, we shall return just: count($this->getAll());

  2. +++ b/core/modules/locale/lib/Drupal/locale/LocaleProjectStorage.php
    @@ -0,0 +1,158 @@
    +    return !empty($this->cache) ? $this->cache : $this->keyValueStore->getAll();
    

    here we need something like:

    static $all = FALSE;
    if (!all} {
      $this->cache = $this->getAll();
      $all = TRUE;
    }
    return $this->cache;
    
akozma’s picture

FileSize
15.6 KB
2.57 KB

Fix for calling getAll() and other small fixes.

akozma’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 9: replace-locale-project-table-9.patch, failed testing.

akozma’s picture

Status: Needs work » Needs review
FileSize
15.59 KB

Re-roll.

Status: Needs review » Needs work

The last submitted patch, 12: replace-locale-project-table-12.patch, failed testing.

Berdir’s picture

Status: Needs work » Needs review
FileSize
15.62 KB
989 bytes

Fixed typo in the filename and another small error that caused noticed in the one test that I tried to run.

Status: Needs review » Needs work

The last submitted patch, 14: replace-locale-project-table-14.patch, failed testing.

skipyT’s picture

Status: Needs work » Needs review
FileSize
15.3 KB

rerolled the patch and fixed the last failing test

skipyT’s picture

FileSize
15.92 KB

interdiff added

Gábor Hojtsy’s picture

Issue tags: +language-ui

@Berdir, @Sutharsan what do you think?

Sutharsan’s picture

Status: Needs review » Needs work

I have only two remarks:

  1. +++ b/core/modules/locale/locale.compare.inc
    @@ -19,9 +19,7 @@
    +  Drupal::service('locale.project')->deleteAll();
    

    Should be \Drupal::service().

  2. +++ b/core/modules/locale/src/LocaleProjectStorageInterface.php
    @@ -0,0 +1,106 @@
    +   * Returns a list project records.
    

    Small correction: "Returns a list of project records."

Sutharsan’s picture

Status: Needs work » Needs review
FileSize
15.28 KB

Rerolled the patch.

skipyT’s picture

@Sutharsan: thanks! We need some review here to get an RTBC :)

Sutharsan’s picture

Status: Needs review » Needs work

@skipyT, I only re-rolled, did not include the #19 comments. With that I am comfortably enough with it to RTBC the patch.

Sutharsan’s picture

Assigned: akozma » Unassigned

No response from @ocsilalala, removing assignment.

vijaycs85’s picture

Status: Needs work » Needs review
FileSize
15.29 KB
940 bytes

Here is #19

vijaycs85’s picture

looks good. overall, SQL-- and Service(keyValue)++.

  1. +++ b/core/modules/locale/locale.compare.inc
    @@ -91,6 +85,7 @@ function locale_translation_build_projects() {
    +      'name' => $name,
    

    Do we really need name? as the name is already the *key*?

  2. +++ b/core/modules/locale/locale.translation.inc
    @@ -329,13 +327,15 @@ function locale_cron_fill_queue() {
    +  $projects = array_filter($projects, function($project) {
    +    return $project['status'] == 1;
    +  });
    

    is it overkill to ask \Drupal::service('locale.project)->getAllActive() ?

As those two are minor/improvements, +1 to RTBC.

Gábor Hojtsy’s picture

Status: Needs review » Needs work
Issue tags: +sprint
  1. +++ b/core/modules/locale/locale.compare.inc
    @@ -102,17 +97,7 @@ function locale_translation_build_projects() {
    -    db_merge('locale_project')
    -      ->key('name', $project->name)
    -      ->fields(array(
    -        'name' => $project->name,
    -        'project_type' => $project->project_type,
    -        'core' => $project->core,
    -        'version' => $project->version,
    -        'server_pattern' => $project->server_pattern,
    -        'status' => $project->status,
    -      ))
    -      ->execute();
    +    \Drupal::service('locale.project')->set($project->name, $data);
    

    Prior code has $project, new code has $data, is this doing what the prior code did?

  2. +++ b/core/modules/locale/locale.translation.inc
    @@ -57,8 +57,7 @@ function locale_translation_get_projects($project_names = array()) {
    -    $row_count = db_query_range('SELECT 1 FROM {locale_project}', 0, 1)->fetchField();
    -
    +    $row_count = \Drupal::service('locale.project')->countProjects();
    

    Prior code was not an actual count, new code is an actual count, yay :)

vijaycs85’s picture

#26.1 - Yes, it has data already for other purposes:

    $data += array(
      'name' => $name,
      'version' => isset($data['info']['version']) ? $data['info']['version'] : '',
      'core' => isset($data['info']['core']) ? $data['info']['core'] : \Drupal::CORE_COMPATIBILITY,
      // A project can provide the path and filename pattern to download the
      // gettext file. Use the default if not.
      'server_pattern' => isset($data['info']['interface translation server pattern']) && $data['info']['interface translation server pattern'] ? $data['info']['interface translation server pattern'] : $default_server['pattern'],
      'status' => !empty($data['project_status']) ? 1 : 0,
    );
    $project = (object) $data;

that's why we have $project-> on the query.

#26.2 - Yeah, it's much clear now. DX++

Gábor Hojtsy’s picture

Status: Needs work » Reviewed & tested by the community

Thanks @vijaycs85 my concerns were unfounded there. While this can be improved later on, it is already a good step, yup :)

webchick’s picture

Status: Reviewed & tested by the community » Fixed

Looks like good clean-up.

Committed and pushed to 8.x. Thanks!

  • webchick committed 1dd9406 on 8.0.x
    Issue #1842362 by vijaycs85, Sutharsan, Berdir, ocsilalala, skipyT:...
Gábor Hojtsy’s picture

Issue tags: -sprint

More D8-like code++ Yay, thanks!

Status: Fixed » Closed (fixed)

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

Berdir’s picture