With base language functionality (language list, negotiation) moved to language module, language assignment moved to the respective modules (path, node, user, comment, etc), only the following things remained in locale module: (1) interface translation (2) field language support (3) date format translation support. The last two should move away later, but there are already likely many tests that reference locale for the functionality that is already removed instead of language module. The test should reference language module when they only need language module.

This is similar to #1552236: Move user language tests to user module and #1546752: Move negotiation tests to language module where we did the same for negotiation tests and user tests (and also moved them). Except that in this task, there is likely nothing to move, but existing tests need slight updates to depend on the new base module providing the functionality.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

DickJohnson’s picture

Assigned: Unassigned » DickJohnson
Gábor Hojtsy’s picture

Any progress on this one? It has been 10 days.

marcingy’s picture

Status: Active » Needs review
FileSize
5.84 KB

First pass - think this takes care of all locations

marcingy’s picture

FileSize
5.84 KB

First pass - think this takes care of all locations

Gábor Hojtsy’s picture

Status: Needs review » Needs work

So you replaced locale with translation which is not really the goal of this issue. The dependency graph is that 'language' module is the base language support on which 'locale' depends for UI translation and 'translation' depends for content translation. Replacing locale with translation could prove that the UI translation functionality was not needed for those tests at places, however, translation.info still marks locale module as a requirement. That is outdated. Translation.info should not say locale is a requirement anymore, only language module should be.

Then if your other changes still pass, it proves that relying on translation module instead of locale module will mean that you should really only rely on language module (their common base requirement), which is in fact the goal of this issue.

Thanks for working on this!

marcingy’s picture

Assigned: DickJohnson » Unassigned
Status: Needs work » Needs review
FileSize
6.24 KB

Ok this time removed the dependency of transalation on locale and swapped out to language where possible,.

Status: Needs review » Needs work

The last submitted patch, test-dependencies.patch, failed testing.

marcingy’s picture

Status: Needs work » Needs review
FileSize
7.49 KB

Fixes up dependency test - node load isn't failing locally so not sure if it was a bot glitch.

Gábor Hojtsy’s picture

Looks good on a quick review. Will need to set aside some time to review in more detail. Other reviews still welcome in the meantime! :)

Gábor Hojtsy’s picture

Status: Needs review » Needs work

Note that now tests are being moved around and componentized per class. See #1591950: Convert locale tests to PSR-0 for example. That means this will need a reroll, and we need to figure out where all the code ended up that was modified. The tests themselves are not modified, but their place is dramatically different. Think:

 delete mode 100644 core/modules/language/language.test
 create mode 100644 core/modules/language/lib/Drupal/language/Tests/LanguageBrowserDetectionUnitTest.php
 create mode 100644 core/modules/language/lib/Drupal/language/Tests/LanguageConfigurationTest.php
 create mode 100644 core/modules/language/lib/Drupal/language/Tests/LanguageDependencyInjectionTest.php
 create mode 100644 core/modules/language/lib/Drupal/language/Tests/LanguageListTest.php
 create mode 100644 core/modules/language/lib/Drupal/language/Tests/LanguageNegotiationInfoTest.php
 create mode 100644 core/modules/language/lib/Drupal/language/Tests/LanguagePathMonolingualTest.php
 create mode 100644 core/modules/language/lib/Drupal/language/Tests/LanguageSwitchingTest.php
 create mode 100644 core/modules/language/lib/Drupal/language/Tests/LanguageUILanguageNegotiationTest.php
 create mode 100644 core/modules/language/lib/Drupal/language/Tests/LanguageUrlRewritingTest.php

(etc).

I think it would make sense to wait with rerolling until all affected tests are moved. If you can verify all tests are moved, then it would make sense to reroll and here's hoping there will be no major test changes in the meantime, so we can get it in before it needs another big reroll. Thanks!

ZenDoodles’s picture

Status: Needs work » Needs review
Issue tags: -D8MI, -sprint, -language-base

#8: test-dependencies.patch queued for re-testing.

Status: Needs review » Needs work
Issue tags: +D8MI, +sprint, +language-base

The last submitted patch, test-dependencies.patch, failed testing.

marcingy’s picture

Status: Needs work » Needs review
FileSize
7.8 KB

Re-roll to track head changes.

Gábor Hojtsy’s picture

Status: Needs review » Needs work

core/modules/search/search.test is no more.

marcingy’s picture

Status: Needs work » Needs review
FileSize
7.93 KB

Re-roll tracking head.

yurtboy’s picture

Status: Needs review » Reviewed & tested by the community

Applied Cleanly

Checking patch core/modules/entity/tests/entity.test...
Checking patch core/modules/field/modules/text/text.test...
Checking patch core/modules/language/lib/Drupal/language/Tests/LanguageUILanguageNegotiationTest.php...
Checking patch core/modules/path/path.test...
Checking patch core/modules/search/lib/Drupal/search/Tests/SearchLanguageTest.php...
Checking patch core/modules/system/system.test...
Checking patch core/modules/system/tests/common.test...
Checking patch core/modules/translation/lib/Drupal/translation/Tests/TranslationTest.php...
Checking patch core/modules/translation/translation.info...
Applied patch core/modules/entity/tests/entity.test cleanly.
Applied patch core/modules/field/modules/text/text.test cleanly.
Applied patch core/modules/language/lib/Drupal/language/Tests/LanguageUILanguageNegotiationTest.php cleanly.
Applied patch core/modules/path/path.test cleanly.
Applied patch core/modules/search/lib/Drupal/search/Tests/SearchLanguageTest.php cleanly.
Applied patch core/modules/system/system.test cleanly.
Applied patch core/modules/system/tests/common.test cleanly.
Applied patch core/modules/translation/lib/Drupal/translation/Tests/TranslationTest.php cleanly.
Applied patch core/modules/translation/translation.info cleanly

Is there some other tests I can run? Interface related or?

yurtboy’s picture

Status: Reviewed & tested by the community » Needs review

changing status back to "needs review" sorry about that.

Gábor Hojtsy’s picture

Issue tags: -D8MI, -sprint, -language-base

#15: decouple-v2.patch queued for re-testing.

Status: Needs review » Needs work
Issue tags: +D8MI, +sprint, +language-base

The last submitted patch, decouple-v2.patch, failed testing.

Gábor Hojtsy’s picture

The patch does not apply anymore (again) unfortunately.

aries’s picture

Assigned: Unassigned » aries
aries’s picture

Status: Needs work » Needs review
FileSize
6.92 KB

Patch updated.

Gábor Hojtsy’s picture

Status: Needs review » Needs work

I stumbled on a date formats related change but that seems to be good after all (see below). So only the first comment is to be fixed as per my line by line review. Looks good otherwise. Thanks all for working on this, let's get this through :)

+++ b/core/modules/system/system.testundefined
@@ -442,29 +442,27 @@ class ModuleDependencyTestCase extends ModuleTestCase {
     // Assert that the locale tables weren't enabled.
     $this->assertTableCount('language', FALSE);
-    $this->assertTableCount('locale', FALSE);
...
     // Assert that the locale tables were enabled.
     $this->assertTableCount('language', TRUE);
-    $this->assertTableCount('locale', TRUE);

The comment is not good anymore, its testing the language table, which is not managed with locale anymore. Comment needs update too.

+++ b/core/modules/system/system.testundefined
@@ -442,29 +442,27 @@ class ModuleDependencyTestCase extends ModuleTestCase {
@@ -1224,7 +1222,7 @@ class DateTimeFunctionalTest extends WebTestBase {

@@ -1224,7 +1222,7 @@ class DateTimeFunctionalTest extends WebTestBase {
   }
 
   function setUp() {
-    parent::setUp(array('locale'));
+    parent::setUp(array('language'));

I looked this up closer, since the date format save API is theoretically in locale module (http://api.drupal.org/api/drupal/core%21modules%21locale%21locale.module...), however, that seems to be only for the locale UI to this functionality, while system module itself has API to save these formats at http://api.drupal.org/api/drupal/modules%21system%21system.module/functi.... Confusing. The tests do not test using the UI, so relying on language.module would indeed be sufficient here.

aries’s picture

Status: Needs work » Needs review
FileSize
7.03 KB

If I understood well, only the comment needed a fix. Here is it.

Gábor Hojtsy’s picture

Status: Needs review » Reviewed & tested by the community

Right. Looks good now. I thoroughly reviewed this, and it looks good to go.

aspilicious’s picture

Issue tags: -D8MI, -sprint, -language-base

Status: Reviewed & tested by the community » Needs work
Issue tags: +D8MI, +sprint, +language-base

The last submitted patch, Locale2LanguageTest-1561004-23.patch, failed testing.

yurtboy’s picture

Status: Needs work » Needs review
FileSize
7.15 KB

Patch updated and applies cleanly at 2922369

Status: Needs review » Needs work
Issue tags: -D8MI, -sprint, -language-base

The last submitted patch, Locale2LanguageTest-1561004-28.patch, failed testing.

yurtboy’s picture

Status: Needs work » Needs review
Issue tags: +D8MI, +sprint, +language-base
Gábor Hojtsy’s picture

Status: Needs review » Reviewed & tested by the community

Thanks!

yurtboy’s picture

drupalladder!

aspilicious’s picture

Issue tags: -D8MI, -sprint, -language-base

Status: Reviewed & tested by the community » Needs work
Issue tags: +D8MI, +sprint, +language-base

The last submitted patch, Locale2LanguageTest-1561004-28.patch, failed testing.

aspilicious’s picture

Srry common tests are psr-0 now. :(
System tests will follow soon so maybe we should warn catch

Gábor Hojtsy’s picture

Issue tags: +Avoid commit conflicts

Tagged for commit conflicts. Can someone help with a quick reroll? :) Thanks.

aspilicious’s picture

Assigned: aries » aspilicious

I'll do it

Tor Arne Thune’s picture

Re-rolled.

Tor Arne Thune’s picture

Oops, sorry about that. Cross-posted.

Gábor Hojtsy’s picture

Status: Needs work » Needs review
Gábor Hojtsy’s picture

Status: Needs review » Reviewed & tested by the community

Thanks, still looks good!

catch’s picture

Status: Reviewed & tested by the community » Fixed

Thanks! committed/pushed to 8.x.

aspilicious’s picture

Status: Fixed » Reviewed & tested by the community

I did a local diff, looks good indeed

catch’s picture

Status: Reviewed & tested by the community » Fixed
Gábor Hojtsy’s picture

Issue tags: -sprint

Thanks all!

Status: Fixed » Closed (fixed)

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

chx’s picture

Issue tags: -Avoid commit conflicts
chx’s picture

try #2