Now as the new entity field API got committed, we need to convert existing entity types to make use of it. See #1346214: [meta] Unified Entity Field API and the "Entity Field API" tag.
We decided to exploit the EntityBCDecorator
to perform a partial conversion, allowing us to convert only the key pieces of code an leave the rest to follow-ups. This in turn leads to more manageable patches and less changes in a single issue. To be able to follow this approach we need to change all type hints referring to the Node
entity to just EntityInterface
, which moreover is the recommended way according to our coding standards. This will also pave the way for #1810370: Entity Translation API improvements.
Follow-ups
Comment | File | Size | Author |
---|---|---|---|
#196 | interdiff-node-ng-1818556-184-196.txt | 577 bytes | das-peter |
#196 | node-ng-1818556-196.patch | 199.7 KB | das-peter |
#184 | interdiff-node-ng-1818556-182-184.txt | 948 bytes | das-peter |
#184 | node-ng-1818556-184.patch | 199.76 KB | das-peter |
#182 | interdiff-node-ng-1818556-178-182.txt | 5.29 KB | das-peter |
Comments
Comment #1
das-peter CreditAttribution: das-peter commentedAs mentioned here #1498674-83: Refactor node properties to multilingual I've started to build an adapter with the goal to make the NG things fully backwards-compatible.
As I'm not able to continue the work over the weekend I'd like to post what I've done so far.
To use an entity class with
EntityNG
fully backward compatible following steps are necessary:EntityNGHelper
insteadEntity
in the entity class.EntityNGHelper
is the decorator which ensures full backward compatibility.DatabaseStorageControllerNG
in the entity storage controller.baseFieldDefinitions()
to the entity storage controller to declare the entity properties.EntityFormControllerNG
in the entity form controller.$entity->__toArray()
.Known issues:
DatabaseStorageController
is missing inDatabaseStorageControllerNG
. I tried to adapt it but it's not complete yet.EntityNGHelper
tries to be a smart-ass and unsets the entity properties in theinit()
automatically. I'm not sure if this approach works in all cases, maybe we should require to implementinit()
even ifEntityNGHelper
is used - similar to the creation ofbaseFieldDefinitions()
which's necessary in any scenario.The patch contains parts from here: #1778178: Convert comments to the new Entity Field API
Let's see what the testbot says, fails expected. My local tests fail primarily on revision related stuff.
Comment #3
fagoWhile discussing on IRC with das-peter we came to the following idea for easing conversion:
$entity->ng()
which returns a decorator allowing us to work with the new Entity Field API as usual, i.e. doing$entity->ng()->title->value;
(i.e. forward__get()
calls to$this->entity->get($field))
$entity->ng()
but just return $this there. Then all the NGcontrollers like the form, storage and render controller can go with$entity->ng()->field
and/or just add the line$entity = $entity->ng();
to the beginning of their methods.This should help use to speed up conversions as existing code continues work. At the same time new code can start leveraging new functionality via
$entity->ng()
. This workaround is not nice, but I think it's the most practical at the moment. Once the conversion is fully done, all the ng() calls should be removed rather easily by some search/replace magic.Comment #4
plachWorking on this.
Comment #5
fagoTaking over for now.
Comment #6
fagook, here is patch implementing the compatibility mode helper as discussed. Then, when we implement the Node class based upon EntityNG we can force it to have compatibility mode enabled.
All the controllers around like the form controller can probably stay with the non-NG variant for now then.
Comment #7
plachTaking back :)
Comment #8
plachtagging
Comment #9
das-peter CreditAttribution: das-peter commentedI've created this spin-off: #1831444: EntityNG: Support for revisions for entity save and delete operations
Hope this doesn't interfere with the work of plach.
Comment #10
das-peter CreditAttribution: das-peter commentedNext step for #1498674: Refactor node properties to multilingual is done: #1833334: EntityNG: integrate a dynamic property data table handling
Comment #11
tim.plunkettThe other decorators in core all use __call() to pass all calls through. And when implementing an interface, the proper pattern is
The docblock needs to indicate the exact method, and each body can use the same __call()
Comment #12
YesCT CreditAttribution: YesCT commentedI think this is still blocking #1498674: Refactor node properties to multilingual
might be waiting to come back to this after one of the other issues though
Comment #13
fagoas discussed, I've created a patch which enable the BC decorator to work with converted entity properties - see #1890242: Improve the EntityBCDecorator to support converted entity properties.. This would enable use to go with two-step conversion as described at #1818580: [Meta] Convert all entity types to the new Entity Field API.
Comment #14
plachI'll start working on this tonight. I'll incorporate #1890242: Improve the EntityBCDecorator to support converted entity properties. as part of this issue to be able to proceed. We will just merge it in after it's committed.
Comment #15
plachHere is a first stab: it doesn't work at all, but wanted to post it here anyway. At the moment I'm stuck with a fatal in the BC decorator: Comment tries to access
$node->comment
and fails because the$node->decorated->values
array holds the'comment'
value in$node->decorated->values[LANGUAGE_DEFAULT]
instead of$node->decorated->values[LANGUAGE_DEFAULT][0]['value']
. Not sure why, but it seems I cannot make it behave as inEntityFieldTest::testBCDecorator()
.I'll go on with this tomorrow.
Comment #16
plachComment #17
BerdirPlayed around with it a bit too. I was able to hack around until the example test case there worked but it's ugly.
- Also passing the BC entity to hooks, return it from create and pass it to a number of node specific functions.
- Type hints are tricky: I had to copy EntityBCDecorator to NodeBCDecorator and extend that from Node:
- Not sure what's going on with the decorator exactly but at some point, the values are directly in $this->decorated->values['und'], without any subkeys. Maybe that's an optimization from fago somehow? Or related to the fact that we have no data table?
- Loading by properties seems to be broken, or the values are not properly stored, haven't checked.
- $node->original is also a fun case, added a special case for that.
The Node Save test now runs through without fatal error or exception but there are still a few exceptions and failures.
Comment #18
chx CreditAttribution: chx commentedcrosspost.
Comment #19
webchickThis is at least major, if not critical, because translations are basically useless without node titles being translatable, which I understand requires this to happen.
Comment #20
plachMerged #16 in http://drupalcode.org/sandbox/plach/1719670.git/tree/refs/heads/8.x-et-n.... Couldn't work on this more, sorry.
I'll reassign this back to me as soon as I can work on it again (hopefully tomorrow).
Comment #21
plachComment #22
fagook, I gave this a short test. I was able to get it working in principle, such that I received a close-to working node-view page. However, I must say all that bc-decorator wrappers being passed around and sometimes not makes it rather complex. I fear it requires quite some work to get all tests green - lots of work that eats our time we could use to the real conversion instead - which is lots of work but not difficult to do. Thus my feeling is we should better go for the straight conversion and work together to move on fast. Thoughts?
@Git: Let's use a single sandbox for collaboration so we can easily merge each others changes? I don't really care which one, but we could continue using the entity-api sandbox and branches: http://drupal.org/sandbox/fago/1497344 - you all should already have access?
I pushed field-node (based on plach's sandbox), field-node-base (as 8.x base for diffing) and a field-node-fago branch to the repo. (Always *merge* changes, never re-base)
Also attaching the patch. It makes node_load() return BCEntities while entity_load() stays with EntityNG.
We can avoid this by moving the decorator to the Node class, while the real entity becomes NodeNG. See attached patch.
Comment #24
plachOK, it seems it will be easier to do this if we get these in first...
Comment #25
BerdirTo complete it maybe. But I don't think there is anything that prevents us from working on both things in parallel. My suggestion would be to simply not touch any field related tests yet, there is a lot of code to convert everywhere else in core.
Comment #26
fagoBut problem is that this patch will become rather big, so keeping it updated and fixing merge conflicts will be a significant amount - it was already for comments, for nodes I'd think it's way worse. Thus, once we get started with the full conversion we should try to get it done+in asap.
Comment #27
fagoI recalled I promised to post my regex, so here is the regex I used for comments (with PHPstorm):
It's not perfect, but a big help ;-)
Comment #28
plachI got back to this yesterday night: since in #1810370: Entity Translation API improvements we decided to introduce another decorator to deal with entity translation language, I decided to go back to the original strategy and try to pass the BC decorator around by changing type hints. I'm making some progress, but I have not a new patch ready, I'll try to post one tonight.
Comment #29
moshe weitzman CreditAttribution: moshe weitzman commentedThanks for reviving this, plach. This is a pretty major task for the Web Services effort.
Comment #30
moshe weitzman CreditAttribution: moshe weitzman commentedComment #31
plachOk, I was able to complete a full UI-driven CRUD workflow. Let's see how many test failures we have.
Comment #32
plachOops
Comment #34
plachFixed Forum syntax.
Comment #35
plachComment #36
YesCT CreditAttribution: YesCT commented#31: node-ng-1818556-31.patch queued for re-testing.
Comment #38
plachThis one should have less failures.
Comment #40
plachMore fixes.
Comment #41
plachComment #43
plachMore :)
Comment #45
plachAnd again!
Comment #47
plachLet's try two different approaches.
Comment #49
plachThis one reverts some optimizations to the BC decorator and goes back closer to #38. Not sure what to think about the increasing number of failures.
Comment #50
plachForgot the interdiff: @fago do you think there's something wrong in here that could cause troubles?
Comment #51
plachAnother fix.
Comment #52
fagoI tried that already - it will create troubles with cloning the values when the entity is cloned - check the entity uuid tests. There is a comment somerwhere noting this.
Thus, I'd suggest to not re-work the BC-decorator that way - I already spent too much (lost) time on that.
This should just pass through the return of the method if it has to use it already.
Comment #53
plachOk, I'll revert that part.
Comment #54
plach@fago:
Here is my current BC decorator. I retained definitions cache to speed things up a bit. I don't think this should be a problem, right?
The interdiff is a bit old. I actually restored:
Comment #55
plachA big bunch of fixes.
Comment #57
YesCT CreditAttribution: YesCT commentedthe type does not appear to match.
check these for 80 chars.
I got interrupted looking at this. It's probably too early for me to look at it anyway. :)
Comment #58
fago@plach: Passing definitions by reference should work as long as a cloned entity does not change bundles. It's not optimal, but should work out temporarily.
Comment #59
plachYep, that's what I thought.
Comment #60
jibran#1498674: Refactor node properties to multilingual is critical and postponed on this.
Comment #61
das-peter CreditAttribution: das-peter commentedI think quite some fails are provoked by the
EntityFormControllerNG
the methodbuildEntity()
relies on the information provided by$translation->getPropertyDefinitions();
:Thus if there are no definitions e.g. for the book property used by the book module (has a book group on the node form) these form values are lost.
If I look at
entity_form_submit_build_entity()
following could work as a very sloppy workaround:Comment #62
plachYes, that was the next item on my list :)
Comment #63
das-peter CreditAttribution: das-peter commentedJust out of curiosity here a patch with the mentioned change. The interdiff is quite big because I've merged the latest 8.x.
I'm wondering how the test-fails are affected by this little change.
Comment #65
plachBrilliant!
Comment #66
das-peter CreditAttribution: das-peter commentedWhile continue the work I struggled with this a test failure in the test
Node access on any table
TaxonomyTermReferenceItem::setValue()
has a quite picky test for valid values:It seems like it can happen that the values (name, vid) from
TaxonomyAutocompleteWidget::massageFormValues()
get stuck, and thus are preset in e.g.EntityFormControllerNG::buildEntity()
.Calling there
$entity->getTranslation($this->getFormLangcode($form_state), FALSE)
drills down toTaxonomyTermReferenceItem::setValue()
, but there was not even a chance to clean up the raw form values which then throws the error.I guess this could be avoided if there was another way than initializing allllll promperties to figure out which translation languages are available.
For now I've just shut up the picke values test by adding name, vid to the cleanup process in
TaxonomyTermReferenceItem::setValue()
.Addition:
InvalidArgumentException
is still thrown - but now because a strange array is provied as$value
toTaxonomyTermReferenceItem::setValue()
- it looks like a term entity converted to an array. And I've no clue where it comes from and why the heck it is set.I try to track that down but unfortunately the format of the passed array throws an internal exception in phpStorm when debugging the code...
Any ideas / hints very welcome :)
Comment #67
fagook, problem is field API piggy-backing values in $item during auto-creation.
I tried fixing it properly by having the new entity in $item['entity'] see attached patch. But it won't work out as a) in bc mode the entity won't be accessible by default and b) it looks like rendering also relies on $item['taxonomy_term'].
So we probably should do an interim fix by declaring the added stuff for the taxonomy reference item also. OR we bet on #1847596: Remove Taxonomy term reference field in favor of Entity reference to go in first and it there. The same problem happens to be there for the entity reference item.
Comment #68
fagook, I had another idea: We could just allow piggy-backing any data on $items for now. This should help us moving on with the conversion faster, we can remove/refactor it once conversion is complete. Then we actually can rely on our new API to be there and use it...
Attached is a totally untested patch which only works if I did not miss anything. But at least it should show the idea. The BC-decorator uses getValue() so it should pick the extra stuff up by having it there.
Comment #69
fagooh and the missing patch from #67 showing the idea on how to properly deal with to-be created entities
Comment #71
das-peter CreditAttribution: das-peter commented@fago: Thank you very much for the patches. I just had to adjust some minor things. Now the
Node access on any table
tests pass :)The new Patch contains additions of fago and some other adjustments. Some more tests should pass now.
Latest changes are pushed into fagos sandbox (http://drupalcode.org/sandbox/fago/1497344.git/shortlog/refs/heads/field...)
Comment #73
sunAny chance for doing all of those type-hint/phpDoc changes in a separate issue?
Comment #74
tim.plunkettAgreed, changes like these are completely out of scope.
Comment #75
das-peter CreditAttribution: das-peter commentedAs far as I know not all of these changes are just cosmetic but necessary as we deal with a decorated entity. And as
EntityBCDecorator
is a wrapper that implementsEntityInterface
and doesn't extend some specific entity class using theEntityInterface
seems to me the only viable way to do the type hinting.Comment #76
das-peter CreditAttribution: das-peter commentedFixed conditions
Defined default values weren't assigned in
field_populate_default_values()
becauseEntityBCDecorator::__get()
initializes the fields data array with NULL values. Thus the check inEntityBCDecorator::__isset()
always returned TRUE. I've changed the check inEntityBCDecorator::__isset()
to ensure there is a non-NULL value in the data array. It's horrible code but the only solution I can think of - as far as I remember initializing the values was essential and thus I didn't want to change there anything.Other ideas very welcome!
And as seen in the interdiff I made a
Node
toEntityInterface
just in a function doc block. Not sure if really necessary, but I personally prefer to have consistency between function declaration and its doc block over a smaller diff.Comment #78
das-peter CreditAttribution: das-peter commentedThe test-Failure in
Node creation
is related to this #1872690: Exception: Serialization of 'Closure' is not allowed in serialize. Dealing with the expected exception leads to the php errorSerialization of 'Closure' is not allowed
.I think this should be handled in the related issue.
Comment #79
das-peter CreditAttribution: das-peter commentedTest:
Node save
Handling of the properties
enforceIsNew
,newRevision
andisDefaultRevision
moved intoEntityNG
since these properties are part ofEntity
. I don't see why these properties should be handled byNode
itself. Other opinions?Fixed handling of the
original
property inEntityBCDecorator::__get()
.Comment #81
das-peter CreditAttribution: das-peter commentedTest:
Node revisions all
Added handling of the
log
property.Changed the way how
NodeRevisionsAllTestCase::setUp()
creates node revisions (usingget_object_vars()
isn't viable anymore).Added methods
isNewRevision()
andisDefaultRevision()
toEntityNG
(use->value
to access value).Hmm, looks like the overall amount of test-failures increased horribly :|
Comment #83
BerdirThat doesn't make sense?
Both are just normal, protected class properties which are not persisted and only accessed through methods. There is no reason to define them as entity fields?
And if it would be, then the isDefaultRevision assignment would be wrong.
Comment #84
das-peter CreditAttribution: das-peter commented@Berdir You're absolutely right. Atm. I'm fighting with getting the revisions stuff done. The changes worked for the decorated node entity but not for all other NG entities (e.g. comments). However, if I revert the stuff I can't manage to get the test
Node revisions all
passing.Unfortunately I wasn't able yet to figure out what's going wrong and where :|
Comment #85
das-peter CreditAttribution: das-peter commentedI think I found the issue:
EntityNG::__construct()
doesn't set values for the fixed class properties.DatabaseStorageControllerNG::mapFromStorageRecords()
prepares all values to be used for defined fields, thusEntityNG::__construct()
misses them.I've added a check if a value belongs to an existing class property and if it does the value is set to the class property. This shouldn't harm the properties for which a definition exists and are class properties at the same time - these will be reset with the later call to
EntityNG::init()
.The interdiff contains several changes I'm not so sure about:
EntityBCDecorator::__get()
: Special handling of the "magic" flagrevision
.NodeSaveTest::testImport()
: Explicit usage of the methodEntity::enforceIsNew()
instead using a value in the node data array. Has there to be support for theenforceIsNew
flag in the data array?The problem with the flag in the data array is that
DatabaseStorageControllerNG::create()
can't access the protected entity class properties. A solution could be to check if there's a method with the name of the data array item and call it with the value as parameter (would work for enforceIsNew). But I don't like to introduce even more magic ;)Comment #87
das-peter CreditAttribution: das-peter commentedVery small change in
FileFieldTestBase
to fix the creation of test nodes. Should affect quite some other tests.I hope it reduces the failures even more, but there's the risk that the field tests now run as the should and fail somewhere else ;)
Comment #89
das-peter CreditAttribution: das-peter commentedAnother tiny change to fix an issue that nearly drove me nuts ;)
We need to check if a field is not empty before pre-setting its data-structure to the values in
EntityBCDecorator::__get()
.Otherwise we can end up with ghost values like:
Stuff like this can cause strange errors - e.g.
array_flip(): Can only flip STRING and INTEGER values
inDatabaseStorageController::load()
. This function tries to flip the passed id's and has trouble when one of them is NULL.Comment #90
das-peter CreditAttribution: das-peter commentedSome minor changes:
DatabaseStorageControllerNG::__construct()
tests now if the bundle key is set before accessing it in the passed in array.NodeStorageController
defines now the data typestring_field
insteadtext_field
to only rely on core data types. Is there a string length limit in that type?PluginTestBase
&MockBlockManager
use nowDrupal\Core\Entity\EntityBCDecorator
insteadDrupal\node\Plugin\Core\Entity\Node
as node class.DefaultViewsTest
now properly creates a node type before testing. (Otherwise the body field is missing and threat as property)With this changes some more tests should pass :)
Comment #92
das-peter CreditAttribution: das-peter commentedfield_ui_field_edit_form()
calls_field_create_entity_from_ids()
which triggers creating an entity object.Unfortunately creating an entity triggers
EntityNG::getPropertyDefinition()
which ends up inTypedDataManager::create()
, where the method tries to deal with the not yet created field. This fails because the definition of the not created yet field misses the 'type' in$definition
.I don't think this is an
EntityBCDecorator
but anEntityNG
specific issue.Ideas how we can fix this?
Entity Translation
:Had to map 'value' to 'target_id' in
EntityReferenceItem::__set()
andEntityReferenceItem::__get()
. There was already a mapping inEntityReferenceItem::get()
but that wasn't sufficient.Taxonomy term functions and forms
In
taxonomy.module
'taxonomy_term' instead 'entity' was used - causingTaxonomyTermReferenceItem
to throw an exception.Another occurence of
array_flip(): Can only flip STRING and INTEGER values
caused bynode_preview()
- where the terms are expected to be listed, even if not yet created. Solved it by adding an other check to not only skip 'autocreate' but also FALSE tids intaxonomy_field_formatter_prepare_view()
.Comment #93
BerdirUh, that sounds strange. That *very very ugly* helper function was added to be able to create a fake entity object when deleting fields as the entity might not exist anymore at that point. Will look into it.
Comment #95
das-peter CreditAttribution: das-peter commentedFile field widget test
More trouble with
EntityBCDecorator::__isset()
an empty array has to be considered as set, even if an array with only NULL values has to be considered as not set.Otherwise a cleared field isn't stored back and its data persist.
Forum blocks
Since
LANGUAGE_DEFAULT
is mapped to an actual language we've to handle the languages different inforum_field_storage_pre_insert()
to avoid inserting duplicates intoforum_index
. Not sure if the approach I've chosen is valid. It's already NG style, but that shouldn't hurt too much, right?Normalize/Denormalize Test
Fixed an error in
EntityReferenceItem
.Entity UUIDs
Revision ID's weren't properly tested in
EntityUUIDTest::assertCRUD()
.Entity Field API
Looking at the code in
EntityFieldTest::testEntityConstraintValidation()
I would assumeEntityWrapper::setValue($node)
should throw an exception sinceEntityWrapper::definition['constraints']['EntityType']
definesentity_test
but that's not the case.My validation knowledge is to limited to make quick progress, any hints?
Comment #97
das-peter CreditAttribution: das-peter commentedMerged with latest 8.x and field-node-berdir (http://drupalcode.org/sandbox/fago/1497344.git/commit/96612319aeef306f82...)
SearchCommentCountToggleTest
, had invalid node params.Node translation UI
Failures because
Node
is now aEntityBCDecorator
. WhileEntityNG
is handled,EntityBCDecorator
wasn't.I've added now some
$entity = $entity->getOriginalEntity();
to unwrap the entity and use theEntityNG
-Handling.Should we create dedicated test entity types? A non-NG and an
EntityBCDecorator
one?And it seems like there was some tangling with the save button naming - or did I miss a hidden status change that caused that tangling?
I've made
NodeTranslationUITest::getFormSubmitAction()
status sensitive now.Node translation UI
Failures because
Node
is now aEntityBCDecorator
. WhileEntityNG
is handled,EntityBCDecorator
wasn't.I've added now some
$entity = $entity->getOriginalEntity();
to unwrap the entity and use theEntityNG
-Handling.Should we create dedicated test entity types? A non-NG and an
EntityBCDecorator
one?And it seems like there was some tangling with the save button naming - or did I miss a hidden status change that caused that tangling?
I've made
NodeTranslationUITest::getFormSubmitAction()
status sensitive now.The attached interdiff doesn't reflect the changes from the merges! (Otherwise it would be huuuuge)
Comment #99
plach#97: node-ng-1818556-97.patch queued for re-testing.
Comment #101
das-peter CreditAttribution: das-peter commented#97: node-ng-1818556-97.patch queued for re-testing.
Comment #103
das-peter CreditAttribution: das-peter commentedI've no clue why the heck it's failing.
That problem nor the class
Insert
has a meaning to me. The patch doesn't event contain the wordInsert
as class declaration :|Question
Test
Multilingual fields
Is this test-case suitable at all? It was basically a test to check if fields of non-NG entity types can be translated properly, right?
Shall we introduce a dedicated test entity to test this?
Comment #104
das-peter CreditAttribution: das-peter commentedFixes by berdir, thanks!
Comment #106
das-peter CreditAttribution: das-peter commentedLast patch before I go to sleep:
Taxonomy term index
Looks like calling
EntityBCDecorator::__unset()
didn't make the property act like it isn't set but as it is set to empty. Using the function should cause the property to be ignored on save and not removing the data.This isset / unset thing is really tricky :| But maybe I just missed something :D
RDFa markup
rdf_field_attach_view_alter()
used$item['taxonomy_term']
instead$item['entity']
. Changed the usage there and in some other locations.Not sure yet if
TaxonomyAutocompleteWidget::formElement()
needs to be adjusted too.Field: Field handler
An empty field was / is passed to
field_view_field()
not sure if it shouldn't passed at all or if, for the sake of defensive coding, the function itself should check the field has values in the requested language.I went for the later now. Other opinions?
Node: User has revision Filter
Explicitly setting
revision_uid
failed. Adjusted check inNodeStorageController::preSaveRevision()
.Comment #108
fagoTrue, it should be set to NULL though. This is the value of the untranslated field only, so no reason to key by langcode?
This does not look right to me. Should field_language() make sure to return a langcode that works? Looks like there might be a problem with it and BC?
That does not make sense to me, why should an array with only NULL values should be not set? Because of the default values we have in entity-ng here? True, with EntityNG we have those values to be set by default, but they are set to empty by default. Does that cause problems?
Comment #109
fagoAlso #106 contains quite some unrelated hunks - please check the whole patch.
Comment #110
fagohm, I think the better fix would be to fix field API to *always* set default values on hook_entity_create(). There is no reason not to and it's the only situation where we need to set defaults - so we do not need an isset() checks here any more?
Comment #111
fagoThus, having isset($entity->field) being TRUE by default is not nice, but a good performance improvement. Still the field is correctly "empty", so there shouldn't be any harm in it.
The only possible problem I could see is during editing if widget show an extra empty item-form for the empty item, e.g. it shows two empty item forms instead of one. I guess we could fix that by removing empty items when starting editing (=when building the entity form the first time). This should be generally possible within the entity form controllers. Again not very nice, but imho a reasonable small work-a-round for better performance.
Comment #112
das-peter CreditAttribution: das-peter commentedIsset requirements / problems as far as I understand:
EntityBCDecorator::__get()
to be able to return a reference and for performance reasons.EntityBCDecorator::__get()
on a non yet set property will change the return ofEntityBCDecorator::__isset()
for that property - that has to be avoided.EntityBCDecorator::__unset()
doesn't change the return ofEntityBCDecorator::__isset()
of non-fields to FALSE - that should be changed.My idea was to add a tracking, for empty initialized properties, that can be used in
EntityBCDecorator::__isset()
to return the appropriate value.Tracking is done in a protected class variable which is used in the methods
__get()
,__set()
,set()
,__isset()
,__unset()
ofEntityBCDecorator
.EntityBCDecorator::__unset()
: I'm a bit confused there, I'm not sure if the behaviour between properties and fields really has to be different as I did implement this now.However, local testing looks not to bad thus let's see what the test-bot says.
Comment #114
das-peter CreditAttribution: das-peter commentedOptions field UI
The test fails because the
EntityBCDecorator
doesn't recognize fields of this type.This happens because
field_entity_field_info()
doesn't return a field definition if there's no field item class defined. ThusDatabaseStorageController::getFieldDefinitions()
never knows about fields of the list type.We need to push:
http://drupal.org/node/1732730
Comment #115
das-peter CreditAttribution: das-peter commentedTranslation functionality
translation_test.module
implementstranslation_test_node_insert()
that saves the $node object.Of course
drupal_write_record()
isn't the thing to use anymore.However, what's the reason we need to save the node again in this hook? This feels somehow odd, especially singe all tests pass as well when I don't call
$node->save()
.Comment #116
das-peter CreditAttribution: das-peter commentedEntity Test translation UI
andComment translation UI
"Missing"
$entity->updateOriginalValues();
after making the changes inEntityTranslationControllerNG::removeTranslation()
.REST: Update resource
"Missing"
$entity->updateOriginalValues();
after making the changes inEntityResource::patch()
.What?!? I'm a bit confused by the need to call
EntityNG::updateOriginalValues()
and / or wasn't this already part of the storage controller somewhen?Comment #118
das-peter CreditAttribution: das-peter commentedRe-roll
Comment #120
das-peter CreditAttribution: das-peter commentedEntityReferenceAutoCreateTest
andEntityReferenceAdminTest
.Although that ids function was ugly, it had nothing to do with that.
Fields for entities without bundles (although the check there is wrong, will fail ugly if an entity has bundles and one bundle == entity_type) are in 'definitions', fields for entities with bundles are in 'optional'.
entity_reference_entity_field_info()
always added the settings to definitions which worked fine for entity_test but not node.Then definitions contained a partial info and at the bottom, where the optional fields are added with
+=
it didn't overwrite the existing key.I've changed the entity reference hook to an alter hook and checked where I need to add it, which is ugly, but the whole function should go anyway, we need a solution to add additional field data/settings right in
field_entity_field_info()
EntityReferenceSelectionSortTest
.Fixed creation of test content.
Entity Field API
.The entity wrapper didn't update
EntityWrapper::$entityType
when a non new entity was set inEntityWrapper::setValue()
.Thus setting a node still validated as 'entity_test' entity.
What's left:
Comment #121
BerdirOpened the following issues for things that I noticed when working on the entity reference fix:
- #1919468: EntityManager::getAllBundleInfo() adds default entity_type_id bundle for entity types with bundles
- #1919466: Support for adding field settings in field_entity_field_info()
Comment #123
das-peter CreditAttribution: das-peter commentedNow this definitely needs reviews :)
Open points / questions:
Multilingual fields
(NodeFieldMultilingualTestCase
).Details: #103
Node create
: depends on #1872690: Exception: Serialization of 'Closure' is not allowed in serializeOptionsFieldUITest
: depends on #1839064: Implement the new entity field API for the list field typeEntity Test translation UI
,Comment translation UI
andREST: Update resource
Details: #116
Translation functionality
Details: #115
EntityBCDecorator::__isset()
Details: #114
Field: Field handler
Details: #106
NodeSaveTest::testImport()
Details: #85
Comment #124
Berdir#120: node-ng-1818556-120.patch queued for re-testing.
Comment #126
das-peter CreditAttribution: das-peter commentedOh, even less fails than expected! :)
I expected
Node create
to fail too.And the test
Multilingual fields
(NodeFieldMultilingualTestCase
) is maybe even obsolete.So, even if not green yet this looks promising.
Comment #127
plachI'll have a look to the last test tonight, if no one beats me to it.
Awesome work, guys!
Comment #128
YesCT CreditAttribution: YesCT commentedwhy do these, and a bunch of other function definitions not just say (EntityInterface $...) ?
Are they missing a use statement?
Comment #129
YesCT CreditAttribution: YesCT commentedSome of these comments are temporary, so I dont know how picky we want to be with them...
should be:
@param array &$definitions
in an entity
// necessary as EntityNG always keys default language values with
extra newline. remove.
change function declaration to also have type EntityInterface
no type hints?
Comment #130
YesCT CreditAttribution: YesCT commentedre #128 @berdir says hooks always use fully qualified, so they can be cut and paste.
See "API documentation (in .api.php files) should use full class names. Note that if a class is used more than once in multiple hook signatures, it must still be "use"ed, and then only the short names of the class should be used in the function." in namespaces standards http://drupal.org/node/1353118
Comment #131
YesCT CreditAttribution: YesCT commentedthese seem to be missing an update to their respective function declarations
use fully qualified \Drupal...
re-wrap to 80 chars (clearly this is optional)
Comment #132
das-peter CreditAttribution: das-peter commented@YesCT: Thank you very much for the feedback. Patch updated.
Comment #134
andypostRelated #1368610: It is confusing why creating a node requires users to have permission to "view published content"
Comment #135
plachThis should fix the last failures (!)
As @das-peter pointed out the Multilingual fields test is obsoleted by the Entity Field API, which always uses the
LANGUAGE_DEFAULT
language code for the original values. Instead of further messing with the BC decorator, I just converted the test to NG, which shows how much simpler dealing with field language has become :)Comment #136
plachComment #137
BerdirAwesome, this is green.
Will try to run my Convert test_entity to entity_test issue with all the BC decorator changes as that one has tons of field API tests that should help verify that the changes are correct and hopefully fix some of my tests too.
Review below but this looks quite good already IMHO. I think we need a plan on how we want to get rid of the temporary stuff, see comment below and then this should get in ASAP.
I think we should find a way to get rid of this, possibly override the form controller to map $form_state['values']['revision'] directly to setNewRevision(). This already happens a bit later (during submit/save).
The Custom block entity should already handle this, as it is NG and supports revisions.
Do you know which values are added additionally?
An alternative of this might be to simply define them explicitly, that worked fine with the additional_key in field_test. However, it also affects some tests that compare whole arrays.
Hm, the same thing should allow me to fix DateTimeItem.
I still think there should be standardized way to return the "storage value".
Interesting, that is exactly the fix that I suggest in the file field type issue to support things like prepare_view() load improvements. So we should look into making this the default behavior imho, but tha can happen in a follow-up issue.
Wondering if we still need that @todo, there's not that much left that a helper function could do.
The @var definitions should probably be updated, see Comment.php.
We should probably write up an issue summary and document the approach here, overview of the changes and what is temporary and what not (And a plan to remove the temporary things, not yet sure what's the best approach there).
Otherwise, all these type hint changes might be confusing for reviewers.
I am not sure why this was langcode specific as changed is not yet translatable.
This might be the only case where we actually created nodes with multiple initial languages.
The change isn't very nice but I think such cases are very rare and don't need to be supported directly.
Can we use entity_create() for the node as well?
This will conflict with my DUBT patch for the entity tests and I would like to keep that conflict as small as possible. Maybe that should be in a different test class then we can remove the whole thing and it won't conflict. Isn't there already a test class for the BC decorator?
Are we sure this change doesn't break not yet converted entity types? What happens if you add a taxonomy field to users and try autocreate?
Same with the other taxonomy_term/entity changes.
Comment #138
amateescu CreditAttribution: amateescu commentedWe shouldn't care about that, #1847596: Remove Taxonomy term reference field in favor of Entity reference is taking care of it.
Comment #139
Berdir@amateescu: That's not what I meant.
It' only temporarly broken anyway, it would work again once all entity types are converted, whether it is replaced by ER or not.
But until that happened, or it is replaced with ER, it would be broken.
Comment #140
longwave+ protected $intializedEmpty = array();
Should be $initializedEmpty?
Comment #141
fagoAmazing to see this green!
Well, I think it would not hurt to support writing arbitrary stuff into $entity->field[0]->custom ? It could be still ignored by all the typed-data API and e.g. only returned by $this->getValue() and support by $this->setValue() as the plain values of a field item while ->getPropertyValues() would be "clean" ?
For that, we could re-factor the field-item class with #1869562: Avoid instantiating EntityNG field value objects by default and have it all in the protected $values.
Yes, let's take care of that in #1868004: Improve the TypedData API usage of EntityNG.
Is that change necessary? If the entity has a bundle it's required to be passed into entity_create() - so throwing a notice is the least it should do then... an exception would be even better.
Can we add a comment on why we need that? (Actually I still miss the point here ;) Why does setting the field to NULL in unset not cut it?
I don't see how the comments relates to the code below?
This assumes $item is an array what the old code did not?
I'm looking forward to cleaning this up by using $item->entity always - even in case of auto-creation ;-)
Comment #142
vijaycs85Fixed:
Removing todo mentioned in #137
updated.
in #141 updated.
Comment #143
plachThe attached patch should address the pending issues form the reviews above. I only omitted the
$initializedEmpty
stuff as @das-peter can provide better feedback on that. Since the patch reverts some changes that were likely introduced to fix some tests I expect some failures, which I'll fix asap.Comment #144
plachSome notes:
I think a possible approach to complete the conversion might be to start converting every module in its own issue. This would allow us to incrementally provide all the node field definitions and thus get rid of the BC decorator instances in the related places. Also #1498674: Refactor node properties to multilingual might help with that.
Yes, I totally agree. When working on the early patches here I often found myself wishing for an automated way to map from
->value
to->target_id
and viceversa. Probably a'default property'
key in the field definition would be enough for this.This was probably paving the way for multilingual support for search code. Anyway this line will just become
$node->changed->value
once we convert that part and we are done with #1810370: Entity Translation API improvements.Not sure about this:
field_entity_create()
is already supporting this scenario, thus I thinkentity_create()
should too. In another issue obviously.Manually tested this use case and everything worked well.
Comment #144.0
plachUpdated issue summary.
Comment #144.1
plachUpdated issue summary.
Comment #145
plachUpdated the issue summary as suggested in #137.
Comment #147
plachFixed the exception.
Comment #148
das-peter CreditAttribution: das-peter commentedJust adjusted comment in EntityBCDecorator::__unset() to actually make sense in that context.
Comment #149
fagohm, this seems to fail to return a reference on the proper value. That means a $node->property['foo'] = '3'; would fail?
Comment #150
das-peter CreditAttribution: das-peter commentedThe issue with
EntityBCDecorator::__isset()
:In D7 an entity was often an instance of
stdClass
- thus you were able to simply add properties as you liked e.g.$node->foo = 'bar'
.The entityNG approach in D8 now needs definition for all properties, thus the
EntityBCDecorator
had to workaround this to be really backward compatible.So if you access
EntityBCDecorator->foo
which is handled byEntityBCDecorator::__get()
it tries to find the appropriate definition and if there's none, it initializes an internal data-structure forfoo
to be able to work with references.The initialization looks like this
$this->decorated->values['foo'][LANGUAGE_DEFAULT][0]['value'] = NULL;
.Thus
EntityBCDecorator::__get()
returnsarray('value' => NULL)
for a basically non-existing property. (Don't ask me why, we had some issues there and that was a solution ;) )Now
EntityBCDecorator::__isset()
does nothing else as callingEntityBCDecorator::__get()
and then run anisset()
on the returned value. But wait(!), this will initialize non existing properties and the return value will bearray('value' => NULL)
! Which means theisset()
check inEntityBCDecorator::__isset()
will always evaluate to TRUE for non-existing properties!One could argue why not simply check properties with
isset($value[0]['value'])
then?Well, maybe
EntityBCDecorator->foo
will containarray('snafu' => 'bar')
, soisset($value[0]['value'])
would then return FALSE even if a proper value is set.Easiest way, I could think of, to work around this was to track empty initialized or explicitly unset properties and check the tracking in
EntityBCDecorator::__isset()
.Comment #151
das-peter CreditAttribution: das-peter commentedForget the comment above (will correct it later).
I found some other related stuff and will re-roll the whole thing again.
To do so here's a patch with some stuff reverted, and some changes that hopefully keep the patch green.
No interdiff yet, I just need the results of the test-bot to see where I can reproduce errors - my machine is to busy to run all the tests ;)
Comment #152
das-peter CreditAttribution: das-peter commentedNice, the proof of concept patch is green :)
Slightly adjusted patch attached.
I'm not sure how everything is related, looks like a lucky punch ;P, but I fixed the missing reference for the cases in which a defined property was initialized empty.
Further I ensured it worked not just with properties that have 'value' as key for the data.
Open question is if properties with more than just one data key will work as well.
As far as I know there's a discussion ongoing about define the "default key" to map 'value' always to that.
Comment #154
das-peter CreditAttribution: das-peter commented#152: node-ng-1818556-152.patch queued for re-testing.
Comment #156
das-peter CreditAttribution: das-peter commented#152: node-ng-1818556-152.patch queued for re-testing.
Comment #158
BerdirThis needs to use $this->moduleEnable() now.
Comment #159
das-peter CreditAttribution: das-peter commented@Berdir: Thanks for the hint!
Patch re-rolled against latest 8.x and made the changes mentioned by berdir in #158.
Comment #161
das-peter CreditAttribution: das-peter commented*blargh* more adjustments needed to get test (
EntityFieldTest::testBCDecorator()
running. Now the setup for the test should work.Test failed locally too, now it passes.
Comment #163
BerdirI really think we should move the BC test into a separate class. This adds the schema installation to all other tests of that huge test class too, making it unecessarly slower.
Comment #164
YesCT CreditAttribution: YesCT commented@Berdir That sounds like a follow-up to me. Are you ok with that? I can open it.
Comment #165
BerdirAs discussed, the test failed and needs to be fixed anyway, moving to a different file is not that hard.
Also, I'm not suggesting to move it because of those additional dependencies. Those are just another sign why that test doesn't belong in that test class. It's a test for a different API, an API that is only temporary. Being able to just remove a file is easier than looking for test methods in an already large test class.
Comment #166
das-peter CreditAttribution: das-peter commentedI'm in favor of a separate class too. Thus here's an updated patch.
I've added some more assertions for the
EntityBCDecoratorTest
as well.Unfortunately I don't expect this to become green, even if it does on my local system.
The same happened with the patch in #161.
(Because of Windows?!? Normally it's the other way around ;) )
Would be great if someone could just run the
Entity Field API
and the newEntity Backward Compatibility Decorator
tests to see if one fails.Comment #167
BerdirThanks!
use statements are unused, config isn't needed it seems and enabling the modules once is enough :)
Comment #168
das-peter CreditAttribution: das-peter commentedCleanup according to #167.
Comment #170
das-peter CreditAttribution: das-peter commentedI really don't get it why the test doesn't fail locally. I've added the schema
role_permission
to the setup method now. Hope this helps.Comment #171
BerdirI believe this is one of the early changes that I made before the BC decorator really worked, is this still necessary?
Interesting that this works here but $node->get('changed, $langcode) in search didn't. Maybe this is never called with a non-default langcode? Because according to the change I had to make there, this explodes if called for a properties that is not actually translatable (not sure if it should or just fallback to the language neutral one)
Forgot to mention that before. Should be "Contains \Drupal\..."
That @todo is kinda pointless, as this is true for all getOriginalEntity() and getBCEntity() calls.
But this looks really good to me. Maybe @fago can have another look through the BC Decorator stuff because I don't feel comfortable RTBC'ing that stuff.
Comment #172
plachThat code should be
$node->get('changed, $langcode)->value
to work the same way I guess.Comment #173
fagoI had a look at the patch and in particular the changes at the BC-decorator. BC-decorator changes look great to me, good job!
This part does not make much sense to me. It's in the NG-form controller, so why should it get a BC decorator? It should be able to work with the NG-entity.
Can we comment on why and when this is necessary? I'd assume it's because of the storage controller assuming it can go with ->value.
That actually confuses me a bit, as EntityReferenceItem already defines it as 'target_id', but the NG-storage works with "value". Has its storage been broken without breaking tests?
This creates potentially stale data we need to take care of. But that we can off-load to #1868004: Improve the TypedData API usage of EntityNG which is about to re-vamp this logic.
Comment #174
das-peter CreditAttribution: das-peter commentedActually I ran all comment tests with a conditional (
is_object($values['nid'])
) breakpoint set and it never was hit.So I assume we could remove that - however I wasn't bold enough yet to do so.
I've checked this again. Was really confusing thus I changed the code now, I hope it's more obvious what's going on there now - even if it looks horrible... ;)
What I'm a bit concerned about is the fact that we still have to deal with node objects based on
stdClass
there.Changed.
@todos removed.
Well
NodeFormController::buildEntity()
passes over toEntityFormControllerNG::buildEntity()
which uses$this->getEntity()
and this is handled byNodeFormController::getEntity()
which returns an instance ofEntityBCDecorator
.As
EntityBCDecorator
andEntityNG
are closely related I thought it would be better to keep the code inEntityFormControllerNG
instead copying it intoNodeFormController
.However, we could build something like an
EntityBCDecoratorFormController
.I've just ran the EntityReference tests with a breakpoint there, no hits. So I removed this mapping and still no EntityReference test-fails, let's see if another test fails.
Anything to do here?
Comment #176
das-peter CreditAttribution: das-peter commentedI've found the reason why there was:
DatabaseStorageControllerNG::attachPropertyData()
always usedvalue
to set the property data.I've changed this now to base on the field property definition, however the current approach is just sophisticated guessing and nothing more. We really should fix that.
@fago As far as I remember there's already an issue (adjust property definition) for that, right?
Comment #177
fago> @fago As far as I remember there's already an issue (adjust property definition) for that, right?
I don't think we have already an issue for that. Feel free to open one!
Yes, it does not make much sense to me to specifically pass an $bc_entity into the NG controller - the NG controller is supposed to work with NG-entities... So if we want/need to work with a bc entity here, couldn't we just stay with the non-NG EntityFormController?
Comment #178
das-peter CreditAttribution: das-peter commentedI found the issue where I stumbled over this problem earlier: #1847588-30: Implement the new entity field API for the Entity-Reference field type
So basically we need a new issue for "Field API: Support a proper naming pattern in the schema columns", right?
I don't think we can / should rely on the
EntityFormController
because there wasEntityFormControllerNG::getEntity()
just to use the BC decorator. I've now created a dedicated form controller. I think that's not the worst solution for now.Comment #179
Berdir#178: node-ng-1818556-178.patch queued for re-testing.
Comment #180
fagoI think it's an issue of improving the default NG storagecontroller only, not field API. So, maybe something like "Implement a sane storage default mapping", e.g. map column names like user__target_id to $entity->user->target_id and vice versa.
Yes, but that's the whole point. Why should the NG controller use a BC decorator if it's supposed to work with NG API? Cannoted we stay with the regular form controller and use it with BC - I mean that's the whole idea of a BC-entity: be able to use it with the old code.
Comment #181
das-peter CreditAttribution: das-peter commentedIssues for the "
DatabaseStorageControllerNG
stuff" created: #1936382: Implement a sane storage default mappingEntityFormController
stuff:One problem is that
EntityFormController::buildEntity()
usesentity_form_submit_build_entity()
which makes use of$entit->set($property_name, $value)
and that tiggers anInvalidArgumentException
if an unknown field is set.We could fix this using something like following code in
EntityBCDecorator::set()
:Locally all node tests pass with this adjustment.
Is this approach ok? If so, shall I integrate something similar for
EntityBCDecorator::get()
?As soon as I know how to proceed I'll create another patch.
Comment #182
das-peter CreditAttribution: das-peter commentedI've decided to give the in #181 mentioned approach a try. Here comes the patch.
Comment #183
fagoGood catch! So we make set() of the BC controller acting as previously, i.e. accepting everything. So that was the problem of the BC-entity!
Maybe let's add a comment on what happens here, so that it's clear we make set() work with any $property_name given.
Interesting :)
Comment #184
das-peter CreditAttribution: das-peter commentedComments added, hope that single line is sufficient ;)
Ouch, forgot to remove the debugging artifact while creating the patches. Luckily this never made it into the pushed code.
Comment #185
YesCT CreditAttribution: YesCT commented@das-peter I didn't see the debugging stuff in the interdiff. Can you point it out to me? I'm curious to see.
Comment #186
YesCT CreditAttribution: YesCT commentedAh, @fago pointed it out in #183 It's not in the interdiff because this issue is using a sandbox so the interdiffs are not strictly between the patches posted here. (Also, the xdebug irc bot factoid has some info I'll look at in more detail.)
Comment #187
fagoI had another look at the whole patch and I think it's ready to move on (given it comes back green). Thoughts?
Comment #188
plachSounds good :)
Comment #189
das-peter CreditAttribution: das-peter commentedSame here, I think that looks quite good.
Comment #190
webchickPatch is green. Team Europe is offline. Marking RTBC, and will leave it for a bit, but try and commit it tonight. EXCITING!!
Comment #191
webchickComment #192
webchickDarn. As much as I want to commit this, I don't think I can do so until we have some data on how this affects performance. Tagging accordingly.
While we're waiting, some feedback:
That's really unfortunate. :( I understand that it gets us more flexibility, but at the cost of DX. :(
On IRC, msonnabaum suggested introducing a NodeInterface here instead, so we keep the flexibility, but also explain to people what it is they're dealing with. I don't think it's necessarily worth holding this issue up on that, but OTOH it would make find/replace much easier if we did it before we lose that granular information.
This, however, looks really bizarre to me. We're losing a LOT of data about expected input this way.
Hunks like this, on the other hand, are very nice. :)
Interesting. It's nice to have this, although hopefully we can delete it soon. :D
Comment #193
BerdirWill try to do some profiling today.
Some answers to your feedback:
- I know why @msonnabaum suggested a NodeInterface but that doesn't help here. The reason we are doing it *here* is because of the EntityBCDecorator that we are working with that doesn't extend from Node and can't implement NodeInterface. This could be reverted after we replaced the BC Decorator. However, we might still need this for translations, see the discussion in #1391694-53: Use type-hinting for entity-parameters and below.
- The reason for the property change is that $node->status is now a class and has to be accessed as $node->status->value (after the BC decorator is removed). The only reason we even document the properties in the Node class is for autocomplete support in the IDE. As you see in the weird hack below in init(), we're then unsetting them because we don't actually use them but are going through __get()/__set(). If we switch to an interface (doesn't matter if it's EntityInterface or NodeInterface unfortunately because interfaces can't have properties) that is pretty much useless and we could just as well remove it. If we can keep it, maybe there is a way to use the actual class there, e.g. IntegerItem, StringItem, FileItem. Note that you can always use dpm($node->getPropertyDefinitions()) to learn about what properties an entity has. The advantage there is that it contains also dynamic properties, e.g. fields and stuff added by other modules.
- About the test, huh, didn't we change that to "Contains \Drupal\..."? :)
Comment #194
Berdir#184: node-ng-1818556-184.patch queued for re-testing.
Comment #195
BerdirOk, did some tests with the frontpage and 10 nodes.
It is 25% slower right now for those 10 nodes. A considerable amount of that is the BC decorator that has tons of __get() and language() calls on EntityNG. I think it's therefore not that useful to do a lot of profiling now, we need to convert everything, throw out BC decorator and then do some serious profiling and see what we can optimize. It's also not really useful to try and optimize the BC decorator IMHO. We just need to get rid of it asap.
Also checked with 20 nodes and it gets slower but I think that is because the generic part makes up a smaller percentage of the whole page request.
Comment #196
das-peter CreditAttribution: das-peter commentedFixed doc block:
Comment #197
fagoSure, all the usage of the BC-decorator slows down things definitely as it is decorating every get/set call. That needs to go away before release anyway though. Then, there is a remaining performance impact which we need to work on - as discussed at the comments conversion. The follow-ups here are:
#1869562: Avoid instantiating EntityNG field value objects by default
#1869574: Support single valued Entity fields
I agree with berdir that once we've removed the BC-decorator we can concentrate on performance and optimize things in a second step. However, from the comment performance optimization we already know that we want/need to do #1869562: Avoid instantiating EntityNG field value objects by default, so that can be already worked on. In fact, it's the next issue on my todo-list as it involves some changes we want to earlier rather than later.
Comment #198
fagoI agree with that, however it would have to be StringItem[] or so. Problem here is that we do not have a coding standard for denoting an "array" of classed objects yet, so we need to sort that out first.
Comment #199
fagoChanges look good and I think we've got all questions addressed, so setting back to RTBC for re-consideration.
Comment #200
webchickOk thanks for checking. 25% is :( :( :( However, Berdir pointed out that in our big soul-sucking critical performance issue at #1744302: [meta] Resolve known performance regressions in Drupal 8 Entity Field API is already mentioned, and #1762258: Benchmark: Bypass magic to speed up the new entity field API is cross-linked there. It makes sense that large-scale performance work is going to have to wait until after we drop the BC library. Unfortunately, catch isn't around to validate that this sort of regression is acceptable, but since this blocks 50,000 things and the performance regressions seem to be already documented, I'd like to fast-track it in. I'm obviously happy to be told it needs to be rolled back if that's out of line, though.
Committed and pushed to 8.x. Thanks!
This will need a change notice, methinks. :P
Comment #201
plachI updated http://drupal.org/node/1806650 to mention this issue and the comment conversion one.
Comment #202
fagoThanks! I created the follow-up: #1939994: Complete conversion of nodes to the new Entity Field API
Comment #202.0
fagoUpdated issue summary.
Comment #203
YesCT CreditAttribution: YesCT commentedI added a followup section to the summary, and removed the needs change notice tag
Comment #204
Gábor HojtsyThanks for the heroic work. We now need to move on to #1498674: Refactor node properties to multilingual for D8MI so we can get rid of the node-copy translation module sooner than later and work on an upgrade path. Lots of work still dependent on this to be fixed before API freeze. Any help is welcome!
Comment #205
Gábor HojtsyMoving off of D8MI sprint.
Comment #207
sunThe NG + language/translation changes are fine, but:
How and why was $entity->taxonomy_forums->tid suddenly turned into a single-valued item?
The new logic inserts a single value per language.
The old logic inserted multiple values for each language.
Comment #208
BerdirOpened #1956848: NodeNG conversion follow-up: Only first forum tid is saved into {forum_index} for that, let's not re-open huge issues like this for something that's not super-critical :)
Comment #209
amateescu CreditAttribution: amateescu commentedResetting proper status :)
Comment #210
Gábor HojtsyRetroactively tagging for D8MI.
Comment #210.0
Gábor Hojtsyadded follow up section