Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
Comment | File | Size | Author |
---|---|---|---|
#39 | er-interdiff.txt | 33.55 KB | amitaibu |
#39 | er-fieldItem-1847588-39.patch | 171.33 KB | amitaibu |
#37 | er-interdiff.txt | 33.36 KB | amitaibu |
#37 | er-fieldItem-1847588-37.patch | 171.14 KB | amitaibu |
#33 | er-interdiff.txt | 33.36 KB | amitaibu |
Comments
Comment #1
amitaibuComment #2
amitaibuWork is done in branch - yched\
er-fieldItem-1847588
Comment #3
fagotagging
Comment #4
amitaibuI'm seeing an error whenever I
entity_create('entity_test', array())->save()
causing my tests to fail
Anyone familiar with this?
Comment #5
fagoWhat's the actual error? I could imagine some fields are required on the db level?
Comment #6
amitaibu> What's the actual error?
Unsupported operand types
I was able to reproduce the error it several times on a clean installation (however one time on another computer it didn't work), via hook_init:
Comment #7
amitaibuThe error in #6 was due to EntityTestStorageController::baseFieldDefinitions() having a wrong name (entityreference instead of entity_reference).
Now that we moved the ER plugin under entity-reference module should we make entity-test dependent on ER?
Comment #8
amitaibu@fago,
I see that in DatabaseStorageControllerNG::mapToStorageRecord() we have
$record->$name = $entity->$name->value;
, however ER has the "target_id" and "entity" properties, it doesn't have "value", so it will fail when entity_test is enabled.Question are:
1) How to overcome that
->value
hardcoding?2) what to do with "user_id" in entity-test -- should entity_test module rely on ER?
Comment #9
fagoHaving
$entity->user_id->target_id
in the API is weird in general though. If we use target_id instead of value for references, I think we should rename user_id to "user", e.g. have$entity->user->target_id
. That would be more inline with the naming of fields anyway.To fix, I see multiple ways:
a) Just customize it in the entity storage controller and map it "manually" there.
b) While not being so nice any more, we could fix it by using the value of the first not computed property by default?
c) A cleaner solution would be to support a proper naming pattern in the schema columns, e.g. "field__column" or "field:column" (if allowed in the schema?).
I must say I'd like c) most, e.g. rename the user_id field to user and the user_id column to user:target_id or user__target_id ?
I think so, yes.
Comment #10
amitaibuchx from IRC - the short version, took me some time to understand what he meant ;) :
Comment #11
fagoThis test cases is actually about testing non-configurable fields, so converting it isn't a good idea.
Comment #12
amitaibuDatabaseStorageControllerNG::mapToStorageRecord()
Patch is against 8.x, and interdiff against the main ER patch.
Comment #13
tim.plunkettSorry if it's too early for this sort of review...
Probably out scope, but due to annotations we've moved to more keys with underscores instead of spaces
Ewwww :(
Here and elsewhere, "Contains \Drupal\..."
Don't use top level classes, just \ them
typo entityre_ference
Needs visibility
Missing the fully qualified namespace, here and every method
Here and everywhere, I believe it should static::, not 100% sure
s/id/ID
I've seen entity_reference or entity reference, but not entity-reference
$node_1, $node_2
Sure, I guess :)
Comment #15
amitaibuThanks Tim!
I know :) Didn't know what where was the best place to to put it. Any suggestions, while I fix the other tests?
Comment #16
amitaibuCan you explain what you mean?
I've just moved the original class under Entity reference module - currently it's using
self
.Tests are copied and adapted from TaxonomyTermReferenceItemTest which uses the $term1. I actually think $node1 is nicer than $node_1, but if you feel strongly about it, I can change it :)
Comment #17
tim.plunkettThe moved code stuff can stay as is then, that just wasn't clear it was moved.
By visibility I mean public/protected.
Comment #19
amitaibuOh, removed wrong file just before created the patch :/
Comment #21
amitaibuChanging target_id => value seems to be way more complicated than I expected. To keep this patch small, aviod scopre creep, and arguably "value" is better than "target_id" -- I'll keep it as value.
Comment #22
amitaibuMissed another test --
entity-test/add
doesn't work now. Damn, it's an harder than expected kind of patch ;)Comment #23
amitaibuOk, enough for today. Let's see in the morning if there are still failing tests.
Comment #26
amitaibuNope, seems we need to change to target_id after all.
Current problem is that after this code:
I get wsod, because the loaded values are incorrect
Comment #27
amitaibuFound the issue from #26, but fixed as hack DatabaseStorageControllerNG::mapFromStorageRecords()
Not sure what's the proper fix.
Uploading to test with testbot (takes to long on my local)
Comment #29
Berdir@fago and I already had a similar discussion in regarts to date fields in the comments NG issue.
There, only the entity -> storage conversion was a problem, not both as in here.
Somehow, we need to make it more flexible, but I'm not quite sure yet.
Right now, this essentially means that contrib can't define data types that don't have a value property. Which is bad.
Maybe we can somehow use the property data.
Note that this will conflict with #1833334: EntityNG: integrate a dynamic property data table handling and #1869250: Various EntityNG and TypedData API improvements has changes related to these parts as well I think.
I disagree that we should be doing this.
The entity reference data type will also be used for e.g. the uid property on nodes/comments, which would make entity_reference a dependency for node.module, do we really want that?
I don't have a problem with keeping this in Drupal\Core and entityreference.module would just be responsible for exposing it as a configurable field in the UI.
Comment #30
fagoFor setting the value we can omit the $key, as the field-items implement their own logic to set the items from a primitive. This would be already fixed with #1869250: Various EntityNG and TypedData API improvements.
Well, this could be
+ $key = key($entity->{$name}[0]->getProperties());
Not nice, but better as the previous hard-coding to 'value'.
@berdir: yep we should come-up with a general mapping the storage controller uses, as written above:
Comment #31
amitaibuKept the original field API in place as per #29
Fixed tests. I hope I got them all... All those fails are a result of the hardcoded "value" key.
Comment #32
fagoGood work!
True. Having dependencies on various modules just for various always-needed storage-fields seems weird, so I like this approach.
I just found some minor issues:
entity_reference_field ?
Can we keep that comment?
This could need a comment why we need to implement the hook on our own.
Well we could just use
$duplicate->{$field}->getValue() == $entity->{$field}->getValue()
?While not directly related, in that case
$target_translation->$field = $source_translation->$field;
should do it.Comment #33
amitaibuAddressed fago's comments.
Comment #34
tim.plunkettStill says entityre_reference?
Comment #36
amitaibuOh, missed that there's a typo there :)
I'll re-roll.
Comment #37
amitaibuComment #39
amitaibuThis actually caused the test in #37 to fail, so re-added it.
Comment #40
amitaibuOk, test are back green, and I've addressed fago's comments, so merging it to ER's main branch.