System_modules_submit() contains the following code that should be put in locale_modules_enabled():

<?php
 
// Notify locale module about module changes, so translations can be
  // imported. This might start a batch, and only return to the redirect
  // path after that.
 
module_invoke('locale', 'system_update', $new_modules);
?>
Files: 
CommentFileSizeAuthor
#25 545518-follow-up-fix.patch670 bytesbfroehle
PASSED: [[SimpleTest]]: [MySQL] 31,592 pass(es).
[ View ]
#13 drupal.locale-import.13.patch2.63 KBsun
PASSED: [[SimpleTest]]: [MySQL] 31,516 pass(es).
[ View ]
#4 drupal.locale-import.4.patch2.71 KBsun
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch drupal.locale-import.4.patch.
[ View ]
#1 locale_modules_enabled_00.patch1.76 KBXano
Failed: 12071 passes, 2 fails, 5 exceptions
[ View ]

Comments

Issue tags:+Quick fix
StatusFileSize
new1.76 KB
Failed: 12071 passes, 2 fails, 5 exceptions
[ View ]

Assigned:Unassigned» Xano
Status:Active» Needs review

Status:Needs review» Needs work

The last submitted patch failed testing.

Title:Add locale_modules_enabled()Add locale_modules_installed() and locale_themes_enabled()
Assigned:Xano» sun
Status:Needs work» Needs review
StatusFileSize
new2.71 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch drupal.locale-import.4.patch.
[ View ]

Good catch!

Any feedback?

#4: drupal.locale-import.4.patch queued for re-testing.

Title:Add locale_modules_installed() and locale_themes_enabled()Move Locale module specific code out of module.inc and system.module

#4: drupal.locale-import.4.patch queued for re-testing.

Version:7.x-dev» 8.x-dev

Although badly needed, this is D8 material according to the rules (I had to learn today). It may be backported at a later point in time (though that's unlikely).

Version:8.x-dev» 7.x-dev

Hm. This doesn't really affect any functionality and merely moves code where it belongs.

Issue tags:-Quick fix

#4: drupal.locale-import.4.patch queued for re-testing.

Status:Needs review» Needs work
Issue tags:+Quick fix

The last submitted patch, drupal.locale-import.4.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new2.63 KB
PASSED: [[SimpleTest]]: [MySQL] 31,516 pass(es).
[ View ]

Re-rolled against HEAD.

Looks good to me, but I haven't got a D7 install here at the moment, so I cannot yet confirm whether it works or not.

FWIW, this functionality is covered by tests. But happy to wait for a manual test, too.

Issue tags:-Quick fix

#13: drupal.locale-import.13.patch queued for re-testing.

Issue tags:+Quick fix

#13: drupal.locale-import.13.patch queued for re-testing.

subscribe

#13: drupal.locale-import.13.patch queued for re-testing.

Status:Needs review» Needs work

Suppose we

+++ modules/locale/locale.module 21 Nov 2010 23:44:18 -0000
@@ -773,7 +773,24 @@ function locale_language_list($field = '
+ * @todo This is technically wrong. We must not import upon enabling, but upon
+ *   initial installation.

This should be removed because 'install' is related to module and 'enable' for themes.

All other seems good to go.

We have enough tests for module\theme enable but not sure about translation.

Powered by Dreditor.

Status:Needs work» Needs review

This should be removed because 'install' is related to module and 'enable' for themes.

You describe the current state. The @todo is intentional and highlights that the theme system is missing an 'install' operation. String translations are not supposed to be re-imported when a theme is re-enabled. That's utterly wrong, and thus, I'd like to keep this comment.

Created #1067408: Themes need an installation status for D8.

Status:Needs review» Reviewed & tested by the community

Actually, automatic strings re-import is not a good thing but we have no install for themes :(

Status:Reviewed & tested by the community» Fixed

Committed to CVS HEAD. Thanks.

Status:Fixed» Needs work

I don't think this patch was committed correctly. Maybe the theme.inc part of it was missing?

Status:Needs work» Reviewed & tested by the community
StatusFileSize
new670 bytes
PASSED: [[SimpleTest]]: [MySQL] 31,592 pass(es).
[ View ]

Yes, the theme.inc part didn't get committed, because the patch in #13 wouldn't even apply:

$ curl http://drupal.org/files/issues/drupal.locale-import.13.patch | patch -p0
patching file includes/theme.inc
Hunk #1 FAILED at 1256.
1 out of 1 hunk FAILED -- saving rejects to file includes/theme.inc.rej
[...]

I've re-rolled the patch to just that file and attached it.

Aren't we missing some test coverage, then?

@plach: Don't think so. Without the follow-up in #25, HEAD merely attempts to import string translations twice.

Priority:Normal» Major

import string translations twice.

For themes only.

This small WTF should be fixed ASAP

#25: 545518-follow-up-fix.patch queued for re-testing.

Version:7.x-dev» 8.x-dev

patch -p0 < 545518-follow-up-fix.patch
patching file includes/theme.inc
Hunk #1 succeeded at 1255 (offset -1 lines).

Version:8.x-dev» 7.x-dev

Good catch. Committed to 8.x. Moving back to 7.x.

Version:7.x-dev» 8.x-dev

Doesn't look like this was committed to Drupal 8 - the patch still applies.

Issue tags:-Quick fix

#25: 545518-follow-up-fix.patch queued for re-testing.

Status:Reviewed & tested by the community» Needs work
Issue tags:+Quick fix

The last submitted patch, 545518-follow-up-fix.patch, failed testing.

Version:8.x-dev» 7.x-dev

What's happening here? D8 contains the change already.

Status:Needs work» Needs review

#25: 545518-follow-up-fix.patch queued for re-testing.

What's happening here? D8 contains the change already.

You're right, the commit in 8.x was at http://drupalcode.org/project/drupal.git/commitdiff/fc5f56e9c

Status:Needs review» Reviewed & tested by the community

Strange, I'm positive it wasn't there before. Maybe it was committed on April 4 but not actually pushed until later?

Anyway, #25 is certainly RTBC for Drupal 7 also, as long as the tests still pass.

Status:Reviewed & tested by the community» Fixed

Committed to 7.x.

Status:Fixed» Closed (fixed)
Issue tags:-Quick fix

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