Support from Acquia helps fund testing for Drupal Acquia logo

Comments

loganfsmyth’s picture

Assigned: Unassigned » loganfsmyth

Making good progress. Don't have time to finish it right now, but I can probably get a patch up tonight some time.

Gábor Hojtsy’s picture

Looking forward to that! Thanks!

Gábor Hojtsy’s picture

loganfsmyth’s picture

Status: Active » Needs review
FileSize
16.04 KB

Uploading for testing. Still definitely fails some tests.

Gábor Hojtsy’s picture

On 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?

Status: Needs review » Needs work

The last submitted patch, locale-dompref-settings-1301460-4.patch, failed testing.

Gábor Hojtsy’s picture

FYI 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!

Gábor Hojtsy’s picture

Clean up after spammer.

loganfsmyth’s picture

Status: Needs work » Needs review
Issue tags: -D8MI

Status: Needs review » Needs work
Issue tags: +D8MI

The last submitted patch, locale-dompref-settings-1301460-4.patch, failed testing.

loganfsmyth’s picture

Status: Needs work » Needs review
FileSize
16.24 KB

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

Gábor Hojtsy’s picture

Status: Needs review » Needs work

Hm, 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.

Gábor Hojtsy’s picture

Also, locale_update_*() should use the 8000 range, and now that we have 8000, it should be 8001 probably.

Gábor Hojtsy’s picture

@loganfsmyth: do you have time to work on this in the near future? Thanks.

good_man’s picture

Status: Needs work » Needs review
FileSize
16.13 KB

Patch rerolled for the new Drupal core directory.

Status: Needs review » Needs work

The last submitted patch, locale-dompref-settings-1301460-12-rerolled.patch, failed testing.

good_man’s picture

Status: Needs work » Needs review
FileSize
16.25 KB

Seems two lines were missing.

Gábor Hojtsy’s picture

Thanks 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!

good_man’s picture

Assigned: loganfsmyth » Unassigned
Status: Needs review » Needs work

@Gabor: I didn't quite get what you mean in #13. and is there any hook_locale_language_deleted() ?

loganfsmyth’s picture

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

loganfsmyth’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, locale-dompref-settings-1301460-21.patch, failed testing.

loganfsmyth’s picture

Status: Needs work » Needs review
FileSize
19.03 KB

Oops. Guess that test didn't quite get converted properly.

good_man’s picture

Status: Needs review » Needs work
+++ b/core/includes/locale.inc
@@ -337,7 +338,9 @@ function locale_language_url_fallback($language = NULL, $language_type = LANGUAG
+  if (($prefix && empty($prefixes[$default->language])) || (!$prefix && empty($domains[$default->language]))) {

Why not just testing the $domains array? instead of testing with $prefixes && !prefixes.

Gábor Hojtsy’s picture

Status: Needs work » Needs review

@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?

Gábor Hojtsy’s picture

Status: Needs review » Needs work

Ok, 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.

+++ b/core/includes/locale.incundefined
@@ -428,22 +431,52 @@ function locale_language_url_rewrite_url(&$path, &$options) {
+        if (isset($prefixes[$options['language']->language]) && ($prefix = $prefixes[$options['language']->language])) {
+          $options['prefix'] = $prefix . '/';

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.

+++ b/core/modules/locale/locale.installundefined
@@ -12,6 +12,16 @@ function locale_install() {
   locale_language_save(language_default());
+
+  // Initialize the domain and prefix settings.
+  $prefixes = array(
+    language_default()->language => '',
+  );
+  locale_language_negotiation_url_prefixes_save($prefixes);
+  $domains = array(
+    language_default()->language => '',
+  );

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

loganfsmyth’s picture

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

$default = languge_default();
$default->default = TRUE;
locale_language_save($default);

Other changes:
1. I removed the unnecessary array_intersect_key calls in the url provider submit handler.
2. Added variable deletion in hook_uninstall().

loganfsmyth’s picture

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

Status: Needs review » Needs work

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

loganfsmyth’s picture

Status: Needs work » Needs review
FileSize
19.47 KB

I'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.

Gábor Hojtsy’s picture

Changes look great. I think solving those leftover properties here is fine, since we are touching the same code anyway.

loganfsmyth’s picture

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

Gábor Hojtsy’s picture

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

Gábor Hojtsy’s picture

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

Gábor Hojtsy’s picture

Status: Needs review » Needs work

#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!

loganfsmyth’s picture

@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!

Gábor Hojtsy’s picture

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

Gábor Hojtsy’s picture

Talked 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!

loganfsmyth’s picture

Status: Needs work » Needs review
FileSize
19.52 KB

I'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?

Gábor Hojtsy’s picture

Status: Needs review » Reviewed & tested by the community

Right, looks good to me now, let's get this in finally :)

Dries’s picture

Status: Reviewed & tested by the community » Needs work
+++ b/core/modules/locale/locale.installundefined
@@ -258,6 +246,37 @@ function locale_update_8001() {
+  // Look of languages directly instead of using language_list().

* 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?

loganfsmyth’s picture

Status: Needs work » Needs review
FileSize
19.81 KB

* Fixed that typo.
* Added @TODOs to the domain and prefix settings functions.

Dries’s picture

I'm comfortable with this patch now, assuming it turns green. Thanks for the quick turn-around.

Gábor Hojtsy’s picture

Status: Needs review » Reviewed & tested by the community

Great, thanks.

catch’s picture

Status: Reviewed & tested by the community » Needs work

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

Gábor Hojtsy’s picture

Status: Needs work » Needs review
FileSize
19 KB

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

Status: Needs review » Needs work

The last submitted patch, locale-dompref-settings-1301460-47.patch, failed testing.

Gábor Hojtsy’s picture

Status: Needs work » Needs review
FileSize
19 KB

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

Gábor Hojtsy’s picture

Status: Needs review » Reviewed & tested by the community

Back to RTBC then as per above.

xjm’s picture

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

xjm’s picture

FileSize
2.27 KB
18.97 KB

Attached just changes five verb forms and removes an extra blank line.

catch’s picture

Status: Reviewed & tested by the community » Fixed

This all looks good, committed/pushed to 8.x. Thanks!

Status: Fixed » Closed (fixed)

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

Gábor Hojtsy’s picture

Issue tags: +language-base

Tagging for base language system.

Gábor Hojtsy’s picture

Issue tags: +negotiation

Tagging for language negotiation too.