Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
(Based on discussions held Multilingual Sprint in Montreal)
Comment | File | Size | Author |
---|---|---|---|
#18 | locale-domain-prefix-ui-1280530-18.patch | 20.98 KB | loganfsmyth |
#16 | domainprefixui.png | 27.9 KB | loganfsmyth |
#16 | locale-domain-prefix-ui-1280530-16.patch | 20.99 KB | loganfsmyth |
#10 | locale-domain-prefix-ui-1280530-10.patch | 19.78 KB | loganfsmyth |
#7 | DomPref-OldLangconf.png | 42.81 KB | loganfsmyth |
Comments
Comment #1
Bojhan CreditAttribution: Bojhan commentedCan we merge these?
Comment #2
Gábor HojtsyNo. The issue is very sparse, but it refers to the language edit screen (path prefix, domain) vs. the config screen where you select which one Drupal will actually use. This is the 3. and 4. screenshots in #1279840: META: smarter defaults and UI fixes for language selection.
Comment #3
loganfsmyth CreditAttribution: loganfsmyth commentedThe consensus on this rather than crosslinking, it would be even nicer to simply move prefix and domain settings into the negotiation provider configuration page, since that is really the place where you would want to configure that type of thing.
Comment #4
Gábor Hojtsy@Bojhan: sorry, let me correct myself. The answer is yes :) HAHA. So to illuminate the consensus that @logansmyth mentioned, we talked about this a lot in Montreal with @webchick and @plach, and we all agreed at the end that the prefix and domain settings are only ever used for negotiation/selection/urls, and if you don't have the URL option, those will never be used, so it makes total sense to have it on the URL configuration form. So yes, you are absolutely right, we should merge those. You are perfectly right :)
Comment #5
loganfsmyth CreditAttribution: loganfsmyth commentedHere is an initial patch. It works as far as I can tell, but it breaks tons of locale tests because they pretty much all rely on using the language-add page to set a prefix. I still need to go through and fix all of the tests, and add new ones for the new settings locations.
Comment #6
Gábor HojtsyOn a very quick review, it looks conceptually sound. Can you provide screenshots of how this looks like? That would be great for the Usability review :)
Comment #7
loganfsmyth CreditAttribution: loganfsmyth commentedHere are some screenshots and an updated patch that fixes all of the broken tests.
Original language configuration pages require you to enter prefix or domain.
New language config page only requires basic language info.
The old url negotiation page had you enable domain or prefix negotiation, but you had to go to the language page to actually change the values.
This moves domain and prefix configuration to the page where they actually matter.
Comment #8
loganfsmyth CreditAttribution: loganfsmyth commentedComment #10
loganfsmyth CreditAttribution: loganfsmyth commentedOops, missed a test outside of Locale.
Comment #11
Gábor HojtsyWow, the UI looks slick. I'll have a thorough code review later, will try to get some UX people attention here :)
Comment #12
Gábor HojtsyLinked in from #1293304: Break up locale.module, but how?, where we explore how negotiation fits in with other language components, and when people need to enable it. The flows explored there validate that language configuration does not necessarily mean you need language negotiation too, which this patch very well enables :)
Small comment on the UI: I think it would be valuable to show the language code as a hint in either the label or description to the field for path prefix/domain, so users can derive their values from that if they want to (especially for domain).
Comment #13
Bojhan CreditAttribution: Bojhan commentedQuick review:
The description for language name in english, native language name and direction are a bit useless. I think without those people focus more on the label which are pretty descriptive already. I am concerend using the word native (because thats for non native speakers, sometimes difficult).
The whole path prefix configuration still confuses me, first of all I dont know what domain or path prefix options are. Secondly the path prefix configuration help is not really understandable. If people are not familiar with the concept these prefixes dont really make much sense. Also, why is the textfield so long? And not appended with mysite.com/
Comment #14
Gábor Hojtsy@Bojhan: thanks for the review. Notes on your points:
- On the language list having useless stuff with English name vs. Native name, this is really just a pretty old hack to be able to display the language names in their native names in language switcher blocks. Once the config management initiative gets here, we'll need to figure out the UI for language name config translation too, and we'll loose both the English name and Native name field, and would have a Name field instead which would be in the language being configured. Ie. if you configure your site in English, it will be English, its Dutch, then you'll provide the language name in Dutch. Its the same like people do for block titles (in their native language), view names, etc. The language list has some usability brokenness remnants that we'll hopefully be able to work out later, not yet.
- On path prefix and domain options, for path prefixes, you should make up whatever you want. Let's say you have English and German on the website. You can make the path prefixes respectively: (a) empty and 'de'; 'en' and 'de'; 'english' and 'deutsch'; '0' and '1', whatever fits your goals/design. For domains, you can make up domains for yourself, it can be en.example.com, de.example.com or example.com and example.de, etc. However, you need to set up those domains first outside of Drupal of course, so Drupal knows to serve these domains. (We can make usability improvements later to check whether the domain is going to work like the clean URL check, to help users not to foobar their site with invalid domains picked, but let's not try to solve all problems in one issue).
- For path prefixes, I agree we could make the field prefixed with the site base path/URL to make the resulting URL more evident. For domains, we ask for the whole domain, so it might or might not be a subdomain of your main domain. For path prefixes, the URL prefix form hint like in the 403/404 page setup or front page setup would work good, agreed.
Does this help understand the concepts behind these screens? Any updated feedback?
Comment #15
Gábor HojtsyOk, did a code review! All-in-all a great patch! I'd add that we'll want to eventually move out the domain/prefix values from the language table, since they belong to negotiation only and also make the negotiation settings from bastardized variables to some real configuration storage, but the scope of this patch is big enough that we should not strive to do that here yet. I think this improves usability a great deal, especially with the suggested changes from Bojhan, so is definitely a good direction IMHO. Detailed code comments:
Include "Use with caution in a production environment." like in the code above?
As per @Bojhan, let's prefix field with base path like on the 403/404 page setup UI. Also include hint for language code as discussed above?
Should have one line of main comment and supporting detail after a newline as per coding standards.
Let's not do text replacement like !type, we should have two versions of the two error messages each with the type name literally included, and conditionally used based on $type. Doing !types makes it very hard if not impossible for some languages to translate this sentence so it makes sense (especially since you included $type untranslated).
I've not seen this test condition reproduced elsewhere. Can you move it to a place where it makes sense or rewrite it here to make sense? Would be good to test that prefixes stick (get saved), but there does not seem like we have a test for that after your patch.
Comment #16
loganfsmyth CreditAttribution: loganfsmyth commentedUpdated with Gábor and Bojhan's recommendations.
Here is a new screenshot with the langcode showing, and the field prefix.
Comment #17
Gábor HojtsyGreat update, thanks for working on this! My notes:
We don't usually do "excessive" indents like this in form API constructs, let's avoid.
Also, you did not unify the strong warning text here yet.
Minor: dot at end of line.
Minor: dot at end of line.
URL
Dot at end of line. And avoid whitespace inside parenthesis.
Dot at end of line. And avoid whitespace inside parenthesis.
Finally, we will eventually want to move out the prefix and domain settings from the language table. Do you think its feasible to do here, or you think/agree we are already doing lots of stuff here? I imagine we could use two variable's in the meantime (while the config management initiative figures out how to store stuff in great ways), since the negotiation system uses odd variables for all its stuff anyway. This issue coupled with #1301148: Stop pretending we have configuration translation for languages is about to simplify language configuration immensely, and we'll need to write the data migration / update function either here or in a followup. I think this is a pretty big change as-is, but if you feel you have capacity to include simple var storage and data migration updates, that is good too :)
Setting needs work for the cosmetic feedback above.
Comment #18
loganfsmyth CreditAttribution: loganfsmyth commentedUpdated with fixes you mentioned in your patch.
I do think this is a pretty bit change already. That said, I'm happy to give the prefix/domain variable migration a shot, whether it's in this issue or a new one.
Comment #19
Gábor HojtsyRetitle to be more specific. Opened #1301460: Decouple domain/path language negotiation storage from language config storage for the config storage separation / migration task.
Comment #20
Gábor HojtsyAll right, looks good to me know. With #1301460: Decouple domain/path language negotiation storage from language config storage we ensure it gets its data migrated too, which is required to continue with #1301040: Move language listing functionality from locale.module to a new language.module so we can ensure it gets done. Let's not hold back this patch for it, its a superb usability improvement on its own.
Comment #21
Dries CreditAttribution: Dries commentedLooks good. Committed to 8.x. Thanks!
Comment #23
Gábor HojtsyTagging for base language system.
Comment #24
Gábor HojtsyTagging for language negotiation too.