As part of the l10n_update integration in core this issue adds the function 'Get status of local and remote translation source'. See the UML in #1191488: META: Integrate l10n_update functionality in core for the big picture where this fits in. This patch includes the following parts of the UML:
- Get available translation sources
- Get status of local translation source
- Get status of remote translation source
- Batch get remote translation status
Comment | File | Size | Author |
---|---|---|---|
#30 | locale-remote-status-1742894-30.patch | 45.32 KB | Sutharsan |
#30 | interdiff-1742894-27-30.txt | 11.08 KB | Sutharsan |
#27 | locale-remote-status-1742894-27.patch | 43.67 KB | Sutharsan |
#27 | interdiff-1742894-26-27.txt | 12.5 KB | Sutharsan |
#26 | locale-remote-status-1742894-26.patch | 43.37 KB | Sutharsan |
Comments
Comment #2
Sutharsan CreditAttribution: Sutharsan commentedTagging for D8MI
Comment #3
Sutharsan CreditAttribution: Sutharsan commentedlocale-remote-status-0.patch queued for re-testing.
Comment #5
webflo CreditAttribution: webflo commentedRerolled.
Comment #6
pp CreditAttribution: pp commentedUse $language or $langcode instead of $lang
Comment #7
Sutharsan CreditAttribution: Sutharsan commentedIn this patch:
Still todo:
Comment #8
Sutharsan CreditAttribution: Sutharsan commentedChanging title to match the increased scope of the patch.
Comment #9
attiks CreditAttribution: attiks commentedAmazing work, some small comments
is the space needed in front of ++
what is the unit: seconds, minutes, ...
add a blank line to improve reading.
if $check_local is FALSE, does this mean that $check_remote is TRUE? If not add an elseif ($check_remote)
is this a root folder?
this will need to be changed to translations:// once #1658842: Introduce a translations:// stream wrapper to access the .po file directory is committed
Comment #10
webflo CreditAttribution: webflo commentedThe patch looks good. But still a lot of @todos. Here are some minors.
Typo: placeholders.
for supported placeholders. I think its better to standardize the wording.
Comment #11
Gábor Hojtsy@Sutharsan: can you work on this going forward or should we try and find others to help out?
Comment #12
Gábor HojtsyAdd tag.
Comment #13
Gábor HojtsyElevating to major as it should have been.
Comment #14
Sutharsan CreditAttribution: Sutharsan commentedAt the last day of the code sprint in München started to realize that the way the batch is coded is wrong (a lot of processing occurs after the batch function is called). This may or may not work now, but certainly will not work if the batch is followed by the second batch process of downloading and importing the translation. Therefore I will need to rewrite the code to properly build the batch.
Thanks webflow and attiks for your reviews. I will include your remarks, but not make a separate patch from it. You probably won't recognise the code once I'm done ;)
Comment #15
Sutharsan CreditAttribution: Sutharsan commentedOk, that wasn't too hard (once I understood the batch API).
Done:
Todo:
Comment #16
Gábor HojtsyComment #17
Gábor HojtsyJust a quick review. I this this looks good, minus some of the @todo items. I do not agree with this TODO though:
I think this is fine as-is, its a one off check and therefore would have too much of an overhead to get a stream wrapper.
Over 80 chars.
I think this new function looks like a good wrapper. I'm not sure if you need the debug stuff around for future working patches, it looks pretty straightforward :)
I've also asked webflo and attiks to take a look, don't wait for them with any updates though IMHO.
Comment #18
Gábor Hojtsy@Sutharsan: Do you need any help with this? What kind of help is needed?
Comment #19
Sutharsan CreditAttribution: Sutharsan commented@Gábor, thanks for the offer, but I'm fine. Writing the test took more time because I had to climb a lot of hills to get it to work.
This is a working version with still some work todo, but good enough for a review.
Done:
Todo:
Comment #20
Sutharsan CreditAttribution: Sutharsan commentedThe Localization Update module has three update modes: "Local files and remote server", "Local files only" and "Remote server only". I consider removing the the "Remote server only" option and thus reducing it to one choice (checkbox): "Local files only". I'm looking for some feedback on this.
Option "Local files only" not selected
This is the default. Both remote and local translation files will be investigated and the most recent translation will be used to import. Local files can be either downloaded files stored in the local file system, files included in custom modules or files included in profiles as part of a distribution. Downloaded files (e.g. from l.d.o) will only be saved in the local file system if a Translation directory has been set in the configuration. This option has no name because it is difficult to catch it in a few words. If no Translation directory has been defined it will import remote files from the translation server and will import local po files from (custom) modules or distributions which include their own translation files and have defined this in the .info file or with hook_locale_translation_projects_alter().
Option "Local files only" selected
With this option selected only local po files will be considered for import. This option is typically used for multi site installations which share a common directory for po files. One of these sites downloads the po files (Option "Local files only" not selected) from a translation server and stores the in the shared Translation directory. An other use of this option is in cases where a staging system is used to feed the site with new interface translation and no remote translation sources are used.
What do we loose with this change?
We loose the option where only remote sources are used. But since we now allow custom modules to define a local file as their translation source, we need to make exceptions on this rule any way. Because with a strict use of a "Remote server only" option, custom modules would never impor their local po file. I can't think of a use case where there is a need for only remote translation sources and all local sources should be ignored. Both code and interface simplicity will improve by dropping this option.
Comment #21
Sutharsan CreditAttribution: Sutharsan commentedDone:
locale_translation_source_build()
.I think we can postpone the admin page until a later issue.
Comment #22
Gábor HojtsyReflecting on #21, I think its fine to drop the remote only option. Otherwise trying to get you more reviewers :)
Comment #23
Gábor HojtsyOh and for the user interface I assume that would come somewhere between #1804688: Download and import interface translations and #1804702: Display interface translation status.
Comment #24
Sutharsan CreditAttribution: Sutharsan commentedOnce #1804688: Download and import interface translations is brought to a decent state, all the variables and settings will be on the radar and a configuration page can be added. #1804702: Display interface translation status is the issue for the update interface and integrating everything into other processes (module enable/disable, add/remove language, etc.)
Comment #25
attiks CreditAttribution: attiks commentedNice patch, some typos and minor remarks
Maybe add a comment that we enable German by default.
I know it doesn't really matter, but Drupal 7?
file --> files
two spaces between files get
projct --> project
may be removed
dot after custom_module_one may be removed.
No idea how we should right this, since modules is now in the root folder?
proces --> process
should this be remote or local files?
... stored *in* a state ...
one we too many
... But we also ...
Maybe add a @see that references the 'preceding'.
Why is this cloned? If it's needed (and I assume it is), add a comment.
and --> are
Maybe explain why this is needed
another clone
remove 'or a local ', because the function name says that.
remove location
is the @todo still needed?
i think you have to add @see locale_menu()
Try to remove the link out of the message, so it's easier to translate.
debug
i assume this is for a follow up issue, if it exists link to it in a comment.
Type: transaltion --> translation
I know, not part of the patch, but there's a typo translatione --> translation
This sounds strange: "Don't hide the module' or something like that?
Comment #26
Sutharsan CreditAttribution: Sutharsan commentedAs proposed in #21, the 'remote check' option is now dropped.
Comment #27
Sutharsan CreditAttribution: Sutharsan commentedWe can just change it to 'modules/custom/example_module ...' It does not matter where the directory is as long as the file is readable.
One clone removed (I thought I did before). But to the why? I thought it was obvious, but perhaps I am missing something. $project is an object and is therefore by reference. Added one line of comment.
Other comments included.
Comment #28
attiks CreditAttribution: attiks commentedI think this looks good, maybe add the notes from #24 into the issue summary.
Comment #29
webchickMost of these are nit-pickish but this patch introduces some spelling/grammar errors which we need to fix before commit. Also, some work on the naming to make it more obvious what's happening here.
Nice work on the batch API stuff, and the tests! Really exciting to see l10n_update in core pushing forward! :D
I don't think this variable is descriptive enough; I had to grep around for context in the rest of the patch to understand. From reading the docs at LOCALE_TRANSLATION_CHECK_ALL and friends, I think it should rather be "update_check_mode" or something (and then those constants named LOCALE_TRANSLATION_UPDATE_CHECK_X)
I would also suggest ALL should be BOTH or REMOTE_AND_LOCAL or something like that. "All" makes it sound like there are multiple choices here, but there are really only two, and REMOTE on its own is not one of them.
I was going to whine here about adding a new variable and not using CMI, but I noticed this variable already exists in 8.x. :) So ignore me. :D
"tests" its timestamp.
String? Not an integer?
"Drupal" should be capitalized.
", another" rather than ". An other"
Great job with these comments, btw. Very clearly describe what's going on.
The test "checks" whether
No ' in scenarios.
"Set up"
It probably makes sense to create constants for these so they are self-documenting.
"it's"
Can we pull those two state()->delete() lines into an API function and call it from this form submit handler, rather than embedding the logic in here?
Is this legit? I thought you needed to do
t('blah blah <a href="@something">blah</a>')
?Comment #30
Sutharsan CreditAttribution: Sutharsan commentedAll comments processed.
Comment #31
attiks CreditAttribution: attiks commentedThis looks good to me, great job!
Comment #32
webchickAwesome, thanks a lot! Committed and pushed to 8.x. Not sure if this needs a change notice?
Comment #33
Gábor HojtsySuperb, thanks all! Let's move to #1804688: Download and import interface translations and #1804702: Display interface translation status :)
Comment #34
Gábor HojtsyIt did not take the sprint tag removal for some reason.
Comment #35.0
(not verified) CreditAttribution: commentedAdding UML details.