Context

Spin-off from #1992138: Helper issue for "field types as TypedData Plugins" / #1969728: Implement Field API "field types" as TypedData Plugins.

That issue adds actual validation on entity fields.
This means that implicit or explicit constraints contained in FieldItem::getPropertyDefinitions() (e.g 'type' => 'uri' in LinkItem, 'type' => 'integer' for 'tid' in TaxonomyTermReferenceItem...) that were just "dormant" so far in HEAD, are now fired.
This shows a couple problems, I'll open separate issues for them.

Issue

The "auto create / freetagging" feature of the autocomplete widgets (for entity_reference or taxo fields) break the constraint on 'target_id' / 'tid' being of type 'integer', because the items created by the "auto create" flow end up with a 'target_id' / 'tid' = FALSE, which is not an integer.

Patches coming up.

CommentFileSizeAuthor
#4 e_r_constraints_autocreate-2012662-4-VALIDATE.patch1011 bytesyched
#4 e_r_constraints_autocreate-2012662-4-VALIDATE-FIX.patch3.66 KByched
#4 e_r_constraints_autocreate-2012662-4-FIX-ONLY.patch2.68 KByched
#2 e_r_constraints_autocreate-2012662-1-PASS.patch2.37 KByched
#1 e_r_constraints_autocreate-2012662-1-FAIL.patch991 bytesyched
#1 e_r_constraints_autocreate-2012662-1-PASS.patch2.35 KByched
#1 e_r_constraints_autocreate-2012662-1-FIX-ONLY.patch1.38 KByched

Comments

yched’s picture

yched commented
Status: Active » Needs review
StatusFileSize
new e_r_constraints_autocreate-2012662-1-FIX-ONLY.patch1.38 KB
new e_r_constraints_autocreate-2012662-1-PASS.patch2.35 KB
new e_r_constraints_autocreate-2012662-1-FAIL.patch991 bytes

- "FAIL" patch adds a mock validation step in EntityFormController to demonstrate the buggy behavior when validation is actually performed. This should fail.
- "PASS" adds a proposed fix (standardize on '0' instead of FALSE to indicate "new entity being created"). This should pass.
- "FIX-ONLY" is just the fix, without the mock validate() calls. This should pass, and is the candidate patch to commit to HEAD.

yched’s picture

yched commented
StatusFileSize
new e_r_constraints_autocreate-2012662-1-PASS.patch2.37 KB

Oops, warnings in the "mock validation code". Updated version of the "PASS" patch.

Status: Needs review » Needs work

The last submitted patch, e_r_constraints_autocreate-2012662-1-PASS.patch, failed testing.

yched’s picture

yched commented
Status: Needs work » Needs review
StatusFileSize
new e_r_constraints_autocreate-2012662-4-FIX-ONLY.patch2.68 KB
new e_r_constraints_autocreate-2012662-4-VALIDATE-FIX.patch3.66 KB
new e_r_constraints_autocreate-2012662-4-VALIDATE.patch1011 bytes

So, ok:
- the "PASS" patch still has many fails despite including the fix for this issue, because triggering constraint validation causes a lot more stuff than just constraints on target_id / tid.
- in the fix, the taxonomy formatters need to be adjusted accordingly

New patches:
- the "VALIDATE" patch adds a mock validation step in EntityFormController to demonstrate the buggy behavior when validation is actually performed. This should fail.
- the "VALIDATE+FIX" patch adds a proposed fix (standardize on '0' instead of FALSE to indicate "new entity being created"). This still fails, but with less fails :-)
- the "FIX-ONLY" patch is just the fix, without the mock validate() calls. This should pass, and is the candidate patch to commit to HEAD.

yched’s picture

yched commented
Issue tags: +Field API NG blocker

Tagging

berdir’s picture

berdir
Primary language German
Location Switzerland
commented
Issue tags: +Entity Field API

@yched: Note that all issues also need the Entity Field API tag to show up at http://entity.worldempire.ch/conversions

amateescu’s picture

amateescu commented
Status: Needs review » Reviewed & tested by the community
Issue tags: -Entity Field API

Given that tests will come naturally when #1969728: Implement Field API "field types" as TypedData Plugins gets in and we'll actually use the constraints, I think it's fair to get this one committed before.

amateescu’s picture

amateescu commented
Issue tags: +Entity Field API

Restoring tags..

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Reviewed & tested by the community » Needs review 
+++ b/core/lib/Drupal/Core/Entity/Field/Type/EntityWrapper.phpundefined
@@ -90,7 +90,7 @@ public function setValue($value, $notify = TRUE) {
-      $value = FALSE;
+      $value = 0;

This fix seems incomplete now all new values are 0 which is a int... what if the value being set is a string?

yched’s picture

yched commented

@alexpott not sure what you mean. There should not be a case where 'target_id' gets assigned as a string.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Needs review » Reviewed & tested by the community

@yched oh - yep ingore me...

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Reviewed & tested by the community » Fixed

Committed d3d704e and pushed to 8.x. Thanks!

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

Anonymous’s picture

(not verified) commented
Issue summary: View changes

Add structure