Problem/Motivation
While working on #1966436: Default *content* entity languages are not set for entities created with the API we figured that Drupal 8 misses the 'language' entity key, as it got introduced for Drupal 7 in #1495648: Introduce entity language support.
Right now Drupal 8 hard-codes "langcode" as language key, but it should respect the defined 'language' key as we do for the other keys.
Proposed resolution
Check for what language key is defined.
Remaining tasks
None
User interface changes
None
API changes
Only the langcode
entity key API addition.
Beta phase evaluation
Issue category | Bug because we have a language entity key in D7, hence this fixes a regression. |
---|---|
Issue priority | Major because all D7 sites exploiting the language entity key could not upgrade properly to D8 without schema changes or ugly workarounds. Not critical because this should not be a very frequent use case. |
Prioritized changes | The main goal of this issue is fixing a regression and removing hardcoded assumptions in the whole Entity API, hence this reduces fragility and provides a more stable API to work with. |
Disruption |
This may require a few patches concerning the entity or language system to be rerolled. A minor API break is introduced by this issue: entity type providers will need to add a langcode key in the entity type definition, if the entity type is language aware. |
Comment | File | Size | Author |
---|---|---|---|
#155 | 2143729-language-entity-key-155.patch | 85.35 KB | plach |
Comments
Comment #1
fagoComment #2
fagoComment #3
Gábor HojtsyAdd some D8MI tags.
Comment #4
Gábor HojtsyAlso this is a regression based on #1495648: Introduce entity language support in Drupal 7.
Comment #5
damiankloip CreditAttribution: damiankloip commentedWorking on this now.
Comment #6
BerdirFYI: There are a few hardcoded assumptions in ContentEntityBase on langcode. And a possible conflict if it would be named language.
Comment #7
damiankloip CreditAttribution: damiankloip commentedMade a start on this the other day, but no time since then.
Here is that start.
Comment #8
tstoecklerThe config system hardcodes 'langcode' everywhere including outside of config entities, i.e. in normal config. It should enforce that the langcode key is 'langcode', otherwise all hell breaks loose.
Will take a stab at this now.
Comment #9
damiankloip CreditAttribution: damiankloip commentedThen we might as well not bother with this patch?
Comment #10
tstoecklerHere we go. This removes most of the references to
'langcode'
in the entity system.There was some code in the EntityManager to check that entity key fields, such as bundle, langcode, etc. are not translatable, but that was broken. Fixed that because I had to touch that code anyway.
Let's see how badly this breaks.
Comment #11
damiankloip CreditAttribution: damiankloip commentedThe new patch that you have posted is nothing like #7 and is nothing like the issue title. At all. Makes it basically pointless.
Comment #12
tstoecklerRe #8: Well we should still allow this for content entities, only for config entities it doesn't make any sense as far as I'm aware.
Re #10: Sorry, the patch is (obviously?) completely wrong. The interdiff is (almost) correct vs. what I had on my system. Can you elaborate on why the interdiff is problematic/wrong?
Comment #13
tstoecklerSo here's the patch and interdiff I actually wanted to post.
Also unassigning for now. I'd like to continue on this, but I'd like some feedback from @damiankloip first, because I don't understand his reservations yet.
I'll try to explain my position a little better:
The language support of the configuration system is built in a way that it checks the 'langcode' property of a config item and the it checks for possible langcode.*. overrides. Since the low-level config system doesn't differentiate between simple config files and config entities this rules applies to all configuration. We *could* change the config system to account for $entity_type->getKey('langcode') but that would introduce an inconsistency between config entities and regular config files as the latter wouldn't have such a facility to configure this. Therefore I think we should not allow this for config entities.
Comment #14
tstoecklerDamn it, forgot to merge 8.x. Also removed obsolete @todo, see interdiff. Sorry for the noise.
Comment #17
tstoecklerLet's see, this is re-rolled for the entityInfo() -> entityType() change.
Also, I was a bit too ambitious with EntityManager::getFieldDefinitions() change. We should exclude entity keys such as 'label' or 'weight' from that code path.
Comment #19
Gábor HojtsyHm, "Failed to run tests: failed to login to test site." is not very good :O
Comment #20
tstoecklerHere we go. This adapts the approach based on the 'uuid' key one.
Comment #22
tstoecklerSo as we no longer have a default value for the 'language' entity key (as the key doesn't make sense for non-translatable entities) we need to provide it for the respective entities. This is consistent with the 'revision' key, for example.
Also PhpStorm failed to rename one of the instances of $this->langcode_key.
Comment #24
tstoecklerThis doesn't exist anymore.
Fixing that and also providing a default $langcode in ConfigEntityBase.
Let's see how much that fixes.
Comment #25
tstoecklerHuh, I'm certain I attached the interdiff.
Comment #27
tstoecklerThe aggregator hunks got list in the interdiff, because I merged 8.x in between, sorry. They're just added hunks, though.
The first hunk of the interdiff was a merge error with the config-entity uuid-key patch.
The second one fixes aggregator module and took me quite a while to figure out. Let's see how many fails. This should at least install.
Comment #29
tstoecklerHere's another update. Sorry, no interdiff as I had to merge 8.x a bunch of times. The most important change is that I added a 'language' => 'langcode' key to Comment. I then dug around for a *very* long time because content_translation was severely broken with this patch. I couldn't really figure out why but I noticed it had something to do with ContentEntityBase::init() so this new patch includes #2095919: Kill ContentEntityBase::init() . This fixes the problem at least in local testing. Let's see where we're at.
Comment #31
tstoecklerSo unless I inadvertently broke some other tests somewhere with these fixes, this should actually be green. Yay!!
Comment #32
tstoecklerYay, that took way longer than it should have, but green is green.
Now extracting the various (hopefully) unrelated changes that I made in my many futile attempts to make tests pass:
No interdiff, as I literally removed the relevant hunks manually from the patch file.
Comment #33
tstoecklerSo now that we're green, trying to extract #2095919: Kill ContentEntityBase::init() again to see if it's actually a hard dependency here.
Comment #34
tstoecklerHmm... that's weird. But all the better.
This one implements the @todo introduced in the patches above of adding a 'default_language' key. Hardcoding the "default_" prefix would really be weird and break our standard of providing keys for that. And it doesn't make sense to do that in another follow-up.
I also brought some consistency into the definition of the entity keys. That slightly increases the patch size - sorry - but the previous patch made the situation even worse.
In case this is green this is now ready for some (final?) reviews!! :-)
Comment #36
tstoecklerAww, annotations are.... ..hard?!
Comment #37
BerdirBah, I just lost my review, so just a few quick comments.
- Content entity handling should get easier with #2182239: Improve ContentEntityBase::id() for better DX. This will also conflict with #2134857: PHPUnit test the entity base classes
- I don't understand why the order of entity keys needs to be changed here but I just looked at the last patch.
- Should the key be language or langcode, as we're actually assuming it is a langcode?
- I like langcode(). Why not make language() use langcode() instead of the other way round? Should be easier and faster?
- Why are the overrides in Feed necessary? having a language key should *not* imply that it can be translated IMHO, having entities with a language but without being able to translate it should be a supported functionality without those hacks? That's what isTranslatable() is for?
Comment #38
tstoecklerThis needed a reroll.
Moved the langcode() method to the interface and removed the Feed and Item overrides. In return I added proper language code keys. You're probably right that it will work right. I originally added it because I thought only *translatable* entity types need a language key, but that's not the case.
Regarding langcode() calling language(): Because of the $language_manager->language() call in language(), language() and by extension langcode() currently always returns a valid language/langcode on the system. If langcode() would just return $this->{$this->languageKey} it might be an invalid langcode. (Not sure if that's really such a realistic use-case that we need to prevent, however).
Comment #40
tstoecklerSo the field translatability checking got updated/fixed in the meantime. Let's see.
Regarding the name of the key, i.e. language vs. langcode:
I don't really care. We're currently very inconsistent anyway as we have 'id', but 'revision' (but not 'revision_id').
Comment #41
mauzeh CreditAttribution: mauzeh commentedStarting work on this.
Comment #42
plachSpoke with @Gabor we both agree that for consistency this should be called
'langcode'
key, sorry :)Bogus empty line
Do we really want to have both
::language()
and::langcode()
? And then maybe replace all the occurences out there (thus adding a method call for each of those)? Are you sure we want to go this way? :)I am afraid this would not be a big DX win, and we would pay it in terms of performance and karma ;)
Comment #43
plach@mauzeh:
Crosspost, sorry, I can't assign you back :(
Comment #44
mauzeh CreditAttribution: mauzeh commentedComment #45
mauzeh CreditAttribution: mauzeh commentedThis should fix the failing test. Will ask @tstoeckler on the next steps.
Comment #46
mauzeh CreditAttribution: mauzeh commentedTemporarily changing status to "needs review" to trigger the test bot.
Comment #47
mauzeh CreditAttribution: mauzeh commentedComment #48
mauzeh CreditAttribution: mauzeh commentedComment #51
jsbalseraAfter talk with @tstoeckler I assign this to me and I will work on this.
Comment #52
jsbalseraRerolled and langcode function remove (also from the interface), and changed the language key to langcode.
Comment #54
jsbalseraThere were a lot of languages to change for langcode still there. Anyway, this patch will fail, but I upload it as a WIP step.
Comment #56
jsbalseraSome tests needed to know about the language key...
Comment #57
tstoecklerlangcode
anddefault_language
in EntityTypeInterface::getKeys()This should be 'langcode' now.
I suppose we should call this $langcodeKey now. If you don't use PhpStorm or a similar IDE, please ping me and I will do that rename. I wouldn't suggest doing that manually.
This is missing the 'default_language' key.
This should be removed again. (Sorry for telling you to add this, I was a bit confused in Szeged. :-/)
Comment #58
jsbalseraOk, changes made, and also EntityTypeInterface::getKeys() documentation is a little more polished now, like @tstoeckler and I talked in IRC.
Let's see if it still passes :-)
Comment #59
tstoecklerAwesome, @jsabalsera, almost there. I have two super minor nitpicks left.
Sorry, but this is >80 chars now.
This seems to be an accident?!
Comment #61
jsbalseraComment #63
tstoecklerJust noticed this is not on the D8MI focus board.
Comment #64
tstoecklerNeeded a re-roll.
Comment #65
tstoecklerOops, wrong patch :-)
Comment #67
tstoecklerI'm sorry, I completely messed this up. At this point someone should block my user account for trolling...
So not only did I upload a completely wrong patch in #64 I also uploaded a re-roll of #40 instead of #61. #61 doesn't even need a re-roll. So I literally spammed this issue with complete non-sense. I'm very sorry.
Will look into some of the fails now. Hope that's OK, even though you're still assigned, @jsbalsera.
Comment #68
tstoecklerComment #69
tstoecklerOh, sometimes PHP fatal messages are just not helpful at all. Let's see where this gets us.
Comment #71
tstoecklerI declare this week trivial test fail week.
Comment #73
jsbalseraComment #74
tstoecklerAwesome, thanks @jsbalsera! Anyone want to RTBC now?
Comment #75
tstoeckler73: 2143729-73-language-entity-key.patch queued for re-testing.
Comment #76
tstoecklerLet's see if #2182239: Improve ContentEntityBase::id() for better DX broke this.
Comment #78
jsbalseraReroll
Comment #79
Berdirlangcode is a key now, so we shouldn't need to check both things now?
I'd love to optimize this based on similar logic as the other entity keys now have in __construct(), which try avoid to create field objects if possible.
Having to add this is IMHO a bit ugly, as default_langcode is more an implementation detail of the database storage rather than an API.
Not what to do with this, did @plach review this already?
Maybe there were other changes before in this file but now this is the only thing left, so you should probably undo the change, the patch has enough noise already :)
Comment #80
tstoeckler3. I agree. I had thought about this in the meantime, too, but at hoped no one would bring this up, so we can do it in a follow-up :-) Should be fairly easy, though, to simply move this to a property on the storage. That way people need to override the storage, if they want to change the name of that database column. But that makes sense, IMO.
Comment #81
plachI agree
default_langcode
does not belong to the entity type keys as well as the various table keys. Not sure if adding it here will make this easier to commit, but we should definitely plan at least one last iteration to remove all entity type properties that are just SQL storage implementation details.Comment #82
BerdirA key in the entity storage class sounds like a good idea to me.
Comment #83
tstoecklerThis should address #79.
@Berdir please see if I adressed #79.2 sufficiently.
Comment #84
tstoecklerwill fix the following once the testbot gets back:
Unnecessary.
This is no longer true.
Should be something like "This is only used if the entity has multilingual support."
This will be properly cleaned up once we split the storages depending on translatability/revisionability
obsolete
obsolete
This should now be reverted (will fail tests).
Comment #86
tstoecklerOops, setting the default value always was apparently not a good idea. Let's see.
Also adds a comment, fixes #84 and renames defaultLanguageKey to defaultLangcodeKey. I still think that defaultLanguageKey would actually be a better name, since this does *not* actually store a langcode but a boolean, but since it's consistently called 'default_langcode' so far in core, let's just stick with that.
Comment #88
tstoecklerI should really know better than to inadvertantly change unrelated stuff by now.
I would have thought removing the hasField() check would be safe, but $this->langcodeKey can be an empty string sometimes, which this apparently is a safeguard for?!
Let's see.
Comment #89
sunSorry, but that states several problems in one go:
Boolean states should conform to the
is*
orhas*
norm:hasLangcode()
.has_langcode
.Didn't look at anything else in this patch; just saw the comment in #86 flying by.
Comment #91
tstoecklerThat's not really true. It's a boolean whether or not the current language variant belongs to the default language or not. It's a denormalization of
base_table.langcode = data_table.langcode
. For revisions we add a similar property - isDefaultRevision - directly as a query expression but for languages we want to be able to retrieve data without joining on the base table, which is why we have that denormalization.Comment #92
jsbalseraComment #93
vijaycs85Working on test failures in #88
Comment #94
vijaycs85one of the failures is the profile image is not getting uploaded on test (works fine in manual testing). both message & file entity annotations missing 'langcode' entity key, adding them doesn't fix the test fails.
Unassigning for now if anyone want to have a look. I will work on this later.
Comment #95
vijaycs85Comment #96
Berdir88: 2143729-88-language-entity-key.patch queued for re-testing.
Comment #98
BerdirRe-roll, added langcode to the file entity.
Comment #100
BerdirUps.
Comment #102
Gábor HojtsyIt is a lie this is on a sprint, nobody is working on this :(
Comment #103
BerdirYeah, yeah :)
Comment #105
dawehnerJust a quick drive-by review.
Did you considered using $access &= ...; ?
Tip: Just use ->willReturn($entity_keys); instead.
Comment #106
andypostRe-roll, also cleaned-up the patch
Comment #108
plachSome additional clean up.
Comment #109
plachOne more
Comment #112
plachThis should fix unit test failures.
Comment #114
plachThis should fix some failures.
Comment #116
plachLet's try this.
Comment #117
plachPatch is correct, I picked the wrong interdiff.
Comment #119
plachI've run a few failing tests locally and everything is green, let's try with a plain reroll.
Comment #125
plachWeird, I'm getting a different set of failures each time and I can't reproduce them locally...
Comment #128
jerdnas CreditAttribution: jerdnas commentedWhat a mess, I don't understand what's about 119patch? May I
Comment #129
BerdirNot sure how this didn't fail for you :)
This should give a better error and fix at least some of them.
Comment #131
plachWell, I did not run all the failing tests locally, but the ones I tried were green...
This fixes a bug this patch uncovers and adjusts the related test accordingly: it's currently wrong because user 0 and 1 always get English language, since they are created before the Language module is enabled.
Comment #132
Gábor HojtsySo what will happen now if you delete a language and then still have entities in that language? Why is moving this code to setDefaultLangcode() ok? Do we have tests for attempting to ask for a language of an entity that is in a language no longer available?
VERY good catch :)
Comment #133
plachCode managing them will still be able to reference their language code, although it'll be unknown to the system. Maybe we should add also a fake language name, like:
Unknown (it)
.Because that's the earliest place where we know the default langcode, it's called during entity object construction.
All the translated installer tests are actualy covering this case (that's how I discovered it), because English is not available but user 0 and 1 have English language. We could add explicit coverage though.
Comment #134
BerdirComment #135
Gábor HojtsyThanks for cleaning this up. As for the tests, its fine if there is already coverage IMHO :) Thanks!
Comment #136
Gábor HojtsyReviewed the patch in its entirety (not just the last interdiff as above). I think it looks good. Also, it has the signoff of entity system maintainer Berdir.
Comment #137
alexpottWe have no test coverage of an entity type which does have langcode as a key.
Comment #138
plachJust a reroll for now.
Comment #139
YesCT CreditAttribution: YesCT commented@alexpott Did you mean does *not* have 'langcode' as the key?
Comment #140
plachThis should improve test coverage. I could not fix all the failures yet.
Comment #141
alexpottre #139 - Yes indeed - it is amazing how I can think one thing and type another.
Quick review of #140
Huh?
Also why all the unrelated change of $entity_type to $entity_type_id?
Comment #142
plachThis is still a work in progress, I was debugging only the new entity type.
I wll revert them, I needed to "free up" the
$entity_type
variable to store a definition, but it's adding too much noise.Comment #145
plachHere we go, this should be hopefully green.
I'm attaching also an interdiff with #131 for reviewers' convenience.
@alexpott:
Great point about test coverage! This allowed to spot at least two bugs in the previous version. Now we should have coverage for all the most common entity operations.
Comment #146
plachAdded beta evaluation.
Comment #147
plachDraft change record at https://www.drupal.org/node/2396385
Comment #148
Gábor HojtsyMinor: "API *tests*" (word missing)
Major: this method does not seem to be called neither in this interdiff nor the patch applied to core
Major: these are not invoked either, just defined.
Minor: incorrect phpdoc, should be more specific.
Comment #149
plachFixed #148.
Comment #150
plachRerolled after that #2230637: Create a Language field widget and the related formatter went in (yay!) and removed some outdated @todos. Let's see what the bot thinks.
Comment #151
plachSome additional clean-up after #2230637: Create a Language field widget and the related formatter.
Comment #152
plachComment #153
Gábor HojtsyLooks good to me, adds the missing test coverage, cleans up @todo comments referring to this issue :)
Comment #154
alexpottThis seems odd that it is possible. Also how does it relate to this patch?
Should we not throw a more specific exception? Also do we really need this - we assume we have an ID key.
Is this change necessary?
We have two calls to this method which already pass a third parameter
Comment #155
plach1: This uncovers a bug in the installer: when a language different form English is chosen, user 0 and 1 are still created with language
'en'
because the Language module is not enabled yet; when it is eventually enabled, the English language is disabled in favor of the chosen one and things start breaking. This is now covered by the regular installer tests: I had to fix it to make them pass.2: Good point, removed the exception.
3: Yep, the value of the language key affects the order in which strings are collected, so we need to sort arrays and remove keys to compare them properly.
4: Fixed
Comment #156
plach#154.1 can happen also if a language is disabled after being assigned to any content entity.
Comment #157
plachGreen, back to RTBC
Comment #158
plachComment #159
webchickAlex has been all over this one, so assigning to him.
Comment #160
alexpottCommitted b778bfd and pushed to 8.0.x. Thanks!
Comment #162
plachYay, thanks!