Early Bird Registration for DrupalCon Portland 2024 is open! Register by 23:59 PST on 31 March 2024, to get $100 off your ticket.
Comment | File | Size | Author |
---|---|---|---|
#41 | entity-reference-plugin-2015701-41.patch | 41.98 KB | amateescu |
#41 | interdiff.txt | 2.67 KB | amateescu |
#36 | entity-reference-plugin-2015701-36.patch | 42.44 KB | amateescu |
#36 | interdiff.txt | 971 bytes | amateescu |
#34 | entity-reference-plugin-2015701-34.patch | 42.43 KB | amateescu |
Comments
Comment #1
swentel CreditAttribution: swentel commentedComment #2
pfrenssenComment #3
pfrenssenHave done first part of the conversion. Am currently struggling with converting the
hook_field_validate()
togetConstraints()
. Note that this patch contains a fix for Doctrine Annotations that allows empty arrays in annotations. This is part of #1848570: Upgrade to Doctrine\Common 2.4.Comment #4
pfrenssenThis is also depending on #2016177: Regression: Refactor 'autocreate entity' handling from entity_reference. I will update the issue summary with the blocking issues.
Comment #6
pfrenssenThis also contains the test of #1978714: Entity reference doesn't update its field settings when referenced entity bundles are deleted.
Comment #8
klausiWhere is the EntityReferenceValidReference constraint defined?
Comment #9
pfrenssenI forgot to include EntityReferenceValidReferenceConstraint and EntityReferenceValidReferenceConstraintValidator in the patch, and have reset my repository in the meanwhile :-/ Will recreate them.
Comment #10
pfrenssenRecreated the constraint, and removed the second test from #1978714: Entity reference doesn't update its field settings when referenced entity bundles are deleted, this now only keeps the test that was originally intended for this issue.
Comment #12
pfrenssenConverted hook_field_settings_form().
Comment #14
klausiwhy the extra query? $referenced_entity will be FALSE if the entity_load() in EntityReference::getTraget() fails, right?
Comment #15
klausiMoved the constraint + validator out of entity reference module, because we also need that for EntitReferenceItem, which lives outside of entity_reference.module.
Expanded the entity validation test to also cover the user reference example.
Was not able to fix EntityReferenceAutoCreateTest, so this will still fail.
Comment #17
klausiFixed the auto creation test case.
I have no idea why the ContentTranslationWorkflowsTest and ContentTestTranslationUITest fails.
Comment #18
amateescu CreditAttribution: amateescu commentedThose tests fail locally for me even without the patch (on PHP 5.4), so maybe the patch just exposes it to PHP 5.3? Though it's very hard to see why this would happen..
Comment #19
amateescu CreditAttribution: amateescu commentedAlso, as a small review of the patch, we need to convert entity_reference_field_instance_settings_form() as well :)
Comment #21
pfrenssenFor me locally the tests fail with the patch on PHP 5.4, but pass without the patch.
Comment #22
klausiThose are indeed valid test fails because the content translation tests are setting invalid user references, which trigger constraint violations.
TODO: convert entity_reference_field_instance_settings_form()
Comment #23
klausiRemoved the stale entity_reference_get_selection_handler() which was already removed in 8.x, and moved entity_reference_field_instance_settings_form() to the field item class.
Unfortunately the Entity Reference UI test fails, so something is wrong in the form. call_user_func_array() expects parameter 1 to be a valid callback, class 'select' not found. Somewhere a #process callback or a similar form thing is expected to be a class?
Comment #24
amateescu CreditAttribution: amateescu commentedThat's a different bug you're hitting: #2025451: Drupal\entity_reference\Tests\EntityReferenceAdminTest fails on PHP 5.4.9 and libxml 2.9.0
Comment #25
klausiLOL, so I also spend some quality hours on my PHP 5.4 install just to find out that Drupal 8 does not even fully run on 5.4 yet ...
But let's ignore that here since the testbot is happy on 5.3, anyone want to review/RTBC?
Comment #26
klausiFixed line length of some moved code comments, thanks to swentel for pointing that out.
Comment #28
pfrenssenTests failed because #2058257: Fix ConfigFieldItemInterface's method signatures to typehint interfaces instead of classes went in.
Comment #30
swentel CreditAttribution: swentel commented#28: entity-reference-plugin-2015701-28.patch queued for re-testing.
Comment #31
swentel CreditAttribution: swentel commentedThis looks solid to me, except for one minor thing. Other than that, looks RTBC to me, but I'd like to have a sign off from amateescu and/or yched too.
You can drop this line, see #2041423: Rely on 'provider' instead of 'module' for Field plugin types
Comment #32
yched CreditAttribution: yched commentedAnnotations don't allow this of course, but this could be reproduced using hook_field_info_alter().
Dunno if this is deemed important.
We try to settle on using the FieldDefinitionInterface methods in the FieldItem classes (aside from schema(), which is a bit specific).
This should be $this->getFieldSetting('target_type');
Hm, uncoupling this code from $field / $instance and making it run on FieldDefinitionInterface is going to be a bit complex. Would be nice in a followup , though :-)
It would be nice to move _entity_reference_field_instance_settings_validate() & _entity_reference_form_process_merge_parent() to methods in the class ? (preferably as static methods, less overhead when form gets serialized.)
If not here, in a followup ?
Comment #33
amateescu CreditAttribution: amateescu commentedOf course it is, re-added it in the patch attached.
Moved _entity_reference_field_instance_settings_validate() to the new class, but the other is a bit problematic because it's a generic helper that's also used by selection handlers and IMO it should go into Form API. Same for most of the other form and ajax helpers from entity_reference.
I'd prefer to do it here..
Comment #34
amateescu CreditAttribution: amateescu commentedAnd some {@inheritdoc}s.
Comment #35
yched CreditAttribution: yched commentedAwesome, thanks @amateescu :-)
Does the code that sets $form_state['instance'] really gets a FieldInstanceInterface, or a FieldDefinitionInterface now ?
If the latter, maybe this should be $form_state['field_definition'] / $this->getFieldDefinition() ?
[edit: ah, scratch that. instanceSettingsFormValidate() writes settings in this object, and FieldDefinitionInterface has no setters right now :-(. Never mind.]
Should be array(get_class($this), 'instanceSettingsFormValidate')) if we want to benefit from the method being static
Comment #36
amateescu CreditAttribution: amateescu commentedThat's right :)
Comment #37
yched CreditAttribution: yched commentedThanks! RTBC if green :-)
Comment #38
swentel CreditAttribution: swentel commentedImportant, if this one goes in first, #2051157: pass $has_data as a param to ConfigFieldItem::settingsForm() needs work, and vica versa.
- edit - Maybe this one first, the other one is easier to reroll.
Comment #39
alexpottAdding commented out code with a @todo or not referencing an issue is a bit weird. Is this still to do in this patch or is it for a followup - if the latter please create the issue and add the code to the issue and replace all of this with an @todo referencing said followup.
Comment #40
alexpottAlso #2051157: pass $has_data as a param to ConfigFieldItem::settingsForm() has landed so
Need add $hasData param
Comment #41
amateescu CreditAttribution: amateescu commentedAdressed both comments from above. I opened #2064191: Fix the 'target_bundle' field setting - 'target_bundles' are not validated when entity_reference is enabled for the invalid bundle reference test as it seems to be a bigger problem than the scope of this patch.
Comment #42
yched CreditAttribution: yched commentedYup, sorry for letting that @todo slip by :-/
Comment #43
yched CreditAttribution: yched commented#41: entity-reference-plugin-2015701-41.patch queued for re-testing.
Comment #44
alexpottCommitted 6aa975c and pushed to 8.x. Thanks!
Comment #45
yched CreditAttribution: yched commentedFYI: created #2073661: Add a EntityReferenceField::referencedEntities() method
Comment #46.0
(not verified) CreditAttribution: commentedAdded blocking issues.