Support from Acquia helps fund testing for Drupal Acquia logo

Comments

swentel’s picture

Status: Postponed » Active
pfrenssen’s picture

Assigned: Unassigned » pfrenssen
pfrenssen’s picture

Status: Active » Needs review
FileSize
9.4 KB

Have done first part of the conversion. Am currently struggling with converting the hook_field_validate() to getConstraints(). 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.

pfrenssen’s picture

This is also depending on #2016177: Regression: Refactor 'autocreate entity' handling from entity_reference. I will update the issue summary with the blocking issues.

Status: Needs review » Needs work

The last submitted patch, 2015701-3-field_type-entity_reference.patch, failed testing.

pfrenssen’s picture

Assigned: pfrenssen » Unassigned
Status: Needs work » Needs review
FileSize
15.31 KB

Status: Needs review » Needs work

The last submitted patch, 2015701-6-field_type-entity_reference.patch, failed testing.

klausi’s picture

Where is the EntityReferenceValidReference constraint defined?

pfrenssen’s picture

I forgot to include EntityReferenceValidReferenceConstraint and EntityReferenceValidReferenceConstraintValidator in the patch, and have reset my repository in the meanwhile :-/ Will recreate them.

pfrenssen’s picture

Status: Needs work » Needs review
FileSize
8.15 KB
17.49 KB

Recreated 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.

Status: Needs review » Needs work

The last submitted patch, 2015701-10-field_type-entity_reference.patch, failed testing.

pfrenssen’s picture

Status: Needs work » Needs review
FileSize
3.1 KB
19.49 KB

Converted hook_field_settings_form().

Status: Needs review » Needs work

The last submitted patch, 2015701-12-field_type-entity_reference.patch, failed testing.

klausi’s picture

+++ b/core/modules/entity_reference/lib/Drupal/entity_reference/Plugin/Validation/Constraint/EntityReferenceValidReferenceConstraintValidator.php
@@ -0,0 +1,39 @@
+  public function validate($value, Constraint $constraint) {
+    // @todo This should probably be done in ConfigurableEntityReferenceItem.
+    $instance = $value->getInstance();
+    $handler_settings = $instance->getFieldSetting('handler_settings');
+
+    $referenced_entity = $value->get('entity')->getTarget();
+    $referenced_entity_info = $referenced_entity->entityInfo();
+
+    // @todo Use SelectionBase::validateReferenceableEntities()?
+    $query = \Drupal::entityQuery($referenced_entity->entityType());
+    $query->condition($referenced_entity_info['entity_keys']['bundle'], $handler_settings['target_bundles'], 'IN');
+    $query->condition($referenced_entity_info['entity_keys']['id'], $referenced_entity->id());
+
+    $result = $query->execute();
+    if (empty($result)) {
+      $this->context->addViolation($constraint->message, array('%type' => $referenced_entity->entityType(), '%id' => $referenced_entity->id()));
+    }

why the extra query? $referenced_entity will be FALSE if the entity_load() in EntityReference::getTraget() fails, right?

klausi’s picture

Status: Needs work » Needs review
FileSize
12.06 KB
20.27 KB

Moved 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.

Status: Needs review » Needs work

The last submitted patch, entity-reference-plugin-2015701-15.patch, failed testing.

klausi’s picture

Status: Needs work » Needs review
FileSize
776 bytes
19.37 KB

Fixed the auto creation test case.

I have no idea why the ContentTranslationWorkflowsTest and ContentTestTranslationUITest fails.

amateescu’s picture

Those 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..

amateescu’s picture

Also, as a small review of the patch, we need to convert entity_reference_field_instance_settings_form() as well :)

Status: Needs review » Needs work

The last submitted patch, entity-reference-plugin-2015701-17.patch, failed testing.

pfrenssen’s picture

For me locally the tests fail with the patch on PHP 5.4, but pass without the patch.

klausi’s picture

Status: Needs work » Needs review
FileSize
1.8 KB
21.51 KB

Those 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()

klausi’s picture

Removed 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?

amateescu’s picture

klausi’s picture

LOL, 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?

klausi’s picture

Fixed line length of some moved code comments, thanks to swentel for pointing that out.

Status: Needs review » Needs work

The last submitted patch, entity-reference-plugin-2015701-26.patch, failed testing.

pfrenssen’s picture

Status: Needs review » Needs work
Issue tags: -Field API

The last submitted patch, entity-reference-plugin-2015701-28.patch, failed testing.

swentel’s picture

Status: Needs work » Needs review
Issue tags: +Field API
swentel’s picture

Status: Needs review » Needs work

This 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.

+++ b/core/modules/entity_reference/lib/Drupal/entity_reference/Plugin/field/field_type/ConfigurableEntityReferenceItem.phpundefined
@@ -0,0 +1,204 @@
+ *   module = "entity_reference",

You can drop this line, see #2041423: Rely on 'provider' instead of 'module' for Field plugin types

yched’s picture

+++ b/core/modules/entity_reference/entity_reference.moduleundefined
@@ -12,30 +12,6 @@
-    'settings' => array(
-      // Default to a primary entity type (i.e. node or user).
-      'target_type' => module_exists('node') ? 'node' : 'user',

Annotations don't allow this of course, but this could be reproduced using hook_field_info_alter().
Dunno if this is deemed important.

+++ b/core/modules/entity_reference/lib/Drupal/entity_reference/Plugin/field/field_type/ConfigurableEntityReferenceItem.phpundefined
@@ -0,0 +1,204 @@
+      '#default_value' => $field->settings['target_type'],

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');

+++ b/core/modules/entity_reference/lib/Drupal/entity_reference/Plugin/field/field_type/ConfigurableEntityReferenceItem.phpundefined
@@ -0,0 +1,204 @@
+    $field = isset($form_state['entity_reference']['field']) ? $form_state['entity_reference']['field'] : $this->getInstance()->getField();
+    $instance = isset($form_state['entity_reference']['instance']) ? $form_state['entity_reference']['instance'] : $this->getInstance();

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 :-)

+++ b/core/modules/entity_reference/lib/Drupal/entity_reference/Plugin/field/field_type/ConfigurableEntityReferenceItem.phpundefined
@@ -0,0 +1,204 @@
+      '#element_validate' => array('_entity_reference_field_instance_settings_validate'),
...
+      '#process' => array('_entity_reference_form_process_merge_parent'),

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 ?

amateescu’s picture

Status: Needs work » Needs review
FileSize
19.76 KB
41.34 KB

Annotations don't allow this of course, but this could be reproduced using hook_field_info_alter().
Dunno if this is deemed important.

Of 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.

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 :-)

I'd prefer to do it here..

amateescu’s picture

And some {@inheritdoc}s.

yched’s picture

Awesome, thanks @amateescu :-)

+++ b/core/modules/entity_reference/lib/Drupal/entity_reference/Plugin/field/field_type/ConfigurableEntityReferenceItem.phpundefined
@@ -0,0 +1,210 @@
+    $field_definition = isset($form_state['instance']) ? $form_state['instance'] : $this->getInstance();

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.]

+++ b/core/modules/entity_reference/lib/Drupal/entity_reference/Plugin/field/field_type/ConfigurableEntityReferenceItem.phpundefined
@@ -0,0 +1,210 @@
+      '#element_validate' => array(array($this, 'instanceSettingsFormValidate')),

Should be array(get_class($this), 'instanceSettingsFormValidate')) if we want to benefit from the method being static

amateescu’s picture

That's right :)

yched’s picture

Status: Needs review » Reviewed & tested by the community

Thanks! RTBC if green :-)

swentel’s picture

Important, 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.

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
+++ b/core/modules/entity_reference/lib/Drupal/entity_reference/Tests/EntityReferenceFieldTest.phpundefined
@@ -0,0 +1,127 @@
+    // @todo Implement a test case for a referenced entity bundle here.
+    /*$invalid_referenced_entity = entity_create($this->referencedEntityType, array('type' => drupal_strtolower($this->randomName())));
+    $invalid_referenced_entity->save();
+
+    $entity = entity_create($this->entityType, array('type' => $this->bundle));
+    $entity->{$this->fieldName}->target_id = $invalid_referenced_entity->id();
+
+    $violations = $entity->{$this->fieldName}->validate();
+    $this->assertEqual($violations->count(), 1, 'Validation throws a violation.');*/

Adding 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.

alexpott’s picture

Also #2051157: pass $has_data as a param to ConfigFieldItem::settingsForm() has landed so

+++ b/core/modules/entity_reference/lib/Drupal/entity_reference/Plugin/field/field_type/ConfigurableEntityReferenceItem.phpundefined
@@ -0,0 +1,210 @@
+  /**
+   * {@inheritdoc}
+   */
+  public function settingsForm(array $form, array &$form_state) {

Need add $hasData param

amateescu’s picture

Status: Needs work » Reviewed & tested by the community
FileSize
2.67 KB
41.98 KB

Adressed 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.

yched’s picture

Yup, sorry for letting that @todo slip by :-/

yched’s picture

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Committed 6aa975c and pushed to 8.x. Thanks!

yched’s picture

Automatically closed -- issue fixed for 2 weeks with no activity.

Anonymous’s picture

Issue summary: View changes

Added blocking issues.