This is a followup to #1280530: Decouple domain/path negotiation setup from language configuration to decouple the storage too and make it possible for languages to detach from their direct connection to negotiation to stand on their own for needs explained in #1293304: Break up locale.module, but how?.
Comment | File | Size | Author |
---|---|---|---|
#52 | 1301460-52.patch | 18.97 KB | xjm |
#52 | interdiff-49-52.txt | 2.27 KB | xjm |
#49 | 1301460-49.patch | 19 KB | Gábor Hojtsy |
#47 | locale-dompref-settings-1301460-47.patch | 19 KB | Gábor Hojtsy |
#43 | locale-dompref-settings-1301460-43.patch | 19.81 KB | loganfsmyth |
Comments
Comment #1
loganfsmyth CreditAttribution: loganfsmyth commentedMaking good progress. Don't have time to finish it right now, but I can probably get a patch up tonight some time.
Comment #2
Gábor HojtsyLooking forward to that! Thanks!
Comment #3
Gábor HojtsyEspecially now that #1280530: Decouple domain/path negotiation setup from language configuration got committed! Woot!
Comment #4
loganfsmyth CreditAttribution: loganfsmyth commentedUploading for testing. Still definitely fails some tests.
Comment #5
Gábor HojtsyOn a quick review, it seems like a good start! IMHO it would be great to add APIs to retrieve the domains like you did for paths and use them consistently(?). Any reasons against that?
Comment #7
Gábor HojtsyFYI this is becoming increasingly important to fix for #1301040: Move language listing functionality from locale.module to a new language.module since most other dependent patches for that now landed (but not all of them yet :). Thanks again for working hard on the language issues, very much appreciated!
Comment #9
Gábor HojtsyClean up after spammer.
Comment #10
loganfsmyth CreditAttribution: loganfsmyth commented#4: locale-dompref-settings-1301460-4.patch queued for re-testing.
Comment #12
loganfsmyth CreditAttribution: loganfsmyth commentedI still don't really know why that last patch failed the first time, since the test failure was vague, and it passed on my local machine.
Here's a new version that addresses your thoughts on API functions for domains and prefixes Gabor, I also added functions for saving, since I figured that might make life easier when this is hopefully updated with some kind of config management solution.
I also removed the logic that would manually update all prefixes when you submitted the language overview form. Instead I added logic to the prefix loading code to process empty prefixes for non-default languages. Hopefully that will account for the change.
Comment #13
Gábor HojtsyHm, can we implement the path fill-in functionality that you have in locale_language_negotiation_url_prefixes() in hook_locale_language_insert() and hook_locale_language_update() implementations instead? Also, removing path prefixes and domains when a language is removed (via hook_locale_language_deleted()). I think it would be best to have that data cleanup when the changes happen not when we request the data every time. Otherwise looks good.
Comment #14
Gábor HojtsyAlso, locale_update_*() should use the 8000 range, and now that we have 8000, it should be 8001 probably.
Comment #15
Gábor Hojtsy@loganfsmyth: do you have time to work on this in the near future? Thanks.
Comment #16
good_man CreditAttribution: good_man commentedPatch rerolled for the new Drupal core directory.
Comment #18
good_man CreditAttribution: good_man commentedSeems two lines were missing.
Comment #19
Gábor HojtsyThanks for the reroll. Can you work onmovin the data cleanup to the save operation instead of the get op like discussed above, as well as other notes above? Thanks!
Comment #20
good_man CreditAttribution: good_man commented@Gabor: I didn't quite get what you mean in #13. and is there any hook_locale_language_deleted() ?
Comment #21
loganfsmyth CreditAttribution: loganfsmyth commentedHey Gabor, the last few weeks have been rather insane at work, sorry I kind of dropped off the map. Here's an updated patch that uses the language hooks. I didn't realized they existed.
One thing about moving all of that logic to language hooks, that means that we need to make it very clear that to set the default language, locale_language_save needs to be used. If variable_set($language) is used directly, then the default change won't trigger the prefix updates.
Comment #22
loganfsmyth CreditAttribution: loganfsmyth commentedComment #24
loganfsmyth CreditAttribution: loganfsmyth commentedOops. Guess that test didn't quite get converted properly.
Comment #25
good_man CreditAttribution: good_man commentedWhy not just testing the $domains array? instead of testing with $prefixes && !prefixes.
Comment #26
Gábor Hojtsy@good_man: The existing code does that already, this patch just reformulates it for the new slightly changed data structure.
@loganfsmyth: I've reviewed this updated patch and I think it uses the hooks beautifully to modify the data when needed, it is exactly why we introduced these in Drupal 8. I think this looks great. Any other concerns from people?
Comment #27
Gábor HojtsyOk, took considerable time to review this in detail. I think this looks generally great, just three (minor) comments:
1. Let's remove prefix and domain from language_default() too. I've seen you did it in the tests.
2.
Why not use !empty() like in the original code and $prefixes[$options['language']->language]. The kind of array to scalar value copy that you do in the if() we usually don't do AFAIS.
3.
Let's store language_default() in a local variable and not call it three times IMHO.
I think this is all, otherwise looks good and ready to go to me :)
Comment #28
loganfsmyth CreditAttribution: loganfsmyth commented1. Weird. I would have sworn that I did that.
2. Good call. Changed it.
3. I've simplified that install logic more to this, so it will use the standard insert hook to set up the prefix.
Other changes:
1. I removed the unnecessary array_intersect_key calls in the url provider submit handler.
2. Added variable deletion in hook_uninstall().
Comment #29
loganfsmyth CreditAttribution: loganfsmyth commentedComment #30
Gábor HojtsyGreat improvements. The default property setting in fact makes me think we should actually return from language_default() with that property set to TRUE, right? That would allow us to use the $language object from there the same way we use it when returned from language_list() where we do the same. (http://api.drupal.org/api/drupal/core--includes--bootstrap.inc/function/...). I'd imagine we add $language->default = TRUE; after the variable_get() in language_default() to ensure we always add that property. That would be a more general improvement and not necessitate any change to the locale install routine. Otherwise it all looks good.
Unfortunately the eventual commit of this is dependent on #1336170: Add locale module to upgrade tests as per @catch, so trying to mobilize people to work on that too.
Comment #31
loganfsmyth CreditAttribution: loganfsmyth commentedI've moved that ->default setting to language_default(). I also expanded out the language object definition in drupal_web_test_case so it is easier to read and change the patch's the update hook to 8002.
I see that the new locale_update_8001 does a query directly on the languages table instead of calling language_list. Any particular reason for that? If so should we be doing that in this patch as well?
Not directly related to this, but observations:
Plurals, formula and javascript where removed from the language object, but all those keys are still set in language_default and drupal_web_test_case. Also drupal_web_test_case still references 'native'.
Not sure if it's worth making a separate issue for those, or if I can just wrap them into this patch.
Comment #32
Gábor HojtsyChanges look great. I think solving those leftover properties here is fine, since we are touching the same code anyway.
Comment #33
loganfsmyth CreditAttribution: loganfsmyth commentedNew patch including removal of 'plurals', 'formula' and 'javascript' keys from the default language, and those keys and 'native' being removed from default language in drupal_web_test_case.
Comment #34
Gábor HojtsyOk then I think this is as good as we can get here and once #1336170: Add locale module to upgrade tests lands, we should get this in. Therefore not markup RTBC although I think @catch would not commit it anyway before that, since its one of his recommendations anyway.
Comment #35
Gábor Hojtsy#1336170: Add locale module to upgrade tests is nearing commit (hopefully). It will in itself not cover negotiation settings, so this patch will need to be updated to add that too to the update dump for testing. Once that is committed.
Comment #36
Gábor Hojtsy#1336170: Add locale module to upgrade tests landed, so let's extend the update tests with negotiation config details and then we can get this in! Yay!
Comment #37
loganfsmyth CreditAttribution: loganfsmyth commented@Gábor
Is there documentation anywhere on creating upgrade path tests?
I'll want to set negotiation variable to enable url negotiation, and make sure the languages in the db dump have prefix values. Do I need some kind of test to make sure they are converted properly by the update hooks? Where do I start? Thanks!
Comment #38
Gábor Hojtsy@loganfsmyth: as you can see in #1336170: Add locale module to upgrade tests and in the tests themselves (core/modules/simpletest/tests/upgrade), you'll see it merely tests whether all the updates are ran without errors, not that it results in an expected working situation. Because we don't generally seem to have a practice to run functional tests on top of updated DBs I think the extension of the DB dump with the negotiation settings modified here would be fine. The current dump has Catalan and English. Maybe add one more language and specify prefixes for each IMHO.
Comment #39
Gábor HojtsyTalked to @catch about this, and he said this looks like a fairly simple update, and #1331370: [Upgrade path broken] Stored include paths need to be updated to /core before running the upgrade path is about to add the testing required anyway (and is marked critical), so we should proceed with this as-is (not adding/extending tests here). However, @catch pointed out we should not use language_list() in the update, since its an API function. Just use the DB directly. Thanks!
Comment #40
loganfsmyth CreditAttribution: loganfsmyth commentedI've fixed the update hook to directly query the languages table instead of calling language_list.
So we'll just leave the new locale tests to be done in that separate issue?
Comment #41
Gábor HojtsyRight, looks good to me now, let's get this in finally :)
Comment #42
Dries CreditAttribution: Dries commented* Should be 'Look up' instead of 'Look of'?
* Maybe we should add some @TODO to revisit some functions/code after the configuration management initiative lands?
Comment #43
loganfsmyth CreditAttribution: loganfsmyth commented* Fixed that typo.
* Added @TODOs to the domain and prefix settings functions.
Comment #44
Dries CreditAttribution: Dries commentedI'm comfortable with this patch now, assuming it turns green. Thanks for the quick turn-around.
Comment #45
Gábor HojtsyGreat, thanks.
Comment #46
catchHmm I'm not sure we need those @todos (and they should be lower case anyway).
Once CMI lands we need to review every use of variable_get() and variable_set() in core for either CMI storage or #1175054: Add a storage (API) for persistent non-configuration state, so the presence of variable_get() in a function means it will have to be looked at before release anyway.
edit: actually neutral on the @todos since it might be possible to factor out those helper functions altogether and just use the config API directly for that, but still should be lower case for code style.
Comment #47
Gábor HojtsyI agree we are going to revisit all variable_get(), variable_set() anyway for CMI, and adding @todos all around there looks like extraneous. Here is the above patch with the @TODOs backed out (the typofix still in).
Comment #49
Gábor HojtsyJust renaming locale_update_8002() to locale_update_8003() since another locale_update_8002() has landed. Otherwise the same patch and should fix all outstanding concerns.
Comment #50
Gábor HojtsyBack to RTBC then as per above.
Comment #51
xjmSeveral of the one-line summaries for functions added by this patch don't match our documented standards. They should begin with a third-person present-tense verb. We are in the process of cleaning this up throughout core in #1310084: [meta] API documentation cleanup sprint
I'll add a reroll that fixes this.
Comment #52
xjmAttached just changes five verb forms and removes an extra blank line.
Comment #53
catchThis all looks good, committed/pushed to 8.x. Thanks!
Comment #55
Gábor HojtsyTagging for base language system.
Comment #56
Gábor HojtsyTagging for language negotiation too.