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

  // 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);
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Xano’s picture

Issue tags: +Quick fix
FileSize
1.76 KB
Xano’s picture

Assigned: Unassigned » Xano
Status: Active » Needs review

Status: Needs review » Needs work

The last submitted patch failed testing.

sun’s picture

Title: Add locale_modules_enabled() » Add locale_modules_installed() and locale_themes_enabled()
Assigned: Xano » sun
Status: Needs work » Needs review
FileSize
2.71 KB

Good catch!

sun’s picture

Any feedback?

sun’s picture

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

sun’s picture

Title: Add locale_modules_installed() and locale_themes_enabled() » Move Locale module specific code out of module.inc and system.module
sun’s picture

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

sun’s picture

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).

sun’s picture

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

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

sun’s picture

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.

sun’s picture

Status: Needs work » Needs review
FileSize
2.63 KB

Re-rolled against HEAD.

Xano’s picture

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.

sun’s picture

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

sun’s picture

Issue tags: -Quick fix

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

sun’s picture

Issue tags: +Quick fix

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

andypost’s picture

subscribe

sun’s picture

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

andypost’s picture

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.

sun’s picture

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 do not have an installation status for D8.

andypost’s picture

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 :(

Dries’s picture

Status: Reviewed & tested by the community » Fixed

Committed to CVS HEAD. Thanks.

David_Rothstein’s picture

Status: Fixed » Needs work

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

bfroehle’s picture

Status: Needs work » Reviewed & tested by the community
FileSize
670 bytes

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.

plach’s picture

Aren't we missing some test coverage, then?

sun’s picture

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

andypost’s picture

Priority: Normal » Major

import string translations twice.

For themes only.

This small WTF should be fixed ASAP

sun’s picture

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

andypost’s picture

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).
Dries’s picture

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

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

David_Rothstein’s picture

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

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

sun’s picture

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.

sun’s picture

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

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

sun’s picture

Status: Needs work » Needs review

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

bfroehle’s picture

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

David_Rothstein’s picture

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.

Dries’s picture

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.