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.
Comments
Comment #1
Gábor HojtsyAdding UI language translation tag.
Comment #2
Gábor HojtsyJust 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 :)
Comment #3
Gábor HojtsySo 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.
Comment #4
Sutharsan CreditAttribution: Sutharsan commentedBoth 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.
Comment #5
Gábor HojtsyWe should be able to clear the cache from the outside, no?
Comment #6
Gábor HojtsyHere 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 :)
Comment #7
Sutharsan CreditAttribution: Sutharsan commentedI'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:
_update_process_project_info()
).Since this issue introduces new .info flags, I prefer the first solution in the form of:
Comment #8
Gábor HojtsyWe 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"?
Comment #9
Sutharsan CreditAttribution: Sutharsan commentedAn "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).update_get_project_name()
accepts 'interface translation project' from the .info file as project name.Comment #10
Gábor HojtsyWhy 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.
Comment #11
Sutharsan CreditAttribution: Sutharsan commentedWe 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".Comment #12
Gábor Hojtsy@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.
Comment #13
Sutharsan CreditAttribution: Sutharsan commented@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:
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:Comment #14
Gábor Hojtsy@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 :)
Comment #15
Gábor HojtsyAttached 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.
Comment #17
Gábor HojtsyDid not initialize whitelist properly. Fixed that.
Comment #18
Sutharsan CreditAttribution: Sutharsan commented@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.
Comment #19
Gábor HojtsyYou 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 :)
Comment #20
Sutharsan CreditAttribution: Sutharsan commentedI 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():
Comment #21
Gábor HojtsyI 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.
Comment #22
Sutharsan CreditAttribution: Sutharsan commentedI'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.
Comment #23
Gábor HojtsyThat 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.
Comment #24
andypostLet's get @dww review on it
Comment #25
Gábor HojtsyI'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.
Comment #26
dww#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
Comment #27
dwwLike so.
Comment #28
Gábor HojtsyAbsolutely agreed. Tiny change, thanks for your approval :)
Comment #29
dwwSide 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?
Comment #30
Gábor HojtsyYes, 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.
Comment #31
Sutharsan CreditAttribution: Sutharsan commentedlocale_prepare_project_list would look something like this:
Comment #32
Gábor Hojtsy@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.
Comment #33
Dries CreditAttribution: Dries commentedCommitted to 8.x. Thanks!
Comment #34
Gábor HojtsyThanks all! Added a changelog entry for this: http://drupal.org/node/1548376