Problem/Motivation
The installer used to not be able to assign proper default languages to entities and was therefore unable to create translatable entities (which need a configured language). This was patched with workarounds in #2148071: Cannot install in foreign language (Undefined index: und in ContentEntityBase->language()). The reason for this not working was that the language list was not properly populated / returned in the installer.
Proposed resolution
The underlying problem was fixed in #1862202: Objectify the language system, so we can remove the workarounds added. The site default language should be applied to entities created in the installer as well. #2148071: Cannot install in foreign language (Undefined index: und in ContentEntityBase->language()) already added test coverage for this (along with the workarounds we now propose to remove).
Comment | File | Size | Author |
---|---|---|---|
#21 | 2156945-21.patch | 1.12 KB | amateescu |
Comments
Comment #1
BerdirTest coverage for this was added a short time ago in #2148071: Cannot install in foreign language (Undefined index: und in ContentEntityBase->language()), that's why HEAD is currently failing.
The fix there was to set the language explicitly.
Comment #2
xjmEdit: berdir was faster, again. :)
Comment #3
amateescu CreditAttribution: amateescu commentedSo.. we should document this somewhere or is there anything else we can do?
Comment #4
webchickI think the code in #0 should be made to work. I'd love to see EntityContent base class (or whatever) automatically setting the language property if it's not specified.
Comment #6
Gábor HojtsyHaving a default language for entities when not specified is covered in #1966436: Default *content* entity languages are not set for entities created with the API, and that is not that far if someone can take it over from @Schnitzel who apparently does not have time for it anymore :/ I think that fully covers what people propose here?
I assume the "HEAD fail" comment was temporary and a patch was rolled back to fix it?!
Comment #7
BerdirKeeping this open for now.
I don't think the referenced issue fixes this. That would only change und to $default_language, but we don't haven have und, apparently only during the installer, otherwise the default behavior works.
The defaults are applied in FieldableDatabaseStorageController::create() makes no sense to me at all how we'd be able to circument that, will try to investigate tonight.
Comment #8
BerdirOk, I think I know what's going on.
The problem isn't that the langcode isn't set to the default, that works just fine.
The problem is that
language_list(Language::STATE_ALL);
during then installer when do select a different language than english does *not* return LANGUAGE_NOT_SPECIFIED/und. That seems wrong to me?So what happens is that the following runs into the else, defaultLanguage is und, that is not in the list, boom.
So, in my opinion, this makes this a bug in the installer, not the entity system and #2148071: Cannot install in foreign language (Undefined index: und in ContentEntityBase->language()) therefore unfortunately only a bandaid fix (with useful test coverage! ;)).
And #1966436: Default *content* entity languages are not set for entities created with the API would in a way solve this, by resulting in the same bandaid, namely that the langcode of the newly created user or other entity would match the single existing language, but that doesn't seem like the proper fix to me.
Comment #9
Gábor HojtsyIt is not a good idea to create entities without specific languages if they contain textual information in some way. Then the best practice is to have a concrete language on it, so if/when they become translatable, the source language is properly known. Or even if they don't become translatable ever, views with language arguments would work well. So #1966436: Default *content* entity languages are not set for entities created with the API applies that best practice.
Anyway, while in most cases not a good idea, it is still a feature to create entities with unknown language, and it should not break in any way. I looked at https://api.drupal.org/api/drupal/core%21includes%21bootstrap.inc/functi... and theoretically it ensures that before language module is installed, the language list would contain the locked languages while once language module is installed, it would take the list from the language entities.
There may be a short window, when the modulehandler believes it is installed but the language entities are not in place(?) and/or when we preload the local cached list but don't clear it after the language module is fully installed, so in the same request it still is an empty language list. Just thinking out loud here.
Comment #10
Gábor HojtsyComment #11
plachOn my todo list :)
Comment #12
Gábor Hojtsy#2171015: Drupal 8 HEAD broken: installing Language module fails, after that cannot install any other module is likely fixing this issue as well.
Comment #13
amateescu CreditAttribution: amateescu commentedbroken_multilingual_installer.patch queued for re-testing.
#2171015: Drupal 8 HEAD broken: installing Language module fails, after that cannot install any other module was committed, let's give this another try.
Comment #16
Berdirbroken_multilingual_installer.patch queued for re-testing.
Comment #17
YesCT CreditAttribution: YesCT commentedSo, head passes with that, so I think this is active?
We need a discussion on what the real problem is, and how to fix it.
I'll try updating the issue summary. But I dont really understand the discussion, so will need some help there.
Maybe we need to define the use cases that core currently doesn't allow. (like creating entities with an unknown language? does that mean with langcode = und, or without langcode set? or?)
saving the summary to get some feedback on it.
Comment #18
BerdirRight, so the assumption that the objectify language issue fixed this has been proven as TRUE. This essentially means that the bug here as been resolved.
So we can either close this as duplicate/fixed, or we can check if there's some cleanup that we can do, e.g. for the shortcut quickfix that went in to make sure it has a language.
Comment #19
amateescu CreditAttribution: amateescu commentedWell, if that patch passed, is there a problem that we still need to talk about?
Edit: crossposted with Berdir.
Comment #20
amateescu CreditAttribution: amateescu commentedThis is what we should be able to clean up.
Comment #21
amateescu CreditAttribution: amateescu commentedErr, no, user_install() doesn't use the APIs so that will probably have to stay.
Comment #23
Gábor HojtsyThe proposed cleanups look good to me :)
Comment #24
Gábor HojtsyRetitled for current scope. Looks good as said :)
@YesCT can you help with updating the issue summary? :)
Comment #25
Gábor HojtsyUpdated the issue summary to be short and sweet :)
Comment #26
alexpottCommitted 98ea163 and pushed to 8.x. Thanks!
Comment #27
Gábor HojtsyYay, thanks! Removing from sprint.