Updated: Comment #58
Problem/Motivation
The problem is caused when the language defaults for any content type is missing. Any entity that is created programmatically will not set the default language.
Steps to replicate
- Create a new content type.
- Specify the machine name for content type.
- Choose default language as Not Specified from language settings tabs.
- Programatically try to create the entity, the default language will be missing
Proposed resolution
- The correct place to set the default language for EntityNG would be LanguageItem::applyDefault().
- The easiest way would be a if language module exists check there
- if module exists then set language code from the entity using language_get_default_langcode else it will set the language code to LANGCODE_NOT_SPECIFIED.
- There is a possibilty of function setValue is getting moved into the language manager service defined under core/lib/Drupal/Core/Entity/Plugin/DataType/LanguageItem.php
Remaining tasks
- The patch is ready for review.
- also #122 needs addresses.
Related Issues
http://drupal.org/node/1869562#comment-7280616.
Original report by @fago
I just ran into a problem caused by language defaults missing, see http://drupal.org/node/1869562#comment-7280616.
I think that language module should use hook_entity_create() to set the default language, that way any programmatically created entity will get the usual defaults as well.
Comment | File | Size | Author |
---|---|---|---|
#243 | language.entity.kerneltest.patch | 307 bytes | sun |
Comments
Comment #1
Gábor HojtsyWell, recently in #1947814: New config entities often save as langcode: und incorrectly we introduced a different method to set default config entity language, but it can depend on the form as well if the entity had a UI. So not sure what kind of info do we have in the create hook? HOw do you imagine this would work?
Comment #2
fagoThe create hook sets the programmatic defaults. I guess it should resemble the default values of the form, as people would expect a programmatically created entity to match them?
http://api.drupal.org/api/drupal/core!includes!entity.api.php/function/h...
Comment #3
Gábor HojtsyThe form defaults depend on language settings though (eg. per vocabulary, per content type, etc.).
Comment #4
fagoField API defaults depend on configuration also, that's ok imho. We could just use language_get_default_langcode() as I had to do it over at http://drupal.org/node/1869562#comment-7280616?
Comment #5
plachMarking #1834542: Implement hook_entity_create() to setup the default entity language as a duplicate.
Comment #6
plachI agree we should do this:
language_get_default_langcode()
is all we need to implementhook_entity_create()
properly. Depending on any form workflow would be just wrong IMHO.Comment #7
Gábor HojtsyI believe that is only implemented for content entities so far?
Comment #8
plachWell, I think we should put our efforts in making config entities behave as content entities on this specific side. Since we have an API that's perfectly suited to set language default we should totaly leverage it :)
This would have the additional advantage of allowing the entity form controller to automatically provide the language selector instead of forcing us to manually add it for each entity type. AAMOF it would just need to rely on the
language_select
form element and entity language default, no dependency on the Language module of any kind. I can see a lot of potential in this solution and I've been mulling on this for a while now.Comment #9
Gábor HojtsyYeah, so as I attempted to point this out, this is a bit larger a scope then just using a hook :D
Comment #10
fagoHere is a patch that would implement it based upon #1777956: Provide a way to define default values for entity fields - that way the default would be added in by the language field.
Comment #12
andypostthere's no applyDefaultValue() method anymore
Comment #13
plachI hope to be able to provide a new patch for this soon.
Comment #14
plachComment #15
kfritscheComment #16
kfritscheI currently having some problems with this.
I started implementing
hook_entity_create()
in the language module.It uses
language_get_default_langcode()
and sets this as the language of this new entity.Problem is that multiple tests are failing and I'm not sure if this should be changed or not.
Most importantly "core/modules/system/lib/Drupal/system/Tests/Entity/EntityTranslationTest.php":
It directly checks that the language is not defined at, but we are now directly setting it. I don't know why it was intended this way in the test, but won't change it without further feedback.
Also as we set a language at the beginning following test are failing too, as the entity is already a language-specific one. If the test are right in the current way, we would have to replace more or less Language::LANGCODE_DEFAULT with language_get_default_langcode() in EntityNG.
Or I got something completely wrong here... (Still attaching the failing patch with the hook.)
Comment #17
plachI think those tests do not take into account this use case. It would be good to update them to match the behavior implement here. That seems the correct place.
There are thounsand of ways to do this now, but I think the preferred one currently is
$entity->langcode->value = $langcode
.Comment #19
itarato CreditAttribution: itarato commentedComment #20
itarato CreditAttribution: itarato commentedComment #21
plach@itarato:
Are you still working on this?
Comment #22
vijaycs85update, as per #17
Comment #24
penyaskitoWe shouldn't try to override the language of the entity if this was already set.
Comment #26
penyaskitoSpent a lot of time with this exceptions, the problem seems to be that contact_message has no langcode property when we try to assign the default langcode.
Comment #27
Gábor HojtsyComment #28
vijaycs85#24: 1966436-default-language-24.patch queued for re-testing.
Comment #30
andypostConfig entities probably affected too, for example vocabularies
related: we have not enough tests for upgrade path for language defaults - #2049465: Upgrade of image styles and effects broken
Comment #31
victor-shelepen CreditAttribution: victor-shelepen commentedAccording to last test log. Drupal\contact\Tests\ContactUpgradePathTest fails.
It seems resolved here. It is need to be applied to pass next test.
https://drupal.org/node/2032033#comment-7708661
Comment #32
victor-shelepen CreditAttribution: victor-shelepen commentedI duplicate the patch file.
Comment #33
victor-shelepen CreditAttribution: victor-shelepen commentedComment #34
victor-shelepen CreditAttribution: victor-shelepen commentedComment #35
fagoThe default language should be set via the respective language field class, or an overridden variant.
Comment #36
fagohm, one moment: Should the default language apply to all language fields, e.g. user prefferred language etc? If so, it shuold be the field's default value, else probably not.
Comment #37
victor-shelepen CreditAttribution: victor-shelepen commentedlangcode is a code(scting). In the previous versions people used it as the object language. So I need review.
Comment #38
Gábor HojtsyHow is the upgrade hunk related to this patch? Otherwise would be great to see test coverage for this, eg. creating two entities with the API and the UI respectively and check langcodes. There may be existing tests we can easily extend.
Comment #39
Aron Novaktrying to adjust it now
Comment #40
Aron NovakWhat I see so far:
We must not set the not specified constant in the Entity class, otherwise the language module will never face with the situation of not set / empty langcode.
"langcode is a code(scting). In the previous versions people used it as the object language"
Even after the patch I attached, the condition never applies cause:
dpm(Drupal::entityManager()->getFieldDefinitions('node'));
This says that the langcode is a language_field, so it means \Drupal\Core\Entity\Plugin\DataType\LanguageItem .
Then, before create hook is called, LanguageItem::applyDefaultValue kicks in.
For the reference, I used this piece for testing:
And it was executed via drush.
Hopefully i can continue this stuff later next week.
Comment #41
andypostLet's test patch, also related #1987866: Convert taxonomy_vocabulary_add() to a new style controller
Comment #43
victor-shelepen CreditAttribution: victor-shelepen commented@andypost I see it is related with entities. Could give an advice if a test exists that provides a test to check the default language possibility? #1966436-38: Default *content* entity languages are not set for entities created with the API
Comment #44
andypostProbably #2050801: Unify handling of default values between base and configurable fields should help
Comment #45
BerdirThe correct place to set the default language for EntityNG would be LanguageItem::applyDefault(). The easiest way would be a if module exists check there, but language.module could also alter the typed data language_field and replace the used class.
Entity::save() is not the correct place to check this, that would need to go into the save methods of the storage controllers. $entity->save() is just an optional shortcut, it's not guaranteed to be called.
Comment #46
grisendo CreditAttribution: grisendo commentedAccording to @Berdir, the path from work from is #10. It still can be applied with Git, but it generates errors. Also:
I think those lines are covered by language_get_default_langcode
Comment #47
grisendo CreditAttribution: grisendo commentedRerolled from #10 and adapted.
Comment #48
grisendo CreditAttribution: grisendo commentedComment #50
kfritscheSeems fine to me.
I adjusted the tests because these are now not accurate anymore.
Added simple test which testing if it is the default language, if you create a entity without giving any language.
Added to all the other tests the LANGCODE_NOT_SPECIFIED as language so we don't have to touch the test results.
This should go green now.
Comment #51
kfritscheComment #53
grisendo CreditAttribution: grisendo commentedTest fails in core/modules/content_translation/lib/Drupal/content_translation/Plugin/views/field/TranslationLink.php line 63, because in that test, user 2 should have no language and the link should not be visible. But after the patch, probably user 2 has language, so maybe that's why link is visible.
I send a patch that take this in account, forcing user 2 to have Language::LANGCODE_NOT_SPECIFIED
Comment #55
grisendo CreditAttribution: grisendo commentedOk, forgot to add "use Drupal\Core\Language\Language;" ...
Comment #56
grisendo CreditAttribution: grisendo commentedSo, I'll start to work on tests.
Comment #57
BerdirYou can call $this->getEntity() directly.
I think this function is getting moved into the language manager service, so it's OK to add the check here.
Comment #58
leeotzu CreditAttribution: leeotzu commentedProblem/Motivation
The problem is caused when the language defaults for any content type is missing. Any entity that is created programmatically will not set the default language.
Steps to replicate
Proposed resolution
Remaining tasks
Related Issues
http://drupal.org/node/1869562#comment-7280616.
Comment #59
Gábor HojtsySet the title back. Note that the "Issue title" edits the main issue title, it is *not* the title of the comment.
Comment #60
grisendo CreditAttribution: grisendo commentedStill working on tests right now
Comment #61
plach@grisendo:
Are you still on it?
Comment #62
grisendo CreditAttribution: grisendo commentedYes. I'll upload patch later, as soon as tests don't break.
Comment #63
grisendo CreditAttribution: grisendo commentedThings added:
ViewTestData::importTestViews(get_class($this), array('content_translation_test_views'));
toViewTestData::createTestViews(get_class($this), array('content_translation_test_views'));
because of #2095115: delete ViewTestConfigInstaller and have ViewTestData create views, not 'import' themSecond consideration from @Berdir I don't understand what does it mean, can you explain it more in detail?
For now, let's see if we have a green :)
Comment #65
BerdirWe usually try to avoid dependencies to module code in Drupal\Core code, but this method is supposed to move to the Language Manager service, so that can be ignored for now. Just wanted to comment on it, no changes necessary.
Comment #66
grisendo CreditAttribution: grisendo commentedThere was a problem saving default language code on first time the content type is created. Next times it is edited, it was saved correctly.
Changes:
module_exists('language')
to\Drupal::moduleHandler()->moduleExists('language').
Comment #67
vasi1186 CreditAttribution: vasi1186 commentedThe code looks good to me, if the tests pass, I think it can be set soon to rtbc (after some reviews from other people probably).
Comment #69
vijaycs85Fixing tests to work with current code fixes. Basically, we get site default language for an entity, if a bundle created with language code 'und'.
Comment #71
vijaycs85Wrong patch file (test only and diff file are fine).
Comment #72
grisendo CreditAttribution: grisendo commentedComment #73
das-peter CreditAttribution: das-peter commentedJust did a visual review and found some minor things:
We should use
\Drupal::moduleHandler()->moduleExists('language')
insteadmodule_exists()
Can't we use
$langcode = Language::LANGCODE_NOT_SPECIFIED;
as default and get rid of theelse
? (Very personal preference)Method scope modifiers not specified (public).
Do we really need a helper for that? It's called once and the code piece is rather small.
Missing asterisk in the doc block opening.
How about
How about
// With language module activated, this content type has the language "en" by default.
How about
// With language module disabled, this content type has the language "und" by default.
How about
// With language module disabled, this content type has the language "en" by default.
Parameter documentation doesn't match the method signature.
I would replace "existing" with "the":
If blank, it will be determined by the content type configuration.
Do we need a variable here? It's not re-used anywhere.
Comment #74
grisendo CreditAttribution: grisendo commentedI'll check. The thing about module_exists you comment, it was already in #66
Maybe lost in #71?
Comment #75
grisendo CreditAttribution: grisendo commentedI moved back to #66 since entity content type default language is not saved correctly in #71, it get's to site_default (en) even when it's forced to "und" or some other language.
Fixed suggestions from @das-peter. Suggestions 6, 7, 8 and 9 are also fixed, but the comment is not well, that's not what I wanted to explain there, but I agree is quite confusing. I changed them and provided a more detailed explanation in the comments.
[edit]
Also checked
if (!empty($type->language_configuration)) {
when saving content type language configuration. Was so useless to check if language module exists in a language module hook... and also was breaking tests on #66 because $type->language_configuration was undefined.Comment #76
das-peter CreditAttribution: das-peter commentedAwesome, looks pretty nice.
However, there's new code in the patch that wasn't there in #71 and isn't in the interdiff - because the interdiff is 66-75 and not 71-75. That's quite confusing!
This doesn't look right. This should be properly handled in
language_configuration_element_submit()
, right? I think that if this doesn't work, we should try and fix it in there and not create a dependency on node. However, the language module already implementslanguage_node_type_update()
, so it seems like there's already a dependency :|Does anybody know why?
Leaving "needs review" as I don't have enough information to set it to RTBC / needs work.
Comment #77
grisendo CreditAttribution: grisendo commentedConfusion comes to me also because #69 and #71 are supposed to come from #66, but they don't. They remove that lines, but is not reflected on interdiff.
And you are right, that's why I put hook_node_type_insert in that file, because hook_node_type_update is just below.
Comment #78
pfrenssenI looked through the history of how this patch evolved and it also looks to me that the language_node_type_insert() does not belong in this issue. In #66 a test was added, using content types as an example entity, and to make that test pass the hook_node_type_insert() was implemented.
However, the goal of this issue is to make this work out of the box across all entities, without having to implement any entity specific hooks.
Comment #79
mmilano CreditAttribution: mmilano commented@grisendo, I'm at the badcamp sprint. I'll be digging into this one.
Comment #80
Gábor HojtsyComment #81
Gábor HojtsyTag monster.
Comment #81.0
Gábor HojtsyUpdate the issue summary as per the issue log
Comment #82
kfritscheI removed now both the hook_node_type_insert and hook_node_type_update. This is now handled in the elements submit function. Tested it manually for custom block types and node types.
The problem was that on creation with the form there was no bundle name set yet, so this was always empty.
Hope we can get further here now.
Comment #84
kfritscheOkay, I'm lost here now. In Menu and Vocabulary the controller ($form_state['controller']->getEntity()) still has the old not updated entity. In CustomBlockTypes and NodeTypes it already has the new entity here.
When I'm thinking now about it, the Menu and Vocabulary behavior makes from the old D7 logic more sense, as while the submit is happening nothing got saved yet. But makes this somehow much more complicated to achieve there. But nevertheless I'm not sure if this is still regarding to this issue or if we getting here to some other problem. Maybe we should go with #75 and continue in another issue? So at least the default language is set for entities created by the API, what is the main reason for THIS issue?
Comment #85
Gábor HojtsyWell, #75 has language_node_type_insert() for example, which did not look right. So not sure how can we get back there in a useful way?!
Comment #86
Schnitzel CreditAttribution: Schnitzel commentedDiscussed this with Gabor and decided that we rescope this Issue to be for Content Entities only. Because there is actually a question how we should handle Config Entites and their Default Language.
Because of this, I reintroduced
language_node_type_update
again (will maybe be removed later) and also removed the additional code inlanguage_configuration_element_submit
which probably is not really necessary?Should be green now, as it already worked for Content Entities before.
Comment #88
Gábor Hojtsy86: 1966436-default-language-85.patch queued for re-testing.
Comment #89
Gábor HojtsyQualify the scope of this issue is about content entities.
Comment #90
fagohm, I'm not sure about checking for a module in the core component. So I'd rather go for the hook implementation or even swapping out the class. (discussing with yched)
I'd suggest just "Entity default language" - it's not about the default language from translations, right? Same for the class name.
instead of "have language "und" by default - couldn't we just say it has no language specified? I mean that is what und is about.
Then, it should be "and a content type"
"and a content type"
and a ...
Could we move the module handler to $this->moduleHandler ? Usage of the container smells.
Is there a reason we do this via the web test? I mean couldn't that be a DrupalUnitTestCase as a whole as it is about testing the API, not the interface?
Imho this should say it's the langcode to pass to entity_create(), while by default it does not pass a langcode.
Should type hint to node interface.
I'd suggest to just call it createNode() - every function is in code, so that "viaCode" confused me a bit initially. The comment should be enough to clarify it's using the API not the interface.
$data usually is $values.
Comment #91
yched CreditAttribution: yched commentedAgreed with @fago that hardcoding some "if (module_exists('language') { call a function in language.module }" logic in a Drupal/Core library is not ideal.
hook_entity_create() seems perfectly suited for this kind of things (alter the default field values used in an entity_create())
Besides, the approach in the current patch would affect *all* fields using the LanguageItem class, i.e all fields of type "language" (i.e, for example the "user preferred language" field), while we only want to target "the field that holds the language of an entity".
So, hook_entity_create() looks like the right approach - it would need to act just on the 'langcode' field.
(it seems the name of "the field that holds the language of an entity" is currently hardcoded to 'langcode'...)
Comment #92
Gábor Hojtsy@fago/@yched: Yeah the inherent problem with the hook_entity_create() approach is we need to have a default that is enough for the altering to find out the language was *not properly filled in yet* so the entity language is not overwritten by an overzealous entity_create implementation. Using any of the configured language codes for that would be a problem because how do you tell if those are not deliberately chosen by a user from a UI?! Then picking some other funky language code is a problem because then how do you ensure if the langugae module is not enabled that the language will become something sane even then...
So maybe we need a one-off property on each entity then for only this use case? $entity->languageUninitiliazed = TRUE :D? Better ideas?
Comment #93
fagoRegarding the language key problem - I opened #2143729: Entity definitions miss a language entity key.
Yes, the comment says hook_entity_create() is for applying default values but actually we don't know whether we have to apply the default. So, to fix that we could just pass on the $values with which the entity is created to the hook as well, what imo makes sense.
So let's use hook_entity_create() and fix it.
Comment #94
Schnitzel CreditAttribution: Schnitzel commentedtwo related to this issue issues
#2144249: Write test that Config Entites created via API are created in Site Default language
#2144247: Possible Issue with default language when changing name of Configuration Entity via API
Comment #95
Gábor HojtsyAddressed the direct code feedback from @fago on #90 1-10 (all of them), except:
6. The reason this is using the web UI (and therefore webtest) is that the language config element is not on the node type API, its altered in. So we could save it on a different API, but this is all in one, easier to see :)
7-10: I actually removed this in favour of drupalCreateNode() (which is also only implemented on the webtestbase heh). I think that should be fine with langcode NULL but I did not try. There is also in fact a drupalCreateContentType() but that in itself would not save the config for the language, so I did not use that. Even if we would use that, it would appear we don't use any UI, but the drupalCreate* methods are on the webtestbase :)
I did not address the hook entity create stuff yet.
Comment #96
Gábor HojtsyBTW it is also fascinating that the test is in views module. Why not language module? :D
Comment #97
Schnitzel CreditAttribution: Schnitzel commentedReviewed the Code Changes, looks good. Let's see what Testbot thinks
Comment #98
Gábor HojtsyMoved to language module, not in views module anymore :D Also moved to language tests namespace (from system tests namespace :D).
Comment #100
Gábor HojtsyErm, I don't see how #95 would pass and #98 would fail :/ I just renamed the test file. Did it not even run before? :/
Comment #101
fagowell, afaik this is supposed to test the API, so it's strange to bother with the UI then?
As I understood the issue the problem is that node-creation API does not cover the language, thus it's an API problem. Consequently, I'd expect to see an API being tested not the UI on top of it. If it goes via the node form it could be altered in by something, i.e. a regular API usage might not work as expected.
Thus, imo it would be best to do it as unit-test case. I don't think it's a big deal if you have to set content type language defaults somehow differently - that's the API we have....
Comment #102
Gábor Hojtsy@fago: right, drupalCreateNode() and drupalCreateContentType() are only on the web test base though. So we need one-off custom copies of that then on unit test base then. :/ They don't use the UI they use the API already in webtestbase.
Comment #103
Gábor HojtsySo looks like remaining tasks are:
- figure out why the test fails now
- restore the node creation method but with a nicer name and all of #90 6-10 points addressed
- convert the test to a unit test
- move the logic from the language type to the language module entity_create hook passing on the default values, so they can be inspected and the right default can be added.
I'll not be able to work on this more this week, so feel free to continue.
Comment #104
Leksat CreditAttribution: Leksat commentedI made all improvements pointed in #103, except moving logic to hook_entity_create. The reason is that we don't have initial $values array in this hook, so don't know which values was passed in, and which were created from defaults.
For the test I used DrupalUnitTestBase. I also moved the file from the UnitTest namespace (probably that was why test failed).
Comment #106
Leksat CreditAttribution: Leksat commentedComment #108
Leksat CreditAttribution: Leksat commentedWhen I worked on the patch I executed only EntityDefaultLanguageTest on my local machine, but when I submitted my patch, two other tests had failed. I tried to quickly find out why they fail, but I dived so deep in the code and issues, so I need a lifebuoy now :)
Here are some thoughts that came in my mind during my research.
TranslationLinkTest fails because of this change made during #2076445: Make sure language codes for original field values always match entity language regardless of field translatability.
When we change the langcode of the second user in TranslationLinkTest::setUp()
the ContentEntityBase::onChange() throws an InvalidArgumentException.
I don't know if this is a correct behavior.
(I did not check why the second test fails.)
Regarding the issue about should we use hook_entity_create or LanguageItem::applyDefaultValue, I think, that hook_entity_field_info_alter could be used as well to set default value in the langcode field definition. But the hook could be used only after #2114707: Allow per-bundle overrides of field definitions is done.
Here is what I tried, and it worked for me:
IMO it's a good place, because the hook called only once, and then cached (probably we'll need to clear that cache on default language setting update).
Comment #109
Gábor HojtsyJust marked #2156945: Clean up installer entity defaults following bugfix a duplicate. That briefly made HEAD fail due to another patch :/ Let's make sure we have test coverage properly for creating entities without languages and then loading them and ensuring they work.
Comment #110
roderikTaking this ( @ global Sprint Weekend ) ...
Comment #111
roderik106: 1966436-default-language-105.patch queued for re-testing.
Comment #112
roderikSubmitting Leksat's #106 for review without changes.
(It turns out I've just been using it for some self training on installing PHPUnit / running tests for the first time. Being able to learn from practical examples is nice :) )
Hoping to sum up current status of this issue:
#109 : "Let's make sure we have test coverage properly for creating entities without languages..." - the code reads like Leksat has everything covered as per #105
#108 -1 : local tests produce no failures to I'm assuming the encountered issue is gone
#108 -2 : using hook_entity_field_info_alter() reads like a good idea, right? So do we wait for #2114707: Allow per-bundle overrides of field definitions to be committed?
Comment #114
vasi1186 CreditAttribution: vasi1186 commentedComment #115
vasi1186 CreditAttribution: vasi1186 commentedHi,
after spending some time on debugging, I've found that the issue comes from content_translation_load_translation_metadata(), and more specifically because a call to $entity->initTranslation($langcode) for the 'und' langcode is made. As Lekstat says, this has the root in TranslationLinkTest::setUp()
The general issue was that you could not assign anymore the Language::LANGCODE_NOT_SPECIFIED to an entity which had the translation enabled. In this case, the translation was enabled in the ContentTranslationTestBase::enableTranslation().
And the exception was thrown in ContentEntityBase::onChange() (when you tried to change the langcode of the entity) becasue the $entity->initTranslation($langcode)call in content_translation_load_translation_metadata() inserted an entry in the $entity->translations array for the 'und' langcode.
But, as I could find out, the issue was coming from ContentEntityBase::setDefaultLangcode(). The defaultLancode property was set there always to Language::LANGCODE_NOT_SPECIFIED when it could not be set from the language of the item. So, what I did, was to add this:
I think it makes sense that the setDefaultLangcode method uses the language_get_default_langcode() if possible before Language::LANGCODE_NOT_SPECIFIED (unless there was a good reason why to not use here).
Let's see if the test bot will give us a green for the tests.
The next thing would be that this code:
is now used in two more places (at least 2 which are introduced in this patch). So, should we think to another solution for this?
Comment #116
vasi1186 CreditAttribution: vasi1186 commentedComment #117
Gábor HojtsyYeah the theory was definitely that it would be enough to set in the LanguageItem. @fago, @yched?
Comment #118
yched CreditAttribution: yched commentedI'm afraid I'm a bit lost here :-( - @plach / @fago would probably know better.
Comment #119
vijaycs85Test is green in #115. Updating it with small code fix. Do we still have any item to address here?
Comment #120
Gábor HojtsyI think #117 still needs to be addressed. Doing this at two places was not something @fago liked back then :/
Comment #121
fagoYeah, I think berdir in #45 pointed it out quite well:
Yeah, problem is LanguageItem is provided by the Entity component. Couldn't we just move LanguageItem to the language module? I don't think it has to be in the component?
Also, the default logic shouldn't be duplicated - yes. Not sure what's the problem here, but we should fix the real problem instead.
Comment #122
fagouhm - as berdir pointed out in IRC language.module is optional (I forgot), so that's a bad idea then. Else, I think it should implement the field_info_alter hook and swap out the language class with a variant that the default value.
We go with that pattern already in entityreference module, which swaps out the default entityreference class.
Comment #124
vasi1186 CreditAttribution: vasi1186 commented119: 1966436-default-content-entity-language-119.patch queued for re-testing.
Comment #126
vasi1186 CreditAttribution: vasi1186 commentedSeems that a method name changed.
Comment #127
segi CreditAttribution: segi commentedI'm rerolling this patch.
Comment #128
segi CreditAttribution: segi commentedComment #129
segi CreditAttribution: segi commentedHere it is the rerolled patch. I only rewrite the patch in comment #126.
Comment #131
segi CreditAttribution: segi commented129: 1966436-128.patch queued for re-testing.
Comment #132
segi CreditAttribution: segi commentedComment #133
YesCT CreditAttribution: YesCT commentedI think it's needs work for #122
Comment #134
BerdirWe also lost the test file in the latest re-roll, that needs to be re-added from the previous patch.
Comment #135
segi CreditAttribution: segi commentedComment #136
jlbellidorerolling #126
Comment #137
jlbellidoAttached a new patch rerolling #126 again. Test is inclued into the patch.
Comment #139
jlbellidoI'll work on this
Comment #140
jlbellidoContentEntityBaseUnitTest fails because of this:
This test was added in #2134857: PHPUnit test the entity base classes, i don't know how to manage this. How it should be managed?
Comment #141
ehegedus CreditAttribution: ehegedus commentedIt still applies
Comment #142
jlbellidoUpdated ContentEntityBaseUnitTest to add $moduleHandler as MockObject and solve the testing errors.
Thanks a lot to @Berdir for help me here.
Comment #143
ianthomas_ukCan we inject the module handler?
language_default() is deprecated
Comment #144
jlbellidoThanks for review, i'll do.
Comment #145
BerdirWe could use the same pattern for the module handler with a getModuleHandler() method within ContentEntityBase but before you spend too much time on that, we should verify if this is actually the correct approach.
Comment #146
jlbellidoInjected \Drupal::moduleHandler() at ContentEntityBase Class and fixed language_default() call.
Comment #148
jlbellidoFixed errors at EntityTranslationTest, sorry for noise.
Comment #149
kfritscheDone multiple things:
* Re-roll
* Fixed fago's comment from #122, therefor introduced new language item in language module, similar to entity_reference, which overwrite the original languageItem. Probably we will rename this on? Don't know exactly how to name it at best?
* Found at the end the reason, why we had to introduce setting the default two times. The defaultLangcode in ContentEntityBase is only changed in the onChange method. But the FieldItemList calls applyDefaultValue($notify = FALSE) and therefor the onChange method will not be called and the defaultLangcode will not be changed.
Comment #150
yched CreditAttribution: yched commentedSo this switches in a different FieldItem class for 'language' in order to be able to override FieldItem::applyDefaultValue().
Meanwhile, #1979260: Automatically populate the author default value with the current user introduces a one-off OwnerFieldDefinition class class to be able to override FieldDefinition::getDefaultValue().
We should really try to come with a unified and more flexible way of specifying custom runtime behavior for default values for base fields (configurable fields allow "default value callbacks" already). If I get things right, that should be addressed in #2226267: Improve default value handling of fields to be consistent.
Comment #151
plachThis is mostly looking good, I have some remarks though:
Eeew, luckily it seems this is no longer needed :)
Missing trailing dot :)
Can we put this call in a separate method so we can unit test this class by mocking the method?
This test looks really good :)
Please let's use the entity storage directly instead of calling the procedural wrappers from OO code.
Comment #152
kfritscheJust fixed all the stuff mentioned in #152.
Comment #153
BerdirThe unit test changes should no longer be needed then?
I guess we should now also be able to clean up some manual code that we currently have, like NodeController::add()? Checked Term and CustomBlock and they apparently don't do this currently.
Comment #154
kfritscheThanks Berdir for pointing this out.
I removed this from the test.
Searched for "get_default_langcode" in core/ and found that is was used for setting default values in:
* ContentEntityNormalizer::denormalize
* NodeController::add
* ShortcutController::addForm
* TaxonomyController::addForm
I removed this from all of these.
Comment #155
BerdirJust field type, not entity field type. Wondering if it would be clearer if you would simply write "Replaces \Drupal\Core\Field\Plugin\Field\FieldType\LanguageItem ...". It's a bit a long namespace...
No "." at the end of a @see.
Method is missing some basic documentation for the argument and return value.
Didn't have a close look at the tests but other than the points above, this looks good to me, nice work.
Comment #156
mr.york CreditAttribution: mr.york commentedDone.
Comment #157
plachThe type hint class/interface needs to be fully qualified with it s namespace, sorry :(
Comment #158
vijaycs85Updated...
Comment #159
jlbellidoComment #160
dags CreditAttribution: dags commentedI picked this up and tried to reproduce. Having trouble creating the node programmatically. I wrote a small script that contains the following:
and I try to execute it using
drush eval creating-entity.php
. Nothing happens. No errors, No entity created. Nothing.When I used
drush scr creating-entity.php
, I at least get an error: .........In the middle of writing this, I went back and ran it again and what do you know...
drush scr creating-entity.php
worked. It's creating entity.Comment #161
mr.york CreditAttribution: mr.york commentedI run @dags code with devel/php, works for me.
Comment #162
dags CreditAttribution: dags commentedThere is a test-only patch in #75 (original in #69) that I'm trying to run locally but having trouble. The test fails saying, "Invalid permission administer content types." and "Invalid permission bypass node access." .. which ultimately causes $this->drupalLogin() to fail. Anyone know why this would be happening?
Comment #163
YesCT CreditAttribution: YesCT commentedwell,
ag "'administer content types'" core | grep -v "Test"
says the permission is defined in
core/modules/node/node.module:735: 'administer content types' => array(
In the patch in #75
so maybe node module needs to be required in $modules, and maybe what ever defines bypass node access?
Comment #164
BerdirExactly, node is no longer required, so you need to enable it in your test.
Comment #165
YesCT CreditAttribution: YesCT commented#1541298: Remove Node module dependency from Testing profile was committed on Feb 13 2014. So patches with tests that require node, that are older than that might all need a similar update.
Comment #166
Gábor Hojtsy158: 1966436-default-content-entity-language-158.patch queued for re-testing.
Comment #168
mr.york CreditAttribution: mr.york commentedReroll.
Comment #169
mr.york CreditAttribution: mr.york commentedComment #171
penyaskito168: 1966436-default-content-entity-language-168.patch queued for re-testing.
Comment #173
YesCT CreditAttribution: YesCT commentedfails
MigrateAggregatorFeedTest.php 52
MigrateAggregatorItemTest.php 79
MigrateCommentTest.php 79
MigrateCustomBlockTest.php 62
those fails are from Migrate asserts like:
$this->assertEqual($feed->language()->id, Language::LANGCODE_NOT_SPECIFIED);
$this->assertEqual(Language::LANGCODE_NOT_SPECIFIED, $comment->language()->id);
which assume that und is what the langcode is supposed to be.
but with this patch, they are getting en for the langcode.
what do we want it to be?
Comment #174
Gábor HojtsyDefaults for content entities in D8 is the site default language unless otherwise configured. So if the test is asserting otherwise, that is not the expected behaviour in D8. All content entities should be created with the site default language unless otherwise setup in the test. Not und by default. So looks like we want/need to fix the test.
Comment #175
Berdir*With* this patch yes, not yet without, right now, it's only for *certain content entities created in the UI where the code explicitly sets the langcode*. Now it's everything. So yes, those tests need to be updated as they rely on the default value.
What's a bit weird is that only the combined test failed, not the single ones... ah, that does make sense, because the separate ones don't have language.module enabled. Because the above is only true if language.module is enabled :)
Which nicely shows the problem that you will have in trying to fix this, the behavior is different with/without language.module and the same test runs in both cases, with the same code.
Suggestion: create a $this->expectedDefaultLangcode = LanguageInterface::LANGCODE_NOT_SPECIFIED in the migrate base class, use that for the expected value in the test, then change it to en in the run-everything test.
Comment #176
Gábor HojtsyI'd much rather fix this to apply the site default language even if language module is not enabled which is our idea for D8 instead of working around this bug in the test.
Comment #177
BerdirIt is only a bug if the expectation is that it should be en even without language.module :)
We discussed it, so yes, the expectation is that it should always be the default language by default, in that case, we want to switch the hardcoded not specified in LanguageItem:.applyDefaultValue() to \Drupal::languageManager()->getCurrentLanguage()->id() and fix the migrate tests to assert for en instead of und.
This will likely cause a bunch of additional test failures, but we can fix those in a second step.
Comment #178
BerdirExpecting a bunch of new test fails now, but should be easy to fix...
Comment #180
vijaycs85Comment #181
BerdirYep, should be getId(), not id(), sorry :)
Comment #182
Gábor HojtsyFixed that and another issues in the test.
Comment #184
Gábor HojtsyBased on the fails, it looks like tests are chock full of asserts for und spcecifically :D 412 fail(s), and 21 exception(s) which seems to be all over the place. I'm not sure that is viable to solve here, so it would be good to get back to the dynamic solution for the aggregator migration test then as per the suggestion in #175. Start again from #168 and fix only this for this issue. Otherwise we'll never be done with this.
Not going to do this though as its more involved than what I have capacity for now. Any takers? :)
Comment #185
BerdirOuch. Yeah, that's worse than I expected because it's not just und/en mismatches, a lot tests are falling apart. I suspect most are related to DrupalUnitTestBase or even WebTestBase not properly setting up the current language inside the test? So maybe it should use the site default language instead of current?
Will have a look, it's possible that most fails are caused by that.
Comment #186
penyaskitoThis patch is based on #168.
My view on this is that locale module should not be enabled on migrate test, as during migrate D6-D8 first sprint multilingual was not considered. Removing it makes the tests green locally.
If in any case this is not the desired behavior (which from my POV it is), we should post a bug in the queue, but it should not block to get this patch in.
Comment #187
chx CreditAttribution: chx commented#186 will very likely fail on MigrateLocaleConfigsTest
$this->assertIdentical($config->get('cache_string'), 1);
because the only reason that's not '1' is because the locale module provides config schema and in turn Config casts it based on the schema.Comment #188
Berdir@penyaskito: That just avoids the test failures, not the underlying problem that the default should be en, not und by default, unless you chose it to be that. Based on what Gabor said and what we're doing for the default user and so on in user_install() for example.
I'd still like to pursue the other approach for a moment, let's see how this goes. I'm not exactly sure what the correct behavior is, probably set it to not specified if there is no langcode and default if there is a langcode field? The problem is that by the time setDefaultLangcode() is called, the default values haven't calculated yet..
But first, let's see how many fails will be left with this...
Comment #190
Gábor Hojtsy@Berdir: default to site default language unless language module is enabled in which case it may have other settings and intervene should be the overall rule. I'm not sure it will not explode the scope too much to fix that here, but I'm confident if someone could do it, it would be you :)
Comment #191
Gábor HojtsyBTW http://drupalcode.org/project/drupal.git/commitdiff/902098c just added a @todo in user_install on this issue, heh :D
Comment #192
BerdirFixing the unit tests so we can better see what's left, but the test fails in the log looked much better...
Comment #194
Berdir@Gabor: Well, in case language module is not installed, the current language is always the default language is always en, no need to special case something there.
The question is more about entity types without langcode*, they should really be treated as NOT_SPECIFIED and not 'en'. Will look into that when the patch is green. Also asked @plach for his opinion/feedback, so assigning to him so that he doesn't forget...
* Yes, there are perfectly valid use cases for content entities to not have a language. translation jobs and job items from TMGMT for example as a source and target language but no language itself :)
Comment #195
BerdirLet's try this.
Could really use some reviews :)
Comment #196
Gábor HojtsyI think the logic makes sense as we discussed earlier that entities without language support would not take default language from language manager, that would be odd :D
Comment #197
cesarmiquel CreditAttribution: cesarmiquel commentedI'm at Austin Sprint and this patch is failing due to the PSR-4 change. I'm re-rolling it.
Comment #198
cesarmiquel CreditAttribution: cesarmiquel commentedI re-rolled the patch following https://groups.drupal.org/node/424758. I resolved two conflicts: one in core/modules/text/src/Tests/TextWithSummaryItemTest.php and the other in core/modules/serialization/src/Tests/EntitySerializationTest.php. One was a function which was no longer in TextWithSummaryItemTest.php and the other one extra key 'default_langcode' in testSerialize() which is also no longer there.
Comment #200
cesarmiquel CreditAttribution: cesarmiquel commentedI seem to have found the problem: two files were removed from the patch. I re-rolled and I'm submitting the fixed patch.
Comment #201
cesarmiquel CreditAttribution: cesarmiquel commentedI seem to have found the problem: two files were removed from the patch. I re-rolled and I'm submitting the fixed patch.
Comment #203
cesarmiquel CreditAttribution: cesarmiquel commented200: 1966436-default-content-entity-language-200.patch queued for re-testing.
Comment #204
Berdir200: 1966436-default-content-entity-language-200.patch queued for re-testing.
Comment #206
BerdirThere wasn't much that didn't conflict in this patch...
Comment #208
BerdirHm, wasn't able to figure out that test fail yet.
Comment #210
Gábor HojtsyRemove config tag, this does not deal with config anymore but that was broken out to #2144249: Write test that Config Entites created via API are created in Site Default language in #86-#89.
Comment #211
naveenvalechaRerolled the above patch #208 patch .As the patch was not applied so I applied it manually. So no diff attached.
Also the Book test passed locally.Looking for the testbot what it shows.
Thanks
Naveen Valecha
Comment #212
naveenvalechaRerolled the above patch after fixing some of the space issues.
Thanks
Naveen Valecha
Comment #215
naveenvalechaAdded the fix for the BookApitest.php
Thanks
Naveen Valecha
Comment #216
naveenvalechaChanged status
Comment #218
naveenvalechaHi
If I will enable the user module in EntityDefaultLanguageTest.php .Then these test fails.
Looking forward to hear regarding the above errors fix.
Thanks
Naveen Valecha
Comment #219
vijaycs85Reroll+test case update...
Comment #220
Gábor HojtsyComment #221
fagoOnce the default value handling has been moved to the new approach (see below), it should be possible to get default value from the "langcode"'s field definition without instantiating the respective field object, thus without duplicating the logic here.
As noted in #2318605: uuid, created, changed, language default value implementations need to be updated this default value logic is deprecated. We should use the new one and implement a respective FieldItemList class which can process the default value to the right value (unless specified otherwise).
Comment #222
jhodgdonQuestion: I have been noticing that even in the UI, if you create an entity before the Language module is installed, the language is unset. See #2323935: Translations page for content created prior to enabling shows "unpublished" for English versions... Is this the same as the current issue, or is it a separate issue?
Comment #224
Gábor Hojtsy@jhodgdon: it is theoretically a different issue, but being resolved here as it is technically closely related. Its the same code. See below:
These change the default behavior to create entities in the language of the page instead of not specified. So without the language module they will all be English.
These ensure that when the language module is enabled, it then uses the configured default language for each entity/bundle.
The without-language-module changes are necessary to do here because the defaults would be very different with the language module if you use the API. So the defaults need to be aligned for both cases.
Comment #226
jhodgdonThis issue is blocking progress on several other issues... what still needs to be done?
Comment #227
Gábor Hojtsy@jhodgdon: todo: 1. reroll 2. feedback in #221 as far as I see.
Comment #228
BerdirRe-roll.
Comment #230
Gábor HojtsyHand edited language_entity to configurable_language. The internal entity type name changed since the last patch.
Comment #232
jhodgdonI looked carefully through the current patch... aside from the 1 test failure (which is hopefully easy to address), it mostly looks great!
So... I'm a bit confused at the disconnect in the default settings for language, between when language module is enabled and not:
So it looks like sometimes the default is the site default language, and sometimes it is the UI language. Shouldn't these be consistent?
Then in core/modules/language/tests/src/EntityDefaultLanguageTest.php -- it seems to be testing that you get site default language without Language module installed, and a configured definite language if Language module is installed... which is the defined behavior from above. But it doesn't seem to test configuration for other choices of bundle language I think, such as "author's preferred language" etc. Are there tests elsewhere for that?
Then:
I think this should be testing that the language matches LanguageInterface::TYPE_INTERFACE, not TYPE_CONTENT? I know they would normally match, but...
Comment #233
Gábor HojtsyFirst fixing the fail. We now have tests for admin language negotiation, which assumed the admin preferred language on the user is not initialised to any meaningful value until the user picks something. This is the expectation. Now with the language field defaulting to things (which is great otherwise), the test failed. The fix is easy, we provide an empty default for this field in the base fields definition.
Comment #234
Gábor Hojtsy@jhodgdon: the difference in defaults looks like semantics. It is default to the page negotiated language until the language module is turned on when it defaults to the site default language. But if you consider what may be the page negotiated language before the language module is turned on, that would also be the site default language. So the defaults are *practically* the same AFAIS, the semantic difference is we use different code to retrieve them. This is getCurrentLanguage() implemented in the default LanguageManager (used before language module is enabled):
We can make this more explicit by using the getDefaultLanguage() method instead for the default cases. This should lead to the same results (holding breath :D).
Your other two points to be addressed later.
Comment #235
Gábor Hojtsy@jhodgdon: as for your two remaining points EntityDefaultLanguageTest.php is indeed not testing all combinations but LanguageConfigurationElementTest already tests all variations of configuring with the dynamic language settings and that they end up with the right values returned from language_get_default_langcode().
And finally for the misuse of TYPE_CONTENT, we could just as well check for the site default language there, since we don't specifically configure the type for any other behaviour. Patch updated.
Comment #236
Gábor Hojtsy@fago/#221: I went to #2318605: uuid, created, changed, language default value implementations need to be updated and to #2226267: Improve default value handling of fields to be consistent from there and found no change notice or explanation as to what before code should be what after code. I would really appreciate some help figuring this out because I have no idea.
Or alternatively fix the issue and do the conversion differently, since this is a bug in the code regardless of conversion (the bug would be there even if converted). Not trying to do multiple things at once in an issue is always something I value for reviewability and committability without arguments.
Comment #237
fagoyeah, I guess I was a bit too quick with #221 - please ignore it ;) We should work on #2318605 first and make sure we've figured out how we do field-type default values and then apply the same pattern here and to other field type modues. There is no reason for language having to be first.
I took a short look at the code and the default value logic used follow the same pattern as other existing field types, thus imo is fine for now.
Comment #238
jhodgdonLooks good to me! This is blocking other issues... has tests... makes things more rational... let's get it in.
Comment #239
webchickLooked through and couldn't find anything to complain about. Very nice and well-commented test coverage added.
This is not a show-stopper by any means, but it's very strange to me to declare such generic helper functions in one arbitrary test class vs. a base class that other tests can take advantage of. Maybe we could have a follow-up to discuss moving them to KernelTestBase?
Elsewise, looks good to go.
Committed and pushed to 8.x. Thanks!
Comment #241
jhodgdon"Fixed" status didn't stick. ?!? Thanks webchick!
Comment #242
Gábor Hojtsy@webchick: the methods added here make creating content types / nodes easier with language settings. Not sure those specifics should be on base classes.
Comment #243
sunQuick follow-up: The new kernel test was added in the unit tests directory.
Comment #244
andypostSure
Comment #245
webchickThanks for catching that!
Committed and pushed to 8.x.
Comment #247
BerdirNote: drupalCreateNode() still hardcodes langcode => und: #2333731: WebTestBase::drupalCreateNode() should not hardcode default values, so many tests are not actually running with real life scenarios now...
Comment #248
BerdirComment #250
andyceo CreditAttribution: andyceo commentedFound reference to this issue in user.install file:
Is there a followup somewhere?
Comment #251
Gábor Hojtsy@andyceo: I am not aware of such an issue. Can you open one?
Comment #252
andypostFiled one #2368081: Remove outdated @todo in hook_user_install()