Set locales for all domains when enabling multilingual #17733
Conversation
|
(Standard links)
|
| while ($domain->fetch()) { | ||
| // skip if the domain is already multi-lang. | ||
| if ($domain->locales) { | ||
| continue; |
There was a problem hiding this comment.
Should we make sure that all domains have the same locales?
There was a problem hiding this comment.
@seamuslee001 I dunno - I think one domain could have less than the others but it gets tricky fast. I decided not to change that edge case
| $queries[] = "ALTER TABLE {$table} DROP {$column}"; | ||
| } | ||
| $domain->find(); | ||
| while ($domain->fetch()) { |
There was a problem hiding this comment.
Something is wonky here... I think some cache is being reset as only first is retrieved
There was a problem hiding this comment.
OK - switched to putting in an array first...
In the process of trying to test a possible wordpress regression I found that enabling mutlilingual on a domain other than domain 1 breaks the site. The reason is that without only domain 1 is being updated with the new language and locales. Since I'm on domain 2 when I do this I can no longer load any page as the locales value has not been set and the queries look for option_value.label not option_value.label_us. Since 'no locale' is no longer valid the locale needs to be set on all sites. Note the code change is primarily an extraction inn order to only run the table update part once
|
So historically domain 1 was treated as the global domain, and I think I messed that up in #17646 and that caused the regression. We've always set the locales value in domain 1 (or technically the first domain since I guess you could delete domain id 1) and my PR changed that. We should probably change it back. |
|
@colemanw I'm unclear as to whether what I hit was 'the regression' - since it was a bit edge-case but yeah maybe it did regress. I think this change probably still makes sense even if we do revert the other |
|
I don't want to completely revert #17646 because the performance boost is a big deal and that was the main focus. Reading from the current domain was secondary so I'll just remove that part. |
|
Hmm tests pass but no test site :-( |
|
test this please |
|
@seamuslee001 any idea about the lack of test site? |
|
@eileenmcnaughton test site seems to be up here http://core-17733-4dpkx.test-1.civicrm.org:8001/ |
|
at least for the latest test |
|
@eileenmcnaughton @seamuslee001 @totten we have multiple discussion threads about this and multiple open PRs. |
|
This is missing an upgrade step so I added one: #17738 |
Co-authored-by: colemanw <[email protected]>
|
This was merged to 5.28 -as was #17646. It fixed a temporary regression but the regression was not in any rc |
Overview
In the process of trying to test a possible wordpress regression I found that enabling
mutlilingual on a domain other than domain 1 breaks the site. The reason is that without
only domain 1 is being updated with the new language and locales. Since I'm on domain
2 when I do this I can no longer load any page as the locales value has not been set
and the queries look for option_value.label not option_value.label_us.
Since 'no locale' is no longer valid the locale needs to be set on all sites. Note the
code change is primarily an extraction inn order to only run the table update part once
Before
When enabling multilinngual for one domain 'locales' not updated for other domains and they become unusable (wordpress)
After
civicrm_domain.locales set for all domains to the language being enabled
Technical Details
@colemanw you attempt to address this here - https://github.com/civicrm/civicrm-core/pull/17650/files#diff-fc3e5db5312bd5c376b4b73c3c1be43fR46 but in fact it needs to be all domains get locales set as it is set for the tables that all domains access
Comments