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.
Comment | File | Size | Author |
---|---|---|---|
#38 | Locale2LanguageTest-1561004-37.patch | 7.89 KB | Tor Arne Thune |
#28 | Locale2LanguageTest-1561004-28.patch | 7.15 KB | yurtboy |
#24 | Locale2LanguageTest-1561004-23.patch | 7.03 KB | aries |
#22 | decouple-v3.patch | 6.92 KB | aries |
#15 | decouple-v2.patch | 7.93 KB | marcingy |
Comments
Comment #1
DickJohnson CreditAttribution: DickJohnson commentedComment #2
Gábor HojtsyAny progress on this one? It has been 10 days.
Comment #3
marcingy CreditAttribution: marcingy commentedFirst pass - think this takes care of all locations
Comment #4
marcingy CreditAttribution: marcingy commentedFirst pass - think this takes care of all locations
Comment #5
Gábor HojtsySo 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!
Comment #6
marcingy CreditAttribution: marcingy commentedOk this time removed the dependency of transalation on locale and swapped out to language where possible,.
Comment #8
marcingy CreditAttribution: marcingy commentedFixes up dependency test - node load isn't failing locally so not sure if it was a bot glitch.
Comment #9
Gábor HojtsyLooks good on a quick review. Will need to set aside some time to review in more detail. Other reviews still welcome in the meantime! :)
Comment #10
Gábor HojtsyNote 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:
(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!
Comment #11
ZenDoodles CreditAttribution: ZenDoodles commented#8: test-dependencies.patch queued for re-testing.
Comment #13
marcingy CreditAttribution: marcingy commentedRe-roll to track head changes.
Comment #14
Gábor Hojtsycore/modules/search/search.test is no more.
Comment #15
marcingy CreditAttribution: marcingy commentedRe-roll tracking head.
Comment #16
yurtboy CreditAttribution: yurtboy commentedApplied Cleanly
Is there some other tests I can run? Interface related or?
Comment #17
yurtboy CreditAttribution: yurtboy commentedchanging status back to "needs review" sorry about that.
Comment #18
Gábor Hojtsy#15: decouple-v2.patch queued for re-testing.
Comment #20
Gábor HojtsyThe patch does not apply anymore (again) unfortunately.
Comment #21
aries CreditAttribution: aries commentedComment #22
aries CreditAttribution: aries commentedPatch updated.
Comment #23
Gábor HojtsyI 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 :)
The comment is not good anymore, its testing the language table, which is not managed with locale anymore. Comment needs update too.
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.
Comment #24
aries CreditAttribution: aries commentedIf I understood well, only the comment needed a fix. Here is it.
Comment #25
Gábor HojtsyRight. Looks good now. I thoroughly reviewed this, and it looks good to go.
Comment #26
aspilicious CreditAttribution: aspilicious commented#24: Locale2LanguageTest-1561004-23.patch queued for re-testing.
Comment #28
yurtboy CreditAttribution: yurtboy commentedPatch updated and applies cleanly at 2922369
Comment #30
yurtboy CreditAttribution: yurtboy commented#28: Locale2LanguageTest-1561004-28.patch queued for re-testing.
Comment #31
Gábor HojtsyThanks!
Comment #32
yurtboy CreditAttribution: yurtboy commenteddrupalladder!
Comment #33
aspilicious CreditAttribution: aspilicious commented#28: Locale2LanguageTest-1561004-28.patch queued for re-testing.
Comment #35
aspilicious CreditAttribution: aspilicious commentedSrry common tests are psr-0 now. :(
System tests will follow soon so maybe we should warn catch
Comment #36
Gábor HojtsyTagged for commit conflicts. Can someone help with a quick reroll? :) Thanks.
Comment #37
aspilicious CreditAttribution: aspilicious commentedI'll do it
Comment #38
Tor Arne Thune CreditAttribution: Tor Arne Thune commentedRe-rolled.
Comment #39
Tor Arne Thune CreditAttribution: Tor Arne Thune commentedOops, sorry about that. Cross-posted.
Comment #40
Gábor HojtsyComment #41
Gábor HojtsyThanks, still looks good!
Comment #42
catchThanks! committed/pushed to 8.x.
Comment #43
aspilicious CreditAttribution: aspilicious commentedI did a local diff, looks good indeed
Comment #44
catchComment #45
Gábor HojtsyThanks all!
Comment #47
chx CreditAttribution: chx commentedComment #48
chx CreditAttribution: chx commentedtry #2