Early Bird Registration for DrupalCon Portland 2024 is open! Register by 23:59 UTC on 18 March 2024, to get $100 off your ticket.
Split from #1827038: Remove stale references to language_content_type variable.
The previous issue dealt with converting a variety of language_* variables to CMI but it has got confusing, so I've split this off to focus on converting language_default only.
Changes required in:
core/includes/bootstrap.inc
2571 $info = variable_get('language_default', array(
core/includes/update.inc
326 $language_default = variable_get('language_default');
335 variable_set('language_default', (array) $language_default);
569 $language_default = variable_get('language_default');
581 variable_set('language_default', $language_default);
core/lib/Drupal/Core/Language/LanguageManager.php
181 $default_info = variable_get('language_default', array(
core/modules/language/lib/Drupal/language/Tests/Condition/LanguageConditionTest.php
52 // @todo remove this when language_default() no longer needs variable_get().
core/modules/language/language.module
511 variable_set('language_default', (array) $language);
core/modules/language/lib/Drupal/language/Tests/LanguageDependencyInjectionTest.php
74 variable_set('language_default', $new_language_default);
core/modules/language/lib/Drupal/language/Tests/LanguageUILanguageNegotiationTest.php
111 variable_set('language_default', (array) $languages['vi']);
Comment | File | Size | Author |
---|---|---|---|
#29 | 2108599.29.patch | 48.47 KB | alexpott |
#29 | 27-29-interdiff.txt | 565 bytes | alexpott |
#27 | interdiff.txt | 3.78 KB | Gábor Hojtsy |
#27 | 2108599.27.patch | 47.95 KB | Gábor Hojtsy |
#26 | 2108599.26.patch | 47.74 KB | alexpott |
Comments
Comment #0.0
ianthomas_ukListed instances of language_default
Comment #1
ianthomas_ukOK, I'm unsure where these should go. They are required by shared code, so belong under system.* somewhere.
#1571632: Convert regional settings to configuration system was going to move the other variables on this form to system.regional, but that got switched to system.date, even though one of the options was country (pointed out in #84 but not followed up).
We could revive system.regional, and perhaps move country back there? Or maybe system.locale?
What about the variable itself? I think language on it's own is too vague and implies active language, so I'd suggest default_language (which happens to be consistent with #2078511: Rename getLanguageDefault() to getDefaultLanguage()).
I think my preference would be system.locale.default_language, which should be a sister of system.locale.country. What do people think before I write a patch?
Comment #2
ianthomas_ukThese changes will conflict / not be possible without #1862202: Objectify the language system. This may be fixed in the final patch for that issue.
Comment #2.0
ianthomas_ukForgot the variable_sets
Comment #3
xjmComment #4
xjmComment #5
ianthomas_uk#1862202: Objectify the language system has landed, but it may now be simpler to fix this together with #2108599: Convert language_default to CMI. Leaving postponed, but now against #2108599: Convert language_default to CMI.
Comment #6
Gábor HojtsyI think you meant #2102477: Convert remainder of language negotiation settings to configuration system.
Comment #7
alexpottSo the patch here is converting default language to CMI. But it is also ensuring that the ConfigFactory always has a language set. In order to resolve the circular dependency the default language is stored on the container and there is a service to retrieve it.
I don't think it makes sense to tackle both language negotiation variables and default language together because default language is so fundamental and can be set in the installed even when the language module is not enabled.
Comment #9
alexpott7: 2108599.7.patch queued for re-testing.
Comment #11
Gábor HojtsyLooks a bit possibly confusing that a parameter and a service has the same name. I know the params and services have different syntax as arguments, but it may end up being confusing nonetheless.
You do these same things multiple times in the code/patch. It would be good to add comments as to why we need to redo these.
We attempted to make really sure that wherever you encounter 'language' in Drupal 8 that would be an object, wherever you encounter 'langcode', that would be the language code. So default_langcode would be more appropriate.
The default_language variable that this patch replaces was an object, so it conformed to this naming convention :)
Original data applies overrides?!?! Also looks like this duplicates logic from the config factory?
This is where the types of the two default_language values are entirely different (and btw neither of them is an object :/).
I know we may not be able to instantiate a Language object here, although it should not need other runtime components to instantiate one. But at least the inconsistency between this array and scalar representation in naming should be resolved.
Ideally the default language would be an instance. Eg. Language::defaultObject() would return an object or new Language() with the default values like it was in the language manager before.
This is very clever. Would need review from the container folks though :D
Are these explicit container rebuilds still needed given that language_save() would do this already via the config event listener?
Or in the later case the config save would result in the event listener called.
Comment #12
alexpott1. Sure - parameter is now language.default_values
2. Actually we don't do it as much and existing comments cover it I think.
3. We're now using the langcode in the system.site file. This was being set to the default language ID at the end of the install process but after discussing with @Gábor Hojtsy and @marcvangend on IRC we agreed that if this langcode was not the site's default language the situation would be very contrived.
4. I thought long and hard about this and yes I think so. If overrides are disabled then there will no overrides here. If they are enabled and you want to react to change then you need to be comparing the value the system has and what you've just set.
5. Fixed when dealing with an array the variables are *_values and when dealing with just a language id we're using langcode.
6. :) Improved comment to say when the container will be rebuilt - which is the next request.
7. The reason why we have to the rebuild the container is because just deleting the PHP storage will not refresh the in memory implementation.
This patch also fixes the installer - at the moment if you install in a language other than english the steps on the left hand side revert to english on the installing profile step!
Comment #13
Gábor HojtsyI think the changes look good.
The reason I'm uncomfortable with getOriginal() is if you compare the newly set value with the original, the newly set value get() will apply the override while getOriginal() will also apply it, so they will be equal even if the underlying value changed. This is currently not a direct problem with the comparison in the patch since we compare langcode and we currently don't put a langcode in the override files (although several people argued that would make total sense, since then you could tell the actual language of the config by looking at the langcode and the override langcode would proberly apply, now you have no way to actually tell the langcode of the current runtime config value). But it will be a problem at least generally for whoever wants to do similar comparisons no?
Comment #15
alexpottThe DUBT and testbase environments needed the
language.default_values
set up so that tests andt()
in the tests will work.Comment #17
alexpottWe need to be able to change the default language mid request and have it take effect so I have changed the
language.default
service on the container from being just an instance ofLanguage
to a newLanguageDefault
service which is a simple getter and setter wrapper around a Language object.Added a new exception to ensure that
language_delete()
does not delete the default language as this is totally unsupported. This business rule is currently only enforced through the UI.What is very interesting is that many of our multilingual tests run in an out of sync environment. This is because once additional languages are added the
LanguageServiceProvider
alters the container and adds a path processor. But in HEADurl_generator
service is persistent so no amount of container rebuilding will ever get this path processor to fire in the instance of the test environment that is actually running the test. However the path processor will be active for any requests made via drupalGet, drupalPost etc... This patch removes the persist tag from url_generator - it depends on way too much stuff to be persistent!Comment #19
Gábor HojtsyI think these changes look great. But the update tests are failing :) However we discussed that the original value problem may not be this easy to solve, and would be better possibly in another issue separately, so we can just rebuild the container whenever the language default is saved. Is that happening too often?
Comment #20
vijaycs85Just re-roll as the tests failing in #17 are green locally.
P.S not related to this ticket, but the failing tests needs message update
Where the 2nd param is message. It can be just
Comment #21
Gábor Hojtsy@vijaycs85: yes, that fail was due to #2176669: Stop persisting the url_generator which was recently fixed. Also I think slightly conflicted with this patch since @alexpott included part of that here originally :)
Comment #22
Gábor HojtsyReviewed again! Looks good except (some non-major stuff):
New argument needs docs.
So it took me some time earlier to realize original != raw. So it would be good to document that original means "in the form it was last saved in" and/or "without unsaved runtime changes" or somesuch. Also I'd mention overrides are still applied.
I see the override application makes no difference for the default language practically, so I leave it to your judgement to include this method/approach here or move to a different issue. I'm actually fine either way (if we make the purpose of this method cleaner with docs).
Comment #23
alexpottThanks Gabor!
Feeback in #22 addressed.
Comment #24
Gábor HojtsyNo other concerns left, looks good to me :)
Comment #25
alexpottActually this is not going to work in its current form - there is no need to set the language on the configuration factory in regular runtime if the Language module is not enabled. Doing so incurs unnecessary costs. The exception to this during the installer which is the only time when Drupal can have a default language other than English and not have the Language module enabled.
How to progress?
Patch coming.
Comment #26
alexpottWith the patch attached the config factory only ever has the language set when the Language module is enabled. During the installer the default language can still be something other than english even though the language module is not enabled. However during the installer the config factory will never have the language set unless the language module is enabled. If the Language module is enabled then the LanugageServiceProvider adds a method call to the config factory definition to set the language to the default language the moment the config factory service is instantiated. That means that if the site is monolingual and has the Language module enabled language overrides still apply (as they should). Currently in HEAD this would actually be broken so I've added a test for this situation.
Comment #27
Gábor HojtsyI think those changes look good. The bug solved there was not introduced by earlier patches in this issue so its really a "side-thing" I think :) Its good to have that resolved nonetheless and made it clearer for us how to approach this. I think it would be good to get in this step ASAP (today before sprint weekend? :), so that we can move on with the remaining variable_get removal in the language negotiation system.
I noticed several grammar issues in comments but those are the only things changed (and there were some offsets when applying), so moving to RTBC as well :)
Comment #28
webchickSince alex wrote some of this patch, assigning to catch.
Comment #29
alexpottNotice a comment in one of the language tests that says we can remove more code :)
Comment #30
Gábor HojtsyHow come not on the sprint :)
Comment #31
catchCommitted/pushed to 8.x, thanks!
Comment #32
michaellenahan CreditAttribution: michaellenahan commentedChange report:
https://drupal.org/node/2182025
Comment #33
michaellenahan CreditAttribution: michaellenahan commentedComment #34
michaellenahan CreditAttribution: michaellenahan commentedSorry I think I was too fast in setting this as fixed. I need a review for the change report, please.
Comment #35
michaellenahan CreditAttribution: michaellenahan commentedI came across this todo while writing the change report, so I guess this needs a follow-up.
Comment #36
vijaycs85Following up #2182093: Fix documentation of Language::$defaultValues
Comment #37
penyaskitoI reviewed the Change Record and edited for clarifying that LanguageDefault is a service.
I think we can set this as fixed now.
Comment #38
Gábor HojtsyWoot, yay!