For integrating the l10n_update module in core (see #1191488: META: Integrate l10n_update functionality in core), we need to reduce the need for code duplication. Currently, the l10n_update module copies various functions from update module. See http://drupalcode.org/project/l10n_update.git/blob/refs/heads/7.x-1.x:/l.... These functions include update_get_project_name(), _update_process_info_list(), etc.

The module copies this code for two reasons (a) to avoid all the other code depth the update module includes, and gather the data we need in a more speedy fashion plus (b) so we don't need to depend on update.module as a requirement. I think for Drupal 8, its fine if we do depend on update module I think, although ideally it would be an API module to gather project data + a module to actually display and check for updates, so we can rely on the API and people can still disable the update module UI if they want to, but I don't think there are such plans right now for update module. Let me know if that is the case though.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Gábor Hojtsy’s picture

Issue tags: +language-ui

Adding UI language translation tag.

Gábor Hojtsy’s picture

Issue tags: +sprint

Just did a quick check and

1. update_get_project_name() and l10n_update_get_project_name() are the same.

2. _update_process_info_list() has some additional functionality vs. _l10n_update_project_info_list(), eg. the core function handles sub themes, base themes and sub modules and, it does however seem to do two things that are likely bad for us:

a) it skips hidden modules and themes (we might want to make this configurable?)
b) it uses http://api.drupal.org/api/drupal/modules%21update%21update.compare.inc/f... to filter the info file data and will not return information that we want to let .info files provide for custom localization servers (like 'project status url' for custom update server)

3. update_get_projects() and l10n_update_project_list() are similar, except l10n_update does not do any caching (for some reason) and the alter hook is differently named to provide for specific altering (which I don't think anybody actually implemented ever?)

I believe it is mostly 2b that update status proved to not be reusable. And that sounds like a pretty easy fix, right? :)

I looked at what we don't have of l10n_update in core yet, and this seemed like it might be the easiest to continue with (I initially thought the file status tracking ('l10n_update_file' schema and its management) would be easier but that is tied to projects too, so looks like it is best if we go identify those projects first :) So moving this onto our sprint :)

Gábor Hojtsy’s picture

Title: Refactor update data collection to support localization update » Extend update data collection to support localization update
Status: Active » Needs review
FileSize
1.5 KB

So this would be the most basic patch for this. It is very simple.

1. It lets us include hidden projects (we need to download translations for them even though they are supposed to be hidden). I'm not sure why we are not looking at updates for them, I think we should hide them later in display, no?

2. It includes three more info file keys, which used to be 'l10n server', 'l10n url' and 'l10n path' in the l10n_update module. I named those in anticipation of locale being eventually renamed to "interface translation" module. We can fiddle with this name later.

I'm not 100% sure we should introduce (2) in update module as-is. I think we can introduce an alter hook for the .info file keys we track and implement that in locale module and cache the list in the function so we don't need to call the alters for each file. That sounds better?

In any case, this seems like the extent of service a core built-in localization update module would need from core update module. This feature will be reliant on update module turned on.

Sutharsan’s picture

Both the approach and the patch look good to me. However, I will leave in on 'needs review' to check if the caching of update_get_projects() may get us into trouble. Want to check if the lack of caching in l10n_update is related to updating the project list afte/before enabling/disabling modules. Or perhaps absence of this was only needed in D6 and not in D7, which changed the update proces.

Gábor Hojtsy’s picture

We should be able to clear the cache from the outside, no?

Gábor Hojtsy’s picture

Here is a bit more generalized version with alter hook for info file property whitelist extension, so other contribs can do this too. It is cached for the request, so it should not have any noticeable impact. I've pinged @dww about this, but I'm not sure he will have any time. I think this is pretty darn simple in itself, so we don't need special approvals :)

Sutharsan’s picture

I've added caching to l10n_update_project_list() and it seems to continue working. But update_get_projects() is not part of this patch and therefore the matter of caching is now not irrelevant.

Can we add a solution for custom modules to this patch? Custom modules do not have a 'project' defined and are therefore not recognized by _update_process_info_list(). See http://drupal.org/node/1430004 for the Localization Update module.

I see two alternatives:

  • A flag in the .info file to indicate the module has a local translation file
  • An alter hook with which the module makes itself known (after being ignored by _update_process_project_info()).

Since this issue introduces new .info flags, I prefer the first solution in the form of:

interface translation file = my_po_file_name_base
Gábor Hojtsy’s picture

We don't want custom modules to provide a "project" to avoid them being pinged back to d.o right? That is the base problem which motivated needing to add a different .info key? I'm fine with "interface translation file" (but it would not be a file name really, it would rather be a "project name", right?). So why not "interface translation project"?

Sutharsan’s picture

An "interface translation project" indeed.
With this patch

  • _update_process_info_list() now accepts a new "project category" parameter. I did not want to add yet another parameter to the function and therefore combined all into a conditions array (type, status, hidden, category).
  • 'interface translation project' is added to the white list.
  • update_get_project_name() accepts 'interface translation project' from the .info file as project name.
Gábor Hojtsy’s picture

Why do we need this category argument? (Also, having the conditions array with required items looks pretty unusual. So I'm wondering if we need to even introduce the array.

Sutharsan’s picture

We need a way to distinguis between the interface translation projects and the normal projects. Without any measures _update_process_info_list() outputs both without distinction. We either use a parameter to instruct the function to output the right data or the function outputs the projects grouped in an array bases in their "category".

Gábor Hojtsy’s picture

@Sutharsan: but we'd get that information in the info array structure and do whatever we want to do with that on the outside. I don't think we should build-in interface translation related assumptions / functionality in update module if we can do it based on its data just as well outside. That is part of the reason I added the .info file whitelist expansion as an alter hook.

Sutharsan’s picture

@Gábor, indeed it's no beauty add more hard coded conditions. This patch has a more generic approach, it adds a callback to preprocess the project data. It requires the locale module to provide a callback function like:

function locale_info_list_preprocess(&$file) {
  // Include hidden files if desired.
  if (variable_get('locale_projects_include_hidden, FALSE) && isset($file->info['hidden'])) {
    unset($file->info['hidden']);
  }
  
  // Include interface translation projects.
  // To enable import of a local translation file the custom module must
  // define 'interface translation project = mycustom' in its .info file.
  if (isset($file->info['interface translation project'])) {
    $file->info['project'] = $file->info['interface translation project'];
  }
}

An alternative approach is to use an alter function which is called after _update_process_info_list() (this still requires patch #6). An example alter function of a custom module which wants its (local) translation file to be imported:

function mycustom_locale_interface_translation_projects_alter(&$projects) {
  // Identify this custom module to enable import of translation file stored locally.
  // Most of this data duplicates .info file definitions.
  $projects['mycustom'] = array(
    'name' => 'My custom module',
    'project' => 'mycustom',
    'core' => "8.x",
    'version' => "8.x-1.0",
    'project_type' => 'module',
     // Optional definitions if the module provides translations via a translation server.
    'info' => array(
      'interface translation server' => 'example.com',
      'interface translation url' => 'http://example.com/files/translations/l10n_server.xml',
      'interface translation pattern' => 'http://example.com/files/translations/%core/%project/%project-%release.%language.po',
    ),
  );
}
Gábor Hojtsy’s picture

@Sutharsan: Naming these preprocess functions could be *very* confusing vs. the theme preprocess functions we have. It totally looks like you just put an alter on it (sent by reference, changing internal properties). The existing alter patterns seems like would apply better. For that, we maybe want to introduce an identifier key sent over to the function which would be 'update' by default, and locale could pass on 'locale', which its alter could understand. Something along those lines. I know we talked through this callback idea in IRC but I've imagined it a bit differently :) This way, it looks like what Drupal otherwise uses an alter for :)

Gábor Hojtsy’s picture

Attached my expansion based on that thinking:

1. Instead of a preprocess callback, I've added a process id (we might be able to find a better name for this).
2. Converted the preprocess callback thing to an alter hook, that gets this process id.
3. Passed on the process ID to the info file whitelist altering too (and cached by process id).
4. Added API docs for both alters proper.

The great advantage of this is that the regular update data collection has absolutely 0 memory footprint increase for the info file storage (unlike previous patches), because we'll not store those with the update module cache. That module does not need that data. However, we added an extensible feature where people can affect the file object as well as the info file properties kept. So we can say for our process with 'interface translation', the project can be mimicked, but that would not affect the update status processing at all.

Status: Needs review » Needs work

The last submitted patch, extend-update-for-interface-translation-15.patch, failed testing.

Gábor Hojtsy’s picture

Status: Needs work » Needs review
FileSize
8.14 KB

Did not initialize whitelist properly. Fixed that.

Sutharsan’s picture

@gabor, We could do without changing _update_process_info_list() and adding hook_update_process_info_list_alter(). Process the file data in $list (or $module_data, $theme_data) before it is handed off to _update_process_info_list(). This processing consists of looping through all elements and do the same things a would be done in hook_update_process_info_list_alter(). No time to write code, just dumping my thought.

Gábor Hojtsy’s picture

You basically suggest breaking _update_process_info_list() into a wrapper and a processing function? Where the wrapper would do the filtering? I think there are so many possible way we can solve this, we should pick a way and run with it :) We are possibly running in circles here. We have very well defined needs here after all :)

Sutharsan’s picture

I just don't know when to stop ;)
No, not a wrapper, but a function that is run before _update_process_info_list(). This prevents changing _update_process_info_list() and gives the same flexibility. 'locale_prepare_project_list()' in the example below.

An example of the locale variation of update_get_projects():

function locale_translation_get_projects() {
  ...
  $module_data = system_rebuild_module_data();
  $theme_data = system_rebuild_theme_data();
  // Process the module data to include hidden modules
  // and custom modules containing interface translation.
  locale_prepare_project_list($module_data);
  _update_process_info_list($projects, $module_data, 'module', TRUE);
  _update_process_info_list($projects, $theme_data, 'theme', TRUE);
  if (variable_get('update_check_disabled', FALSE)) {
    _update_process_info_list($projects, $module_data, 'module', FALSE);
    _update_process_info_list($projects, $theme_data, 'theme', FALSE);
  }
  // Allow other modules to alter projects before fetching and comparing.
  drupal_alter('locale_translation_projects', $projects);
  ...
}
Gábor Hojtsy’s picture

I think that would indeed make lots of sense and we could also change to directly pass on $whitelist (the whole whitelist) or $additional_info_whitelist (to extend the whitelist) or something along those lines instead of a one-off made up $process_id and doing altering, which requires local understanding instead of just leveraging what was already there. Hm.

Sutharsan’s picture

I've removed the hooks and added an $additional_whitelist parameter.
I've considered the use of $alternative_whitelist instead. Not much difference; '_info_file_ctime' is required, only 4 keys are not used by localization_update module. I've reached the point where changes are to me no longer improvements, only changes.

Gábor Hojtsy’s picture

Status: Needs review » Reviewed & tested by the community

That looks beautiful and simple, and as we explored above in detail just the extent of change we need to get started with building this for interface translation.

andypost’s picture

Let's get @dww review on it

Gábor Hojtsy’s picture

I've discussed this with @dww in email and he said he likely does not have time these days (busy with stuff and then D7 sprint for drupal.org), and the extent of the change is minimal, so it should be fine AFAIS.

dww’s picture

#22 seems okay to me. Certainly way better than some of the earlier proposed patches. ;)

My only lingering concern is that _update_process_info_list() is supposed to be an internal function for update.module (hence the leading underscore in the name) and yet it's now being used as a public-facing API. This optional parameter isn't used anywhere by update.module, only by external callers. In that case, seems more appropriate to rename it to update_process_info_list(). If it's going to have this additional parameter to make it more friendly for others to call it, we should just move it into the public-facing API for update.module instead of leaving it as if it's a private method.

I'm not going to un-RTBC it over that, but IMHO that'd be an improvement.

Thanks,
-Derek

dww’s picture

Status: Reviewed & tested by the community » Needs review
FileSize
4.12 KB

Like so.

Gábor Hojtsy’s picture

Status: Needs review » Reviewed & tested by the community

Absolutely agreed. Tiny change, thanks for your approval :)

dww’s picture

Side note: does locale_prepare_project_list() now take care of the hidden module problem somehow? It's not clear how that's going to work given the patch. Are you just altering $module_data to unhide all modules before you call update_process_info_list() or something? That does seem ugly. Is it worth adding another param here to control the skip-hidden-stuff behavior?

Gábor Hojtsy’s picture

Yes, we could alter that data on the outside however we see fit. :) I think it looks interesting that update module would skip checking for hidden modules. I can see how it would not display those if they are submodules of others, but they can have security updates just as well and can make the site insecure. If we assume that only submodules are ever hidden, then it is irrelevant for localization update too, and this is just good as-is with the hidden modules not present in the list, since the localization update functionality also only considers the d.o project level, not the submodule level. So I think we are fine here, and we can return to investigating how people use the hidden module "feature". I know Drupal Gardens uses it to hide top level projects wholesale. However, it is also a hosted service where the module updates come with the service so you are not exposed to update status. And I can imagine in practice hidden modules are almost exclusively used to hide sub-modules.

Sutharsan’s picture

locale_prepare_project_list would look something like this:

locale_prepare_project_list(&$module_data) {
  foreach ($module_data as $file) {
    // Include hidden files if desired.
    if (variable_get('locale_projects_include_hidden, FALSE) && isset($file->info['hidden'])) {
      unset($file->info['hidden']);
    }
  
    // Include interface translation projects.
    // To enable import of a local translation file the custom module must
    // define 'interface translation project = mycustom' in its .info file.
    if (isset($file->info['interface translation project'])) {
      $file->info['project'] = $file->info['interface translation project'];
    }
  }
}
Gábor Hojtsy’s picture

@Sutaharsan: well, as discussed above, we only need higher level projects for localization update too, so its fine to hide lower level modules so long as we get the higher ones. If it makes sense to unhide hidden modules for localization update (because high level projects are hidden), then it equally makes sense for update status. So anyway, the conclusion sounds like that we would not need an option for it either way, and we'll not need to mangle it from the outside as Sutharsan's code showed.

Dries’s picture

Status: Reviewed & tested by the community » Fixed

Committed to 8.x. Thanks!

Gábor Hojtsy’s picture

Issue tags: -sprint

Thanks all! Added a changelog entry for this: http://drupal.org/node/1548376

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