Spin-off from #2002180: Entity forms skip validation of fields that are edited without widgets

Postponed on
#2429037: Allow adding entity level constraints
and
#2105797: Add CompositeConstraintBase so that constraints involving multiple fields, such as CommentNameConstraint, can be discovered which is postponed on
#2343035: Upgrade validator integration for Symfony versions 2.5+.

Problem/Motivation

  • Entity changed validation (=ensure no one edited the entity while editing it) gets ignored by all entity forms but nodes right now. It works for nodes only, because NodeForm takes care of it manually while all the other entity forms aren't.
  • Suppose you have 2 fields (A and B) in a given entity type + bundle.
  • Suppose field B has a validation constraint that depends on the contents of field A (e.g., field A is "date of foo", field B is "date of bar", and you want a validation constraint on field B that it is later than A).
  • Create 2 form displays: one that contains both A and B, and one that contains only A.
  • Using the first form, you can create an entity with values for A and B that satisfy all constraints.
  • If you now edit with the 2nd form, you can change A to something that makes B's value violate its constraint, but because the form only contains field A, that's the only field that will get validated, so the form will submit and the entity will get saved successfully, despite being invalid according to its full constraints.
  • Currently, we run validation against fields on the entity forms that have widgets. However, if a field can be edited with a form element that is not a Field API widget, we do not validate its value at the field-level (i.e., check it against the field's constraints). The form element might have its own custom #element_validate, but since that's a custom function, there's no guarantee that it results in a value that satisfies the field's constraints.

Proposed resolution(s)

  • On form submit, validate the entire entity rather than only the fields in the form.
  • However, what to do about violations that are reported for fields that aren't on the form? For example, if B reports the violation (since it's the one with the constraint), its violation message might not make sense (and might expose confidential information, such as that such a field exists) to a person who only sees what's on the form.

Remaining tasks

  1. Define scenarios for the proper behavior of entity validation:
    Scenario A:
    Constraint is validating field A in dependence of field B. It shows the violation message on field A. If field B is hidden, the constraint is still validated. If validation fails, the message for field A is shown as error message.
    Scenario B:
    Constraint is validating field A in dependence of field B. It shows the violation message on field A. If field A is hidden, the constraint is still validated. If validation fails, the message (for the hidden field A) is not shown, but a general message is put on field B. E.g. if the field is accessible to the current user: "The provided value is invalid in the context of the current value of field %field-label.", if not accessible: "The provided value is invalid for the currently set values."

  2. Write tests for the proper behavior of entity validation

  3. Use these tests to finalize a resolution for this issue (tests will inform implementation approach)

  4. Write patch to make tests pass (See proposed resolution)

API changes

  • EntityFormDisplayInterface::flagViolations() has been added
  • FieldableEntityInterface::validate() now returns EntityConstraintViolationListInterface instead of ViolationListInterface (no API change for callers)
  • Content Entity Forms now have to override ContentEntityForm::getEditedFieldNames() & ContentEntityForm::flagViolations() for flagging violations for customly added base fields

User interface changes

There is the potential for a UI change. If we validate the entire entity and a field that isn't on the form has a violation, we need to inform the user that they need to contact an administrator or something. In other words, we need to tell them that they won't be able to submit the form until all the constraints are passing, including on the fields they don't have access to modify.

We use the following message for that:

+            $message_template = 'The validation failed because the value conflicts with the value in %field_name, which you cannot access.';

Beta phase evaluation

Reference: https://www.drupal.org/core/beta-changes
Issue priority Critical because this is required to ensure no violations related to conditional field validations are wrongly suppressed when using entity form display where only parts of the involved fields are shown.
CommentFileSizeAuthor
#137 2395831-137.patch53.95 KBdawehner
#137 interdiff.txt2.86 KBdawehner
#137 2395831-137.patch53.95 KBdawehner
#137 DC8AEF3C7E_201564_83639_leaf_sized.png32.61 KBdawehner
#134 interdiff.txt10.53 KBdawehner
#134 2395831-133.patch53.86 KBdawehner
#128 interdiff.txt10.5 KBdawehner
#128 2395831-128.patch44.35 KBdawehner
#127 Edit_Basic_page_Test_X___Drupal_8_-_TMP.png127.05 KBplach
#125 interdiff.txt1.14 KBdawehner
#125 2395831-125.patch40.59 KBdawehner
#118 d8_entity_validate_4.interdiff.txt3.92 KBfago
#118 d8_entity_validate_4.patch40.5 KBfago
#114 interdiff-107-114.txt839 bytesmartin107
#114 d8_entity_validate_114.patch40.41 KBmartin107
#107 interdiff-104-107.txt3.58 KBmartin107
#107 d8_entity_validate_107.patch40.41 KBmartin107
#104 d8_entity_validate_4.patch40.39 KBfago
#104 d8_entity_validate_4.interdiff.txt1.69 KBfago
#102 d8_entity_validate_4.interdiff.txt18.89 KBfago
#102 d8_entity_validate_4.patch40.14 KBfago
#100 drooling-homer-simpson.jpg15.08 KBplach
#96 d8_entity_validate_4.interdiff.txt2.61 KBfago
#96 d8_entity_validate_4.patch36.91 KBfago
#95 2395831.95.patch37.47 KBYesCT
#90 interdiff.txt765 bytesdawehner
#90 2395831-90.patch37.49 KBdawehner
#88 interdiff.txt6.45 KBdawehner
#88 2395831-88.patch37.43 KBdawehner
#85 interdiff.txt5.31 KBdawehner
#85 2395831-84.patch37.44 KBdawehner
#83 interdiff.txt7.21 KBdawehner
#83 2395831-83.patch36.01 KBdawehner
#82 interdiff.txt6.77 KBdawehner
#82 2395831-82.patch28.8 KBdawehner
#80 d8_entity_validate_3.interdiff.txt3.79 KBfago
#80 d8_entity_validate_3.patch28.84 KBfago
#79 interdiff.txt1.42 KBdawehner
#79 2395831-79.patch29.04 KBdawehner
#75 interdiff.txt3.21 KBdawehner
#75 2395831-75.patch29.31 KBdawehner
#73 2395831-73.patch29.48 KBdawehner
#73 interdiff.txt2.08 KBdawehner
#71 interdiff.txt5.55 KBdawehner
#71 2395831-71.patch31.55 KBdawehner
#69 interdiff.txt848 bytesdawehner
#69 2395831-69.patch30.44 KBdawehner
#67 interdiff.txt4.15 KBdawehner
#67 2395831-67.patch29.98 KBdawehner
#65 interdiff.txt1.15 KBdawehner
#65 2395831-65.patch26.52 KBdawehner
#63 interdiff.txt4.81 KBdawehner
#63 2395831-63.patch25.37 KBdawehner
#45 2395831-entity-violations-45.patch22.87 KBmartin107
#45 interdiff-42.45.txt600 bytesmartin107
#43 interdiff-40-42.txt1.28 KBmartin107
#43 2395831-entity-violations-42.patch22.86 KBmartin107
#40 interdiff.txt5.75 KBcafuego
#40 2395831-entity-violations-40.patch22.91 KBcafuego
#38 interdiff.txt13.03 KBcafuego
#38 2395831-entity-violations-38.patch16.12 KBcafuego
#30 entity_violations.interdiff.txt8.42 KBfago
#30 entity_violations.patch22.98 KBfago
#28 entity_violations.patch8.42 KBfago
#25 entity_violations.patch19.78 KBfago
#21 entity_violations.wip_.txt7.9 KBfago
#20 entity_violations.wip_.txt4.72 KBfago
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

effulgentsia’s picture

Status: Needs work » Active

Although there's a starter patch in #2002180-39: Entity forms skip validation of fields that are edited without widgets, I'm setting this to "Active" because I think it could use more discussion on the proposed resolution and how to handle the violation messages. See also #2002180-36: Entity forms skip validation of fields that are edited without widgets for a proposal to have a hook for customizing or removing those messages.

effulgentsia’s picture

Issue summary: View changes
fago’s picture

Priority: Major » Critical

Not critical because core has no examples of this situation, the situation would come up in only a minority of real-world sites, and contrib/custom code can be written to work around it (e.g., add the desired validation logic).

Imo, this is still critical:
If this issue is not solved, people will have to provide duplicated entity validation logic for fields being not present in EntityFormDisplays: For one they will have to take care of entity validation for REST and second they will have to take of the same valdiation in forms. They could re-use validation logic of their single fields from their custom form validation, but a) there are no examples doing so in core and b) people will most likely miss that and care about their forms only.

For solving this, I think we have to overhaul the entity form validation integration to blacklist the fields not being present on a form instead of whitelisting the few fields that we know are present on the form. That way, any added entity validation constraints will be automatically picked up and can / have to be properly mapped by the entity form.

Then, I think we need to overhaul violation mapping strategy for the conditional validation (see #2105797: Add CompositeConstraintBase so that constraints involving multiple fields, such as CommentNameConstraint, can be discovered) to work out anyway: As conditional validation involves multiple fields, the constraint can only be put on one field and would be missed for the other field then. For making sure we do not miss the validation logic, we must use a validation strategy that always that runs conditional validation if at least one of the fields is edited; i.e. always run a complete entity validation and then take care about mapping/ignoring violations accordingly. An example where combined validation is required is #2403485: Complete conversion of comment form validation to entity validation.

Thus, bumping to critical. Please comment if you disagree.

fago’s picture

As discussed in Amsterdam with effulgentsia, berdir and yched, a possible validation / violation mapping strategies to solve this would be:

1) Bind constraints to the fields, i.e. constraints put on a field are supposed to only validate the values contained in that field.
2) Apply a "Valid once, valid always" strategy and only validate changed fields. Existing data can be considered valid even if it is not compliant as long as it does not change.
3) Add entity level constraints for things that have to run always (entity changed validation) and conditional field validation (validate field A in dependence of field B)

Thus, I think
a) we need to know which fields are involved in conditional validation for being able to skip it if all involved fields are not accessible / on the form
b) once we do that and 1) + 3) from above, we can skip violations from fields that are not accessible / on the form

Some more related thoughts:
- If conditional validation is used, the violation must be shown as soon as one of the validated properties has changed. Best, we allow attaching conditional validation on entity level but specify covered fields.
- Violations come with a message which is target for a single property path only. If the property cannot be edited, it's violations can be hidden. This means, constraints of a field may only look at the fields value.

More related notes of issues discussed in Amsterdam (will file issues for that)
- Code needs to be able to rely on data types, i.e. enforce primitive data types and structure. It's the job of the serialization to verify basic data structure compliance, as well as every code that assigns a value.
- Entity reference does not validate referenced entities are valid (views based selection handler)
- Where to add entity level validation constraint?

fago’s picture

Action items based on my suggestion in #4:

effulgentsia’s picture

As far as we currently know, are there any issues that need to be solved first before it's possible to have a patch for #5.1-#5.3? For example, does #2227503: Apply formatters and widgets to Comment base fields need to happen first in order to have a core use case of a multifield constraint that the patch would provide a new way of accomplishing? Or does it make sense to explicitly solve #5.1-#5.3 without that use case, and leave that and any other similar core cases to #2105797: Add CompositeConstraintBase so that constraints involving multiple fields, such as CommentNameConstraint, can be discovered?

effulgentsia’s picture

ignore violations based on field access by default (make it overridable from the entity form?)

If we implement #4.2 (valid if unchanged from original) and leave #5.4 to that other issue, then what's a scenario in which there can be a violation for an inaccessible field?

fago’s picture

Or does it make sense to explicitly solve #5.1-#5.3 without that use case, and leave that and any other similar core cases to #2105797: Figure out the best way to do conditional validation?

I think, so yes. We can already verify here that entity level constraints like entity-changed validation are in place. E.g., right now NodeForm adds in changed validation manually, but that's not done by any other entity type; i.e. it's there right now, but ignored for all entities but nodes. (Yet another good reason do solve this.)

I think #5.1-#5.2 can be worked on independently of #2105797: Add CompositeConstraintBase so that constraints involving multiple fields, such as CommentNameConstraint, can be discovered; however #5.3 will need integration with #5.4, which I think is doable in any order (5.3 or 5.4 first). The most logical order to me would be doing #5.4 last, such that we can ensure with its completion that conditional validation is properly integrated with the violation processing put in place with #5.3.

If we implement #4.2 (valid if unchanged from original) and leave #5.4 to that other issue, then what's a scenario in which there can be a violation for an inaccessible field?

Same as you described in the issue summary - it does not change anything but that we determine which fields to validate based on change information instead access.
So the legitimate question becomes: Should we determined it based on access or changed information? I'm not so sure on that, but I must say that basing it on access information seems to be more solid to me, plus would also work with creation forms.

The plus of validating only changed values is that it allows editing of invalid entities by admins, i.e. even if they became invalid for some reason (field configuration changes, e.g. allowed integer range, required fields,...). But at the same time I'm not sure it's really legitimate to only validate only changed values: Maybe sometimes a non-change needs to be validated still? I cannot think of an example, but I'm still unsure. Also, it might end up being confusing to users if there are fields marked as required but not validated as such, while others are. Thus, I tend to think it's best to validate on #access and consider doing changed-only validation in another issue or at least enable contrib do it that way. So people running into issues with unfixable violations for admins could still use the contrib, which could even offer the functionality based on permissions.

fago’s picture

Issue summary: View changes

I quickly verified that changed validation is skipped for terms in forms. Adding this as reason to do this to the issue summary.

larowlan’s picture

Also confirming that moving the name and created validation logic from CommentForm results in the validation being skipped, because those fields don't use widgets - see #2403485: Complete conversion of comment form validation to entity validation

larowlan’s picture

How about if EntityFormDisplay also had an array of non-widget field constraints (Added at run-time by the building form)

CommentForm::buildForm could call

$this->getFormDisplay($form_state)->addFieldValidation('created', 'date', function() use ($form_state, $entity) {
  return $entity->created->access('edit') && $form_state->hasValue('date')) {
});

So the signature is

public function addFieldValidation($entity_field_name, $form_field_name = NULL, callable $applies);

Just throwing it out there

fago’s picture

Issue summary: View changes
fago’s picture

Since most child issues of #1696648: [META] Untie content entity validation from form validation got addressed meanwhile, I'll get started with this. I added the tasks proposed in #5 to the proposed solution.

However, what to do about violations that are reported for fields that aren't on the form? For example, if B reports the violation (since it's the one with the constraint), its violation message might not make sense (and might expose confidential information, such as that such a field exists) to a person who only sees what's on the form.

How to deal with the violation message intended for a different, not shown field is the remaining question and problem. We could just show a generic message that tells the editor that the change would invalidate another field, and if accessible, we could tell him also which field we'd invalidate.
I think situations like this are created by bad configuration and having a pointer to the field missing on the form and thus causing the problem, should be enough to point the site builder / admin to the root of the problem.

fago’s picture

Assigned: Unassigned » fago
fago’s picture

Issue summary: View changes

Created a separate issue for the first task: #2429037: Allow adding entity level constraints.

fago’s picture

Assigned: fago » Unassigned
fago’s picture

Work over form validation logic to run complete entity validation, ignore violations based on field access by default (make it overridable from the entity form?)

Thinking about how to do this, I came over two options:
- Run entity validation using groups and make each field a group. Pass the fields that should be validated as groups, then we should be able to skip validating fields which have no group set. However, I don't think we'd use groups as intended then as it should group constraints and not what to validated. Moreover, I think this might be impossible to implement in symfony validator 2.5 or later. For that reasons, I don't think this is a good option.
- Trigger a complete validation and filter violations by path only. That would work straight-forward, but could become bit problematic with combined/conditional validation. As for that to work one would have to specify the fields covered by a constraint anyway, we could read that information from the violation's constraint and handle it accordingly. Only problem here is that ConstraintViolation::getConstraint() isn't in any interface :/
- Instead of triggering a complete entity validate, we could manually validate each field that should be validated + additionally validate entity level constraints (without triggering validation of all child-fields). That would work fine for combined/conditional validation also, if we make sure they are assigned on entity level (what seems reasonable to me).

fago’s picture

Figured via https://github.com/symfony/symfony/issues/13748 that getConstraints() is not on the interface for BC reasons and already relied upon by Symfony - so I think we could rely on that also. As argued in https://github.com/symfony/symfony/issues/13748 I'd love to see it documented that way also though.

xjm’s picture

Issue tags: +Triaged D8 critical

This issue was discussed with @catch, @alexpott, @webchick, @xjm, and @effulgentsia on Feb. 5, and the branch maintainers confirmed that it is critical.

fago’s picture

Assigned: Unassigned » fago
FileSize
4.72 KB

Started working on this and created a first not-working draft, but it should help to show where I'm going.

The last option would require figuring out how we can run entity level validation without validating some fields what seems hard to solve in general. So I decided to go with variant "Trigger a complete validation and filter violations by path only" - this should result in an API which lets us not miss any violations.

My idea was to wrap resulting violations in a class which has handy methods for filtering violations as needed. See attached WIP.

fago’s picture

Status: Active » Needs work
FileSize
7.9 KB

New file including the class/interface.

larowlan’s picture

Looks great, has separation to allow each entity to do things differently.

Are you going to keep working on it? Happy to pick it up and kick it along.

larowlan’s picture

Issue summary: View changes

#2002180: Entity forms skip validation of fields that are edited without widgets was closed as a duplicate of this, updated issue summary.

fago’s picture

Status: Needs work » Needs review

yeah, I'm on it. Thanks, I'll unassign as soon as I stop. Updated patch.

This will skip node changed validation as long as #2429037: Allow adding entity level constraints is not done.

fago’s picture

FileSize
19.78 KB

Status: Needs review » Needs work

The last submitted patch, 25: entity_violations.patch, failed testing.

fago’s picture

I just talked this problem through with webmozart (symfony validator maintainer) what was very helpful. In a nutshell, he agrees that it's a good idea to use #17-2 and ConstraintViolation::getConstraint().

We also discussed adding secondary paths directly to the ConstraintViolation, what seems a viable improvement for composite constraints. But we could not use any upstream addition right now, and for customizing the violation class we'd have to add overwrite the ExecutionContext (i.e. replace the whole internal class and rely on the interface only). As the fields in question won't change and you have to write custom constraints for composite constraints anyway, adding the information of covered fields there only seems the better option now.

fago’s picture

Status: Needs work » Needs review
FileSize
8.42 KB

Restored EntityFormDisplay::validateFormValues() for BC, it's really simple now but I think it's fine to keep it. I documented when to use it or not. Also improved docs and worked on test fails.

Status: Needs review » Needs work

The last submitted patch, 28: entity_violations.patch, failed testing.

fago’s picture

Status: Needs work » Needs review
FileSize
22.98 KB
8.42 KB

Status: Needs review » Needs work

The last submitted patch, 30: entity_violations.patch, failed testing.

fago’s picture

Issue summary: View changes

So here is how I think we should make this work:

Scenario A:
Constraint is validating field A in dependence of field B. It shows the violation message on field A. If field B is hidden, the constraint is still validated. If validation fails, the message for field A is shown as error message.
Scenario B:
Constraint is validating field A in dependence of field B. It shows the violation message on field A. If field A is hidden, the constraint is still validated. If validation fails, the message (for the hidden field A) is not shown, but a general message is put on field B. E.g. if the field is accessible to the current user: "The provided value is invalid in the context of the current value of field %field-label.", if not accessible: "The provided value is invalid for the currently set values."

Suggestions for better messages welcome :) Added that to the issue summary.

fago’s picture

Issue summary: View changes
larowlan’s picture

Can you clarify the language around 'in dependence' do you mean 'depends on' or 'independent of'?
If you mean the former, I think the proposal makes sense.

fago’s picture

Assigned: fago » Unassigned

I mean it depends on. Sry if that's unclear, feel free to improve it.

> If you mean the former, I think the proposal makes sense.
good :)

Unassigning while not actively working on it.

martin107’s picture

Issue tags: +Needs reroll

Patch no longer applies, Needs reroll

cafuego’s picture

Assigned: Unassigned » cafuego
cafuego’s picture

Assigned: cafuego » Unassigned
Status: Needs work » Needs review
FileSize
16.12 KB
13.03 KB

Go go gadget testbot.

Status: Needs review » Needs work

The last submitted patch, 38: 2395831-entity-violations-38.patch, failed testing.

cafuego’s picture

Status: Needs work » Needs review
FileSize
22.91 KB
5.75 KB

Totaly forgot to git add the new class and interface files. Amended the patch and thus made a much smaller interdiff :-)

Status: Needs review » Needs work

The last submitted patch, 40: 2395831-entity-violations-40.patch, failed testing.

klausi’s picture

  1. +++ b/core/lib/Drupal/Core/Entity/EntityConstraintViolationList.php
    @@ -0,0 +1,146 @@
    +}
    \ No newline at end of file
    

    newline missing here.

  2. +++ b/core/lib/Drupal/Core/Entity/EntityConstraintViolationListInterface.php
    @@ -0,0 +1,81 @@
    +}
    \ No newline at end of file
    

    same here.

Otherwise the identified scenarios make sense to me, so let's write tests for them (unit tests?).

martin107’s picture

Status: Needs work » Needs review
FileSize
22.86 KB
1.28 KB

Just giving the EntityConstraintViolationListInterface a once over before it becomes set in stone for a few years.

EntityConstraintViolationListInterface::filterByFields()

takes an array of strings. We should enforce that the parameter is at least an array.

Also fixed the newline thing.

Status: Needs review » Needs work

The last submitted patch, 43: 2395831-entity-violations-42.patch, failed testing.

martin107’s picture

Status: Needs work » Needs review
FileSize
600 bytes
22.87 KB

Fixed the interface not the implementation Doh.

Status: Needs review » Needs work

The last submitted patch, 45: 2395831-entity-violations-45.patch, failed testing.

Wim Leers’s picture

Does this allow writing validation constraints that force e.g. fields A and B to be updated in tandem in some way? For example: field A is a boolean. Depending on the value of field A, a different set of values in field B is considered valid. In other words: modifying field A can cause a validation error on field B.

If the above is allowed, then this has the potential to break in-place editing, because in-place editing by definition happens on a per-field basis. So if modifying field A triggers a validation error on field B, then Quick Edit will need to be updated to support this.

fago’s picture

takes an array of strings. We should enforce that the parameter is at least an array.

yes, thanks!

Does this allow writing validation constraints that force e.g. fields A and B to be updated in tandem in some way?

Yes, but it's not allowed by this - it's fixes validation with this. We already have a situation like that for comments (see CommentNameConstraint) and imo it really is a typical use case we have to support.

If the above is allowed, then this has the potential to break in-place editing, because in-place editing by definition happens on a per-field basis

True, it would have to support submitting multiple changes as one change to make it work nicely - but I guess that would be a massive change. But even if it does not work with it, it just means that changing a single value might not allow you to change it as much as you could if you change the single value + the value it depends on. Not that much an issue imho. It might be awkward though if you do not have any option lefts you can change a single field to, for which it might be preferable to be able to disable QuickEdit for the field?

fago’s picture

As dawehner revealed in #2105797-38: Add CompositeConstraintBase so that constraints involving multiple fields, such as CommentNameConstraint, can be discovered, our current symfony 2.4 legacy validator does not add the constraint triggering a violation yet. That means we have to take care of #2343035: Upgrade validator integration for Symfony versions 2.5+ before being able to deal with composite constraints correctly.

Wim Leers’s picture

I just noticed that #2429037-36: Allow adding entity level constraints already updates Quick Edit to perform entity-level validation. Great!

But even if it does not work with it, it just means that changing a single value might not allow you to change it as much as you could if you change the single value + the value it depends on. Not that much an issue imho. It might be awkward though if you do not have any option lefts you can change a single field to, for which it might be preferable to be able to disable QuickEdit for the field?

That's exactly the problem, and more:

  1. modifying a field could be possible (there is another value that the user can choose), but it could trigger validation errors in multiple fields — that'd be quite the WTF UX-wise.
  2. another consequence of the above could be that an unmodified field becomes invalid, but that unmodified field is not visible (i.e. hidden in the EntityViewDisplay) — this would make it effectively impossible to modify
  3. you say that fields that don't have other valid values that the user can choose should perhaps not be made in-place editable (this alone has the potential to cause a terrible UX btw) but if we do that, then we have to do another thing that's beyond crazy: for every change the user makes, we have to recalculate whether the other in-place editable fields should remain in-place editable or not

So even if we make Quick Edit smart enough to not make fields in-place editable that don't allow for other values… then we'd have to do that dynamically. And we'd *have* to talk to the server after every request.

That's not only a nightmare in terms of code complexity and cognitive UX, it's an impossibility in terms of performance UX: we'd have to talk to the server after every change to see if fields should no longer be in-place editable. Combine that round trip requirement with a high-latency connection and you have a recipe for disaster.

Conclusion: the only way I see this work is to say "if you do conditional field validation, we don't support in-place editing". Sufficiently simple conditional field validation can work efficiently. But "conditional" implies "arbitrary logic defining the conditionality", and if that's the case, then we're in hell. D8 contrib could make simple conditionality work well. The question is: do we have enough metadata to know whether an entity uses "sufficiently simple" or "complex" conditional field validation? Can we add that metadata?

fago’s picture

another consequence of the above could be that an unmodified field becomes invalid, but that unmodified field is not visible (i.e. hidden in the EntityViewDisplay) — this would make it effectively impossible to modify

Yes, that can also happen with entity form displays with unfortunate configuration. We cannot avoid this, so it's up the site builder to make sure the config makes sense. In the case of QuickEdit, I think it does not matter much whether the changed field makes another field value invalid, or whether it's the another field making the changed value invalid? The resulting message will be different though (see scenarious).

for every change the user makes, we have to recalculate whether the other in-place editable fields should remain in-place editable or not

I was more thinking about an option to disable Quick Edit per field and view mode.

The question is: do we have enough metadata to know whether an entity uses "sufficiently simple" or "complex" conditional field validation? Can we add that metadata?

Yes, we'll have to have that metadata as it's required for handling composite validation correctly. #2105797: Add CompositeConstraintBase so that constraints involving multiple fields, such as CommentNameConstraint, can be discovered is about to introduce a special constraint for that which has the covered fields annotated. Thus, we know whether there is such a constraint defined or not.

Wim Leers’s picture

I was more thinking about an option to disable Quick Edit per field and view mode.

That needs to be automatic. Hence it needs to be inferred (and be inferrable).

Yes, we'll have to have that metadata […]

That's great :)

manningpete’s picture

Issue tags: -Needs reroll

Patch applies, no reroll necessary.

effulgentsia’s picture

Title: Entity forms skip validation of fields that are not in the EntityFormDisplay » [PP-3] Entity forms skip validation of fields that are not in the EntityFormDisplay
Status: Needs work » Postponed

From what I can tell, this is postponed on #2429037: Allow adding entity level constraints (in order to handle the EntityChanged constraint), which is itself postponed on #2346373: Data reference validation constraints are applied wrong.

And, this is also postponed on #2105797: Add CompositeConstraintBase so that constraints involving multiple fields, such as CommentNameConstraint, can be discovered (in order to handle the CommentName constraint).

That's a total of 3, so adding that to the issue title.

jibran’s picture

Title: [PP-3] Entity forms skip validation of fields that are not in the EntityFormDisplay » [PP-2] Entity forms skip validation of fields that are not in the EntityFormDisplay
YesCT’s picture

Issue summary: View changes

yes. one of the 3 issues this was postponed on got in: #2346373: Data reference validation constraints are applied wrong. (this explains the title change)

fago’s picture

Title: [PP-2] Entity forms skip validation of fields that are not in the EntityFormDisplay » [PP-3] Entity forms skip validation of fields that are not in the EntityFormDisplay
Issue summary: View changes
fago’s picture

Title: [PP-3] Entity forms skip validation of fields that are not in the EntityFormDisplay » [PP-2] Entity forms skip validation of fields that are not in the EntityFormDisplay
jibran’s picture

Title: [PP-2] Entity forms skip validation of fields that are not in the EntityFormDisplay » [PP-1] Entity forms skip validation of fields that are not in the EntityFormDisplay
dawehner’s picture

I really like the idea to wrap that specific logic in a new object.

  1. +++ b/core/lib/Drupal/Core/Entity/ContentEntityForm.php
    @@ -69,8 +69,11 @@ public function form(array $form, FormStateInterface $form_state) {
    +    /** @var ContentEntityInterface $entity */
    

    should be FQCN

  2. +++ b/core/lib/Drupal/Core/Entity/EntityConstraintViolationList.php
    @@ -0,0 +1,146 @@
    +            $this->violationsByField[$field_name][$offset] = $offset;
    

    huch, we don't store the actual violations? Maybe the variable name then confused me a bit.

  3. +++ b/core/lib/Drupal/Core/Entity/EntityConstraintViolationList.php
    @@ -0,0 +1,146 @@
    +        // Use parent implementation in order to avoid unnecessary rebuilds of
    +        // the violations by field array.
    +        parent::remove($offset);
    

    Its a bit unexpected that this modifies the raw data, especially in connection via add() we somehow loose all the information.

jibran’s picture

Title: [PP-1] Entity forms skip validation of fields that are not in the EntityFormDisplay » Entity forms skip validation of fields that are not in the EntityFormDisplay
Status: Postponed » Needs work
dawehner’s picture

Working on it.

dawehner’s picture

Status: Needs work » Needs review
FileSize
25.37 KB
4.81 KB

Just tried to get a little bit warmer with the patch. I'll review now. /me passed along to fago again.
I hate what I had to do here, but I don't see a better way, see long comment.

Status: Needs review » Needs work

The last submitted patch, 63: 2395831-63.patch, failed testing.

dawehner’s picture

Status: Needs work » Needs review
FileSize
26.52 KB
1.15 KB

Just a really minor improvement so far.

Status: Needs review » Needs work

The last submitted patch, 65: 2395831-65.patch, failed testing.

dawehner’s picture

Status: Needs work » Needs review
FileSize
29.98 KB
4.15 KB

This now could actually pass

Status: Needs review » Needs work

The last submitted patch, 67: 2395831-67.patch, failed testing.

dawehner’s picture

Status: Needs work » Needs review
FileSize
30.44 KB
848 bytes

Thanks to the fgm we figured out the remaining failures, yeah!

larowlan’s picture

Looking great - some comments below

  1. +++ b/core/lib/Drupal/Core/Entity/Display/EntityFormDisplayInterface.php
    @@ -101,10 +101,40 @@
    +   * This method invokes entity validation and takes care of flagging them on
    +   * the form. This method is in particular useful if the form consists of the
    +   * form display only.
    

    This sentence doesn't read well. Perhaps something like This is particularly useful when all elements on the form are managed by the form display ?

  2. +++ b/core/lib/Drupal/Core/Entity/Display/EntityFormDisplayInterface.php
    @@ -130,25 +157,25 @@ public function buildForm(FieldableEntityInterface $entity, array &$form, FormSt
    +   * This method processes all violations passed, thus any violations that it
    +   * should not handle should be processed before this method is invoked.
    

    Perhaps 'thus any violations that it does not handle should be processed before this method is invoked'?

  3. +++ b/core/lib/Drupal/Core/Entity/Display/EntityFormDisplayInterface.php
    @@ -130,25 +157,25 @@ public function buildForm(FieldableEntityInterface $entity, array &$form, FormSt
    +   * The method flags constraint violations related to fields shown on the
    +   * form as form errors on the correct form elements. Possibly pre-existing
    

    How about 'to fields shown in the form as errors on the correct elements'?

  4. +++ b/core/lib/Drupal/Core/Entity/Display/EntityFormDisplayInterface.php
    @@ -130,25 +157,25 @@ public function buildForm(FieldableEntityInterface $entity, array &$form, FormSt
    +   * violations of hidden fields (= fields not appearing in the display) are
    

    replace the = with i.e. ?

  5. +++ b/core/lib/Drupal/Core/Entity/Entity/EntityFormDisplay.php
    @@ -231,16 +235,65 @@ public function extractFormValues(FieldableEntityInterface $entity, array &$form
    +      // let the widget assign the violations to the right form elements.
    

    Can we switch the word right for the word correct instead? First read I thought it was something to do with position.

  6. +++ b/core/lib/Drupal/Core/Entity/Entity/EntityFormDisplay.php
    @@ -231,16 +235,65 @@ public function extractFormValues(FieldableEntityInterface $entity, array &$form
    +    // Flag all remaining violations; e.g., entity level violations.
    

    is that comma after e.g required?

  7. +++ b/core/lib/Drupal/Core/Entity/Entity/EntityFormDisplay.php
    @@ -231,16 +235,65 @@ public function extractFormValues(FieldableEntityInterface $entity, array &$form
    +      $form_state->setErrorByName(str_replace('.', '][', $violation->getPropertyPath()), $violation);
    

    setErrorByName also accepts an array of parents as the first parameter (no need for the kludgy 'foo][bar][puke' notation) - so we could just use explode here - or add a new method to $violation to return as such

  8. +++ b/core/lib/Drupal/Core/Entity/Entity/EntityFormDisplay.php
    @@ -231,16 +235,65 @@ public function extractFormValues(FieldableEntityInterface $entity, array &$form
    +      // changed to 0.value. Sadly constrains in symfony don't have setters so
    

    %s/constrains/constraints.

    %s/symfony/Symfony

  9. +++ b/core/lib/Drupal/Core/Entity/Entity/EntityFormDisplay.php
    @@ -231,16 +235,65 @@ public function extractFormValues(FieldableEntityInterface $entity, array &$form
    +      // getConstraint() and getCause(), so we have to rely on a specific
    

    Can we sub-class the interface now we control the validator? i.e. define our own interface that adds the required parts and have the Drupal-specific validator return the required objects?

  10. +++ b/core/lib/Drupal/Core/Entity/EntityConstraintViolationList.php
    @@ -0,0 +1,147 @@
    +      if (!$this->entity->get($field_name)->access('edit', $account)) {
    

    Does this need to pass cacheability upstream? Don't think so, but doesn't hurt to ask.

  11. +++ b/core/lib/Drupal/Core/Entity/EntityConstraintViolationListInterface.php
    @@ -0,0 +1,81 @@
    +   * Any violations for fields that are not accessible are removed and returned
    +   * for further processing.
    

    Not sure the 'and returned for for further processing' is correct here, it implies that the removed ones are returned, which isn't the case

  12. +++ b/core/modules/content_translation/src/ContentTranslationHandlerInterface.php
    @@ -8,6 +8,7 @@
    +use Drupal\Core\Entity\EntityTypeInterface;
    

    Unneeded?

  13. +++ b/core/modules/rest/src/Plugin/rest/resource/EntityResource.php
    @@ -209,6 +209,11 @@ public function delete(EntityInterface $entity) {
    +    // Remove violation of inaccessible fields as they cannot stem from our
    

    Should something similar be in POST/PUT/PATCH methods too?

  14. +++ b/core/modules/search/src/Tests/SearchTestBase.php
    @@ -7,6 +7,7 @@
    +use Drupal\Core\Url;
    

    Unused

dawehner’s picture

FileSize
31.55 KB
5.55 KB

Thank you for the review!

setErrorByName also accepts an array of parents as the first parameter (no need for the kludgy 'foo][bar][puke' notation) - so we could just use explode here - or add a new method to $violation to return as such

No it doesn't, see code and documentation. It works for some of the API method, but not here.

Can we sub-class the interface now we control the validator? i.e. define our own interface that adds the required parts and have the Drupal-specific validator return the required objects?

We could indeed ... do you think we should do that? We would have to reimplement the entirety of the constraint violation class.

Does this need to pass cacheability upstream? Don't think so, but doesn't hurt to ask.

Validation happens on POST requests, so we should not care about caching at all.

Should something similar be in POST/PUT/PATCH methods too?

Wait, this happens in the validate() method, which is called from within post() and patch().

jibran’s picture

dawehner’s picture

Status: Needs work » Needs review
FileSize
2.08 KB
29.48 KB

Thank you jibran!

klausi’s picture

Status: Needs review » Needs work
  1. +++ b/core/lib/Drupal/Core/Entity/Entity/EntityFormDisplay.php
    @@ -231,16 +235,65 @@ public function extractFormValues(FieldableEntityInterface $entity, array &$form
    +        $new_violation = new ConstraintViolation($violation->getMessage(), $violation->getMessageTemplate(), $violation->getMessageParameters(), $violation->getRoot(), $new_path, $violation->getInvalidValue(), $violation->getMessagePluralization(), $violation->getCode(), $violation->getConstraint(), $violation->getCause());
    

    Can we split this up into multiple lines? The amount of horizontal scrolling to read this is ... painful.

  2. +++ b/core/lib/Drupal/Core/Entity/EntityConstraintViolationListInterface.php
    @@ -0,0 +1,80 @@
    +  /**
    +   * The entity which has been validated.
    +   *
    +   * @return \Drupal\Core\Entity\ContentEntityInterface
    +   *   The entity object.
    +   */
    +  public function getEntity();
    

    why ContentEntityInterface? Shouldn't this be FieldableEntityInterface? Because that is what we type hint in EntityConstraintViolationList?

  3. +++ b/core/modules/comment/src/CommentForm.php
    @@ -314,24 +315,14 @@ public function buildEntity(array $form, FormStateInterface $form_state) {
    -    // Customly trigger validation of manually added fields and add in
    -    // violations.
    -    $violations = $comment->created->validate();
    -    foreach ($violations as $violation) {
    +  protected function flagViolations(EntityConstraintViolationListInterface $violations, array $form, FormStateInterface $form_state) {
    +    foreach ($violations->filterByField('created') as $violation) {
           $form_state->setErrorByName('date', $violation->getMessage());
         }
    -    $violations = $comment->validate();
    -    // Filter out violations for the name path.
    -    foreach ($violations as $violation) {
    -      if ($violation->getPropertyPath() === 'name') {
    -        $form_state->setErrorByName('name', $violation->getMessage());
    -      }
    +    foreach ($violations->filterByField('name') as $violation) {
    +      $form_state->setErrorByName('name', $violation->getMessage());
         }
    

    Looks like we lost the comment why we have to do special validation stuff for "created" and "name". Please add a comment back.

  4. +++ b/core/modules/content_translation/src/ContentTranslationHandler.php
    @@ -170,7 +169,6 @@ protected function hasPublishedStatus() {
       protected function hasChangedTime() {
    -    return is_subclass_of($this->entityType->getClass(), '\Drupal\Core\Entity\EntityChangedInterface');
       }
    

    So this function is empty now, why? The docs say it should return bool, not NULL? Is this function now obsolete? Please add a comment.

Otherwise looks almost ready!

dawehner’s picture

Status: Needs work » Needs review
FileSize
29.31 KB
3.21 KB

Can we split this up into multiple lines? The amount of horizontal scrolling to read this is ... painful.

I totally agree.

why ContentEntityInterface? Shouldn't this be FieldableEntityInterface? Because that is what we type hint in EntityConstraintViolationList?

So this function is empty now, why? The docs say it should return bool, not NULL? Is this function now obsolete? Please add a comment.

Oh I did not wanted to change that ...

Wim Leers’s picture

+++ b/core/modules/quickedit/src/Form/QuickEditFieldForm.php
@@ -158,18 +158,6 @@ public function validateForm(array &$form, FormStateInterface $form_state) {
     $form_state->get('form_display')->validateFormValues($entity, $form, $form_state);
-
-    // Run entity-level validation as well, while skipping validation of all
-    // fields. We can do so by fetching and validating the entity-level
-    // constraints manually.
-    // @todo: Improve this in https://www.drupal.org/node/2395831.
-    $typed_entity = $entity->getTypedData();
-    $violations = $this->validator
-      ->validate($typed_entity, $typed_entity->getConstraints());
-
-    foreach ($violations as $violation) {
-      $form_state->setErrorByName($violation->getPropertyPath(), $violation->getMessage());
-    }

Just to make sure I understand: this can now safely be removed because it happens in ::validateFormValues() already, always?

(Less code to maintain, yay!)

dawehner’s picture

Just to make sure I understand: this can now safely be removed because it happens in ::validateFormValues() already, always?

(Less code to maintain, yay!)

Good question. So before the form display iterated over each field and ran its validation. This issue changes that and runs the entire entity validation in order to be able to
support \Drupal\Core\Entity\Plugin\Validation\Constraint\CompositeConstraintBase, so we no longer need the validation of the entire entity in QuickEditFieldForm

klausi’s picture

Status: Needs review » Needs work
  1. +++ b/core/modules/content_translation/content_translation.module
    @@ -145,6 +145,13 @@ function content_translation_entity_type_alter(array &$entity_types) {
    +      // Sadly its not possible to use a translation handler here, as its part
    +      // of the entity type.
    +      if (is_subclass_of($entity_type->getClass(), '\Drupal\Core\Entity\EntityChangedInterface')) {
    +        $entity_type->addConstraint('EntityChanged', []);
    +      }
    

    I don't understand this comment. "its" should be "it is" here? Why would we use the translation handler here? But you have the entity type here? Did you mean to explain why this constraint cannot be in ContentTranslationHandler.php anymore? Because it is an entity level constraint and not a field level constraint maybe?

    Why do we have to add the EntityChanged constraint again here? It is already added in EntityType.php? So it gets added twice?

    So the comment should tell me:
    * Why we add the EntityChanged constraint twice to the entity type.
    * Why it is sad that we cannot add it in ContentTranslationHandler.php, maybe because ContentTranslationHandler::getFieldDefinitions() only deal with field level constraints and this is an entity level constraint?

  2. +++ b/core/modules/rest/src/Plugin/rest/resource/EntityResource.php
    @@ -209,6 +209,11 @@ public function delete(EntityInterface $entity) {
    +    // Remove violation of inaccessible fields as they cannot stem from our
    +    // changes.
    +    $violations->filterByFieldAccess();
    

    Should be "violations".

dawehner’s picture

Status: Needs work » Needs review
FileSize
29.04 KB
1.42 KB

Thank you for the review again.

Why do we have to add the EntityChanged constraint again here? It is already added in EntityType.php? So it gets added twice?

Oh yeah, you are absolute right. When I removed the code from the ContentTranslationHandler I thought I have to move it. Good pointer!

Should be "violations".

Fixed!

fago’s picture

Thanks for all the updates. Changes look mostely good to me. Improved some comments on the way -> updated patch attached.

+++ b/core/lib/Drupal/Core/Entity/Entity/EntityFormDisplay.php
@@ -231,16 +235,76 @@ public function extractFormValues(FieldableEntityInterface $entity, array &$form
+      else {
+        $new_violation = $violation;

hm, in that case the violation path is unprocessed what seems wrong. I guess we should just only take over constraint + cause if the ConstraintViolation class is used and else stick to the interface?

ad #60.2:

huch, we don't store the actual violations? Maybe the variable name then confused me a bit.

Right, stems from an old version. Agreed, let's rename - violationOffsetsByField ? A bit verbose but probably better.

ad #60.3:

Its a bit unexpected that this modifies the raw data, especially in connection via add() we somehow loose all the information.

Not sure what you mean here, do you mean that the whole method modifies data?

Just to make sure I understand: this can now safely be removed because it happens in ::validateFormValues() already, always?

Finally, yes ;-)

We could indeed ... do you think we should do that? We would have to reimplement the entirety of the constraint violation class.

Yeah, not sure what would buy us that also. The current situation is not ideal, but it's regular Symfony validator and is hopefully improved with 3.0. Those methods are not in the interface because of BC rules only. webmozart mentioned that he'd prefer to do away with the interface in the first place and stick to the class only as value object.

Otherwise looks almost ready!

Well, this still misses the main part to address #32 which was not possible before due to missing CompositeConstraintBase + add tests for that. Also see remaining tasks of the issue summary.

fago’s picture

Unfortunately I'm afk for the following 5 days (short holiday), however I can help with the remaining once I'm back!

dawehner’s picture

FileSize
28.8 KB
6.77 KB

Not sure what you mean here, do you mean that the whole method modifies data?

Yeah the fact that it removes entries from the list itself. I mean this method both changes state and returns value, I think this is always a bad sign.
But yeah let's just keep it.

and stick to the class only as value object.

Just curious, does that mean we will have setter methods?

For now this are just the renames / docs improvements. More coming later.

dawehner’s picture

FileSize
36.01 KB
7.21 KB

This for now just adds a failing test.

Status: Needs review » Needs work

The last submitted patch, 83: 2395831-83.patch, failed testing.

dawehner’s picture

Status: Needs work » Needs review
FileSize
37.44 KB
5.31 KB

Thanks to @Wim Leers we have a proper help message.

Wim Leers’s picture

Didn't find any problems, only nitpicks.

  1. +++ b/core/lib/Drupal/Core/Entity/ContentEntityForm.php
    @@ -86,6 +89,33 @@ public function validate(array $form, FormStateInterface $form_state) {
    +   *   A nested array form elements comprising the form.
    

    s/array form elements/array of form elements/

  2. +++ b/core/lib/Drupal/Core/Entity/Entity/EntityFormDisplay.php
    @@ -231,16 +235,77 @@ public function extractFormValues(FieldableEntityInterface $entity, array &$form
    +   *   A new constraint violation list with the changed propery path.
    

    s/propery/property/

  3. +++ b/core/lib/Drupal/Core/Entity/Entity/EntityFormDisplay.php
    @@ -231,16 +235,77 @@ public function extractFormValues(FieldableEntityInterface $entity, array &$form
    +      // All this code below is done in order to change the property path of the
    

    This sounds kinda strange. What about this?

    "All the logic below is necessary to change the property path of the"

  4. +++ b/core/lib/Drupal/Core/Entity/Entity/EntityFormDisplay.php
    @@ -231,16 +235,77 @@ public function extractFormValues(FieldableEntityInterface $entity, array &$form
    +      // changed to 0.value. Sadly constraints in symfony don't have setters so
    

    s/symfony/Symfony/

  5. +++ b/core/lib/Drupal/Core/Entity/EntityConstraintViolationListInterface.php
    @@ -0,0 +1,80 @@
    +   * @return \Symfony\Component\Validator\ConstraintViolationListInterface
    +   *   The list of violations being removed.
    ...
    +   * @return \Symfony\Component\Validator\ConstraintViolationListInterface
    +   *   The list of violations being removed.
    ...
    +   * @return \Symfony\Component\Validator\ConstraintViolationListInterface
    +   *   The list of violations being removed.
    

    Either missing a trailing [], or a wrong comment.

    (Twice.)

  6. +++ b/core/modules/comment/src/CommentForm.php
    @@ -314,24 +315,15 @@ public function buildEntity(array $form, FormStateInterface $form_state) {
    +    // Customly flag violations of fields not handled by the form display.
    

    "Customly" is not an actual adjective, but also can't think of an alternative at the moment.

  7. +++ b/core/modules/system/src/Tests/Entity/FieldWidgetConstraintValidatorTest.php
    @@ -57,4 +61,82 @@ public function testValidation() {
    +  public function testValidationWIthCompositeConstraint() {
    

    s/WIth/With/

  8. +++ b/core/modules/system/src/Tests/Entity/FieldWidgetConstraintValidatorTest.php
    @@ -57,4 +61,82 @@ public function testValidation() {
    +    // Provide an invalidate value for the name field.
    

    s/invalidate/invalid/

  9. +++ b/core/modules/system/src/Tests/Entity/FieldWidgetConstraintValidatorTest.php
    @@ -57,4 +61,82 @@ public function testValidation() {
    +    // from the field field to the second field and have a custom error message.
    

    s/field field/field/

  10. +++ b/core/modules/user/src/AccountForm.php
    @@ -348,23 +349,20 @@ public function buildEntity(array $form, FormStateInterface $form_state) {
    +    // Customly flag violations of fields not handled by the form display. This
    

    "Customly" again.

  11. +++ b/core/lib/Drupal/Core/Entity/Entity/EntityFormDisplay.php
    @@ -239,6 +244,22 @@ public function validateFormValues(FieldableEntityInterface $entity, array &$for
    +   * Returns whether the field is editable.
    +   * @param array $form
    

    Missing newline.

  12. +++ b/core/lib/Drupal/Core/Entity/Entity/EntityFormDisplay.php
    @@ -247,10 +268,25 @@ public function flagViolations(EntityConstraintViolationListInterface $violation
    +        // The field might not exist or the current user does not have access. As
    

    80 cols.

dawehner’s picture

FileSize
37.43 KB
6.45 KB

Thank you for the quick feedback!

"All the logic below is necessary to change the property path of the"

Thank you for the suggestion!

Either missing a trailing [], or a wrong comment.

Well, actually the interface we are using here is a list for itself, so this is perfectly fine for itself.
But well, maybe we have usecases of violation matrixes.

"Customly" is not an actual adjective, but also can't think of an alternative at the moment.

Went with manually.

jibran’s picture

Very nice work. Some doc improvements and suggestions.

  1. +++ b/core/lib/Drupal/Core/Entity/Entity/EntityFormDisplay.php
    @@ -231,16 +240,109 @@ public function extractFormValues(FieldableEntityInterface $entity, array &$form
    +   * Moves the property path to be relative to field level.
    

    Can we please explain the function little with some coding example maybe?

  2. +++ b/core/lib/Drupal/Core/Entity/EntityConstraintViolationList.php
    @@ -0,0 +1,147 @@
    +        if ($path = $violation->getPropertyPath()) {
    +          list($field_name) = explode('.', $path, 2);
    

    Can we add comment related to path here? title.0.value etc.

@dawehner what's remaining here other waiting for @fago? :) Patch is in pretty good shape IMHO.

dawehner’s picture

FileSize
37.49 KB
765 bytes

Can we add comment related to path here? title.0.value etc.

Good point!

Can we please explain the function little with some coding example maybe?

I don't get that, its a internal function and all you could do is to pass in a $field_name and a ConstraintViolationListInterface object.

@dawehner what's remaining here other waiting for @fago? :) Patch is in pretty good shape IMHO.

Well yeah, fago should really have a look at it and see whether I missed something, for example in terms of test coverage.

dawehner’s picture

Assigned: Unassigned » dawehner

In case someone wants to review, feel free to review, that would be great.

On the other hand this issue is, I think, now blocked also by final feedback from fago whether we cover now all possible cases.

jibran’s picture

Well I read the IS, remaining tasks and test coverage in the patch last time around and I think it is complete. Did you mean to assign this to @fago instead of yourself?

dawehner’s picture

Assigned: dawehner » fago

Ehem, yeah, that was my intention.

YesCT’s picture

Issue tags: +Needs reroll

I'm attempting a reroll since this does not apply.

YesCT’s picture

Issue tags: -Needs reroll
FileSize
37.47 KB

seems like just a conflict since #2428795: Translatable entity 'changed' timestamps are not working at all changed a line that this patch was deleting. rerolled.

fago’s picture

Thanks dawehner for taking care of the test!

  1. +++ b/core/lib/Drupal/Core/Entity/Entity/EntityFormDisplay.php
    @@ -231,16 +240,109 @@ public function extractFormValues(FieldableEntityInterface $entity, array &$form
    +   * Returns whether the field is editable.
    

    nope.

  2. +++ b/core/lib/Drupal/Core/Entity/Entity/EntityFormDisplay.php
    @@ -231,16 +240,109 @@ public function extractFormValues(FieldableEntityInterface $entity, array &$form
    +    if (($widget = $this->getRenderer($field_name)) && (!isset($form[$field_name]['#access']) || $form[$field_name]['#access'])) {
    

    This access check seems to be new, but I'm not sure why it has been introduced or why it would be necessary. ContentEntityForm::flagViolations() already filters violations by access and for the display it should be enough to respect visibility?

  3. +++ b/core/lib/Drupal/Core/Entity/Entity/EntityFormDisplay.php
    @@ -231,16 +240,109 @@ public function extractFormValues(FieldableEntityInterface $entity, array &$form
    +          if ($violation instanceof ConstraintViolation && $violation->getConstraint() instanceof CompositeConstraintBase && $covered_fields = $violation->getConstraint()->coversFields()) {
    

    hm, I'm not sure this should be handled in the display. Composite constraints are in general not related to the display, e.g. this could apply to separately handled base fields also. So if the failed field applies to a base field, there would be no way to map the violation. Thus, I think we need to find a way to deal with this as part of the logic in EntityConstraintViolationList.

Attached is an updated patch which addresses 1&2 - let's see whether it still works without also. Checking #3 now.

dawehner’s picture

I dont' remember exactly why, but the change was maybe somehow needed in order to fix the failures in https://www.drupal.org/node/2395831#comment-9919998

fago’s picture

hm, problem with 3 is that it really forces us to seperate filtering violations from flagging violations - because we need to know all filtered fields for being able to re-map violations before starting to flag. But that means we cannot keep the so far simple and natural API of

    // Manually flag violations of fields not handled by the form display.
    foreach ($violations->filterByField('created') as $violation) {
      $form_state->setErrorByName('date', $violation->getMessage());
    }
    foreach ($violations->filterByField('name') as $violation) {
      $form_state->setErrorByName('name', $violation->getMessage());
    }

Not sure how to whitelist fields like 'name' and 'created' best without introducing an unnatural "white-list" of fields yet.

Maybe a way to solve it would be to postpone the actual application of the violation - form element map to the last step until all information is set on the object? e.g. by having a ViolationsMapper and doing:

$mapper->assign('field', field')
$mapper->hide('field2')

Then, in the end we just apply the map while being able to treat composite constraints properly also.
edit: A mapper does not make sense as we have many objects (widgets) being part of the mapping process.

webchick’s picture

Drive-by partial review:

+    if ($entity->name->value === 'failure-field-name') {
+      $this->context->buildViolation('Name field violation')
+        ->atPath('name')
+        ->addViolation();
+    }
+    elseif ($entity->name->value === 'failure-field-type') {

That second $entity->name->value looks like a copy/pasta error?

plach’s picture

Issue summary: View changes
FileSize
15.08 KB

Mmh, copy/pasta

dawehner’s picture

Haha :)

The intention of that testcode:

+    if ($entity->name->value === 'failure-field-name') {
+      $this->context->buildViolation('Name field violation')
+        ->atPath('name')
+        ->addViolation();
+    }
+    elseif ($entity->name->value === 'failure-field-type') {
+      $this->context->buildViolation('Type field violation')
+        ->atPath('type')
+        ->addViolation();
+    }
   }

was to have a validation which add a violation to either the name field or the type field.

fago’s picture

ok, here is a new approach that introduces a new getEditedFieldNames() on ContentEntityForm in order to filter fields early enough. With that, customly added fields have to be referenced twice - but I do not see a way to avoid this - you have to tell us the field is there and you have to take care of flagging violations.
I've been wondering though whether we should keep the list of filtered field names in EntityConstraintViolationList and throw a LogicException if violations are retrieved via getByField() that have been filtered before? This would help if someone tries to flag violations for a field being filtered out, but would be problematic with fields filtered due to access.

Thoughts?

Status: Needs review » Needs work

The last submitted patch, 102: d8_entity_validate_4.patch, failed testing.

fago’s picture

Status: Needs work » Needs review
FileSize
1.69 KB
40.39 KB

Fixed entity level violations for EFD::validateFormValues().

fago’s picture

Assigned: fago » Unassigned
dawehner’s picture

+++ b/core/lib/Drupal/Core/Entity/ContentEntityForm.php
@@ -79,6 +79,12 @@ public function validate(array $form, FormStateInterface $form_state) {
+    // Remove violations of inaccessible fields and not edited fields.
+    $violations
+      ->filterByFieldAccess($this->currentUser())
+      ->filterByFields(array_diff(array_keys($entity->getFieldDefinitions()), $this->getEditedFieldNames($form_state)));

I really like this kind of code.

  1. +++ b/core/lib/Drupal/Core/Entity/ContentEntityForm.php
    @@ -75,8 +75,17 @@ public function form(array $form, FormStateInterface $form_state) {
       public function validate(array $form, FormStateInterface $form_state) {
    ...
    +    // Remove violations of inaccessible fields and not edited fields.
    +    $violations
    +      ->filterByFieldAccess($this->currentUser())
    +      ->filterByFields(array_diff(array_keys($entity->getFieldDefinitions()), $this->getEditedFieldNames($form_state)));
    +
    
    +++ b/core/lib/Drupal/Core/Entity/Entity/EntityFormDisplay.php
    @@ -231,16 +239,81 @@ public function extractFormValues(FieldableEntityInterface $entity, array &$form
       public function validateFormValues(FieldableEntityInterface $entity, array &$form, FormStateInterface $form_state) {
    

    The duplication between these two codepaths is a little bit odd, but I guess we can't do anything against it.

  2. +++ b/core/lib/Drupal/Core/Entity/EntityConstraintViolationList.php
    @@ -0,0 +1,215 @@
    +              t($message_template, $message_params),
    

    Let's use the StringTranslationTrait;

martin107’s picture

A few minor nudges in the right direction.

1) Move the optional parameter from EntityConstraintViolationList::__construct()
to the end of the parameter list and added associated @param tag.

2) Now using StringTranslationTrait.

3) A comment was going over the 80 char limit.

4) Removed errant @return type - the text exists already by definition of $violationOffsetsByField.

It is difficult to test that (1) has been consistently applied ... so tests might blow up ... but I will jump on it if it does.:)

Status: Needs review » Needs work

The last submitted patch, 107: d8_entity_validate_107.patch, failed testing.

martin107’s picture

> Repository checkout: failed to checkout

looks like testbot went sideways.... will try retesting in a while

dawehner’s picture

@martin107
Well, this was more about a question, not an actual proposal to change things in regards to point 1)

martin107’s picture

@dawehner

Forgive me I, don't understand.... by point 1 do you mean #107.1? Should the $violations parameter not be optional?

The last submitted patch, 107: d8_entity_validate_107.patch, failed testing.

martin107’s picture

Status: Needs work » Needs review
FileSize
40.41 KB
839 bytes

The fixes addresses what seems to be common to many failing tests... sorry for the distraction.

larowlan’s picture

Asking a few navel-gazing questions here after reviewing again from scratch.

  1. +++ b/core/lib/Drupal/Core/Entity/ContentEntityForm.php
    @@ -86,6 +95,53 @@ public function validate(array $form, FormStateInterface $form_state) {
    +   * foreach ($violations->getByField('name') as $violation) {
    +   *   $form_state->setErrorByName('name', $violation->getMessage());
    +   * }
    +   * parent::flagViolations($violations, $form, $form_state);
    

    Playing devil's advocate - with this approach it is possible for an implementer to forget to call the parent method. Would it more fool-proof for flagViolations to be private and have it call a protected flagAdditionalViolations method, the default implementation of which would be empty? That way if you wanted additional violations, you could implement that, but you couldn't forget the parent call?
    I know private isn't the norm in Drupal - this pattern just stood out to me based on some reading I've been doing on OO design. Feel free to say 'pipe down larowlan'.

  2. +++ b/core/lib/Drupal/Core/Entity/ContentEntityForm.php
    @@ -86,6 +95,53 @@ public function validate(array $form, FormStateInterface $form_state) {
    +  protected function getEditedFieldNames(FormStateInterface $form_state) {
    

    We could use the same approach here.

  3. +++ b/core/lib/Drupal/Core/Entity/Display/EntityFormDisplayInterface.php
    @@ -130,25 +158,26 @@ public function buildForm(FieldableEntityInterface $entity, array &$form, FormSt
    +  public function flagViolations(\Drupal\Core\Entity\EntityConstraintViolationListInterface $violations, array &$form, FormStateInterface $form_state);
    

    Any reason for using the fully qualified class name here?

  4. +++ b/core/lib/Drupal/Core/Entity/Entity/EntityFormDisplay.php
    @@ -7,11 +7,17 @@
    +use Drupal\Core\Entity\Plugin\Validation\Constraint\CompositeConstraintBase;
    

    Doesn't appear to be used?

  5. +++ b/core/lib/Drupal/Core/Entity/Entity/EntityFormDisplay.php
    @@ -231,16 +239,81 @@ public function extractFormValues(FieldableEntityInterface $entity, array &$form
    +    foreach ($violations->getViolatedFieldNames() as $field_name) {
    

    That sounds painful :-X - perhaps the word 'violated' is redundant here?

  6. +++ b/core/lib/Drupal/Core/Entity/Entity/EntityFormDisplay.php
    @@ -231,16 +239,81 @@ public function extractFormValues(FieldableEntityInterface $entity, array &$form
    +      if ($violation instanceof ConstraintViolation) {
    ...
    +      $new_violation = new ConstraintViolation(
    

    This tightly couples this class to ConstraintViolation. Do we have scope to invert the dependency and make the violations responsible for creating a clone of themselves with the new path via a new factory method or are we hamstrung there because vendor code?

  7. +++ b/core/lib/Drupal/Core/Entity/EntityConstraintViolationList.php
    @@ -0,0 +1,216 @@
    +        if ($violation->getConstraint() instanceof CompositeConstraintBase) {
    

    Again this couples EntityConstraintViolationList to CompositeConstraintBase, should|could we not embed this logic inside CompositeConstraintBase instead?

  8. +++ b/core/modules/user/src/AccountForm.php
    @@ -348,23 +349,35 @@ public function buildEntity(array $form, FormStateInterface $form_state) {
    +    $account->_skipProtectedUserFieldConstraint = $form_state->get('user_pass_reset');
    

    There is no other reference to this property in the patch, is this change still needed?

dawehner’s picture

This tightly couples this class to ConstraintViolation. Do we have scope to invert the dependency and make the violations responsible for creating a clone of themselves with the new path via a new factory method or are we hamstrung there because vendor code?

I think that is fine given that symfony 3 will not ship with the interface any longer anyway.

Wim Leers’s picture

Status: Needs review » Needs work

NW for #115.

fago’s picture

Status: Needs work » Needs review
FileSize
40.5 KB
3.92 KB

ad #115.1-2:
> That way if you wanted additional violations, you could implement that, but you couldn't forget the parent call?

Not sure about that:
- It's an uncommon pattern for d8 so far.
- It does not help if you want to override flagAdditionalViolations() - so it's not a "scalable" pattern.
- Having to call parents if you change something without risking to break the parents class logic is imho common.
Thus, I'd prefer to keep it as is - unless others would like to see the pattern also?

115.3,4,5 addressed

115.6:
We could change our ConstraintViolation class just in order to add a new creation helper there. But that creates a dependency on our custom class then, so not sure that is better at all.

115.7:
I'm not sure I agree here. Composite constraint violations just state the fields they cover. Making them responsible of adapting violations makes them aware of how entity violation lists are filtered - but that's the job of the EntityConstraintViolationList. However, we could add a helper for creating the new constraint violation over there - would that address your concern?

115.8:
yes, it's needed. it's moved from validate() to buildEntity().

dawehner’s picture

Not sure about that:
- It's an uncommon pattern for d8 so far.
- It does not help if you want to override flagAdditionalViolations() - so it's not a "scalable" pattern.
- Having to call parents if you change something without risking to break the parents class logic is imho common.
Thus, I'd prefer to keep it as is - unless others would like to see the pattern also?

Well, we are having such an approach for code which is inherited a lot of times, like Block plugins, so the question is, how often does someone extend that class ... Honestly I think its not that problematic to require the parent call.

I'm not sure I agree here. Composite constraint violations just state the fields they cover. Making them responsible of adapting violations makes them aware of how entity violation lists are filtered - but that's the job of the EntityConstraintViolationList. However, we could add a helper for creating the new constraint violation over there - would that address your concern?

Yeah I also consider the composite constraint violations as a really simple dump object

fago’s picture

Issue summary: View changes

Added list of API changes to the issue summary and updated remaining tasks (none).

webchick’s picture

Issue tags: +D8 Accelerate

Tagging.

dawehner’s picture

Status: Needs review » Reviewed & tested by the community

dawehner, reading up the "proposed resolution" of the issue - there is "Ensure all constraints are bound to fields (#4.1) only and document that restriction.".
dawehner, we have nothing done here, but I don't think we can enforce that. People can always do $field->getEntity()

That argument is totally fair, I mean, fields have a lot of power, we can't remove that :)

dawehner, so it means, it's fine as long as people are not doing composite constraints without using compositeconstraintbase

Given that I think the issue is done, the tight coupling to the ConstraintViolation is not an actual issue, as it will be the case anyway in the future.

jibran’s picture

Issue tags: +Needs change record

If all the remaining tasks are done then it's RTBC++. I think We need a change notice because of API change and user interface changes section.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 118: d8_entity_validate_4.patch, failed testing.

dawehner’s picture

Issue summary: View changes
Status: Needs work » Needs review
Issue tags: -Needs change record
FileSize
40.59 KB
1.14 KB

Adapted the user interface section, added a change record and fixed the remaining failures.

YesCT’s picture

interdiff looks good. directly addressing the fails from #118:

Message Group Filename Line Function Status
The Symfony\Component\Validator\ConstraintViolation::getMessageParameters method is deprecated since version 2.7, to be removed in 3.0. Use the ConstraintViolation::getParameters() method instead.

The Symfony\Component\Validator\ConstraintViolation::getMessagePluralization method is deprecated since version 2.7, to be removed in 3.0. Use the ConstraintViolation::getPlural() method instead.

[edit:] I did not look at the whole patch

plach’s picture

Status: Needs review » Needs work
FileSize
127.05 KB
  1. +++ b/core/lib/Drupal/Core/Entity/Display/EntityFormDisplayInterface.php
    @@ -130,25 +159,26 @@ public function buildForm(FieldableEntityInterface $entity, array &$form, FormSt
    +  public function flagViolations(EntityConstraintViolationListInterface $violations, array &$form, FormStateInterface $form_state);
    

    Shouldn't this be called flagFieldViolations()?

  2. +++ b/core/lib/Drupal/Core/Entity/EntityConstraintViolationList.php
    @@ -0,0 +1,216 @@
    +    return new static($this->entity,$violations);
    

    Missing space after comma

  3. +++ b/core/lib/Drupal/Core/Entity/EntityConstraintViolationList.php
    @@ -0,0 +1,216 @@
    +            $message_template = 'The validation failed because the value conflicts with the value in %field_name, which you cannot access.';
    ...
    +              $this->t($message_template, $message_params),
    

    This will break translatable string parsing.

  4. +++ b/core/lib/Drupal/Core/Entity/EntityConstraintViolationListInterface.php
    @@ -0,0 +1,98 @@
    +   * Gets violations flagged on entity level, not associated with any field.
    +   *
    +   * @return \Drupal\Core\Entity\EntityConstraintViolationListInterface
    +   *   A list of violations of the given fields.
    

    Description and return value text are confusing: are we dealing with fields or not?

  5. +++ b/core/lib/Drupal/Core/Entity/EntityConstraintViolationListInterface.php
    @@ -0,0 +1,98 @@
    +   * Violations for inaccessible fields are skipped and remaining violations
    +   * are returned.
    

    This is confusing: we are returning $this, so that means we are altering the internal state of current list before returning it, right? If so this could use a clarification, and filterByFields too.

    If I'm understanding correctly, PHP docs should make clear that filter* methods affect the list's internal state, while getBy* methods return a new list.

Btw, I manually tested this and I got a not so user-friendly error:

dawehner’s picture

Status: Needs work » Needs review
FileSize
44.35 KB
10.5 KB

Thank you plach!

Shouldn't this be called flagFieldViolations()?

What about using flagWidgetsErrorsFromViolations()?

Missing space after comma

Fixed

This will break translatable string parsing.

Sure, let's just copy the string value

Description and return value text are confusing: are we dealing with fields or not?

Yeah this is about entity level violations, clarified the docs ...

This is confusing: we are returning $this, so that means we are altering the internal state of current list before returning it, right? If so this could use a clarification, and filterByFields too.

If I'm understanding correctly, PHP docs should make clear that filter* methods affect the list's internal state, while getBy* methods return a new list.

I used now the following words: Filters this violation list by the given fields., I hope this makes it easier to understand?

Fixed the super weird error message ...

The last submitted patch, 125: 2395831-125.patch, failed testing.

plach’s picture

Status: Needs review » Reviewed & tested by the community

Looks good, thanks.

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
  1. +++ b/core/lib/Drupal/Core/Entity/Entity/EntityFormDisplay.php
    @@ -7,11 +7,16 @@
    +use Drupal\Core\StringTranslation\StringTranslationTrait;
    
    @@ -36,6 +41,8 @@
    +  use StringTranslationTrait;
    +
    

    Doesn't appear to be used by the new code.

  2. +++ b/core/lib/Drupal/Core/Entity/EntityConstraintViolationList.php
    @@ -0,0 +1,215 @@
    +  public function filterByFields(array $field_names) {
    ...
    +  public function filterByFieldAccess(AccountInterface $account = NULL) {
    

    Do we think we have adequate test coverage of these methods?

    In general it is hard to tell if we have enough test coverage with this patch.

  3. +++ b/core/modules/node/src/NodeForm.php
    @@ -285,19 +285,6 @@ protected function actions(array $form, FormStateInterface $form_state) {
    -  public function validate(array $form, FormStateInterface $form_state) {
    -    $node = parent::validate($form, $form_state);
    -
    -    if ($node->id() && (node_last_changed($node->id()) > $node->getChangedTimeAcrossTranslations())) {
    -      $form_state->setErrorByName('changed', $this->t('The content on this page has either been modified by another user, or you have already submitted modifications using this form. As a result, your changes cannot be saved.'));
    -    }
    -
    -    return $node;
    -  }
    

    Nice! I think this means we can remove node_last_changed() and its test.

  4. +++ b/core/modules/system/tests/modules/entity_test/src/Plugin/Validation/Constraint/EntityTestCompositeConstraintValidator.php
    @@ -7,6 +7,7 @@
    +use Drupal\Core\Entity\EntityTypeInterface;
    

    Not used.

dawehner’s picture

Thank you alex! Will work on the response

alexpott’s picture

Re #132.3 - http://www.drupalcontrib.org/api/drupal/drupal%21modules%21node%21node.m... not many usages in d7 contrib - 3 to be precise. Given the complexities of working out the changed time across translations I'd like to remove the function and get people to use the methods on the entity. We could choose to do the removal in a followup - which would make sense since it will require a change record.

dawehner’s picture

Status: Needs work » Needs review
FileSize
53.86 KB
10.53 KB

Added some unit test coverage ... I hope this is enough

plach’s picture

Status: Needs review » Active

Test coverage looks great, the usual nitpicks:

  1. +++ b/core/tests/Drupal/Tests/Core/Entity/EntityConstraintViolationListTest.php
    @@ -0,0 +1,165 @@
    +   * @covers ::filterByFields
    

    Not sure about unit tests, but we are missing the method description here and below.

  2. +++ b/core/tests/Drupal/Tests/Core/Entity/EntityConstraintViolationListTest.php
    @@ -0,0 +1,165 @@
    +   * Builds a entity constraint violation list without composite constraints.
    ...
    +   * Builds a entity constraint violation list with composite constraints.
    

    "an entity"

  3. +++ b/core/tests/Drupal/Tests/Core/Entity/EntityConstraintViolationListTest.php
    @@ -0,0 +1,165 @@
    +   * @return \Drupal\Core\Entity\EntityConstraintViolationList
    ...
    +   * @return \Drupal\Core\Entity\EntityConstraintViolationList
    

    Missing description

  4. +++ b/core/tests/Drupal/Tests/Core/Entity/EntityConstraintViolationListTest.php
    @@ -0,0 +1,165 @@
    +    $violations[] = new ConstraintViolation('test name violation', '', [], '', 'name', 'invalid');
    +    $violations[] = new ConstraintViolation('test name violation2', '', [], '', 'name', 'invalid');
    +
    +    $violations[] = new ConstraintViolation('test type violation', '', [], '', 'type', 'invalid');
    +    $violations[] = new ConstraintViolation('test type violation2', '', [], '', 'type', 'invalid');
    +    // Add two entity level specific violations.
    +    $violations[] = new ConstraintViolation('test entity violation', '', [], '', '', 'invalid');
    

    [Übernitpick award]
    I'd move the blank line below before the comment ;)

  5. +++ b/core/tests/Drupal/Tests/Core/Entity/EntityConstraintViolationListTest.php
    @@ -0,0 +1,165 @@
    +    // Add two entity level specific violations with a combound constraint.
    

    "compound"

plach’s picture

Status: Active » Needs work

oops

dawehner’s picture

Status: Needs work » Needs review
FileSize
32.61 KB
53.95 KB
2.86 KB
53.95 KB

Not sure about unit tests, but we are missing the method description here and below.

Yeah I actively, did not added them. The method name describes is properly already and we have used that sanity now in many places :)

I'd move the blank line below before the comment ;)

plach’s picture

Status: Needs review » Reviewed & tested by the community

Yay, an award, now my bio does really look good!

The last submitted patch, 137: 2395831-137.patch, failed testing.

fago’s picture

Thanks. Yeah, this had only indirect test coverage so far - nice to see a dedicated unit test added also. Other improvements look good also!

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs change record
+++ b/core/modules/node/node.module
@@ -738,28 +738,6 @@ function node_page_title(NodeInterface $node) {
-function node_last_changed($nid, $langcode = NULL) {
-  $node = \Drupal::entityManager()->getStorage('node')->loadUnchanged($nid);
-  $changed = $langcode ? $node->getTranslation($langcode)->getChangedTime() : $node->getChangedTimeAcrossTranslations();
-  return $changed ?: FALSE;
-}

We need a separate CR to document this removal. I documented why I thought this was permissable in beta in #133

dawehner’s picture

Status: Needs work » Reviewed & tested by the community
Issue tags: -Needs change record
alexpott’s picture

Status: Reviewed & tested by the community » Fixed

This issue addresses a critical bug and is allowed per https://www.drupal.org/core/beta-changes. Committed 068edfc and pushed to 8.0.x. Thanks!

  • alexpott committed 068edfc on 8.0.x
    Issue #2395831 by dawehner, fago, martin107, cafuego, YesCT, plach,...
yched’s picture

Congrats all, awesome patch ! Sorry for not being able to help more here :-/

I was (finally) looking at the patch when it got committed, so I posted my (very minor) remarks to a followup patch in #2501405: Doc adjustments after "Entity forms skip validation of fields that are not in the EntityFormDisplay". They were totally fine for a followup anyway.

The only thing that bugs me a bit is that we have kept EntityFormDisplay::validateFormValues(), which is mostly useless now, and blurs the waters a bit - when should I use it, when is validation performed by other means ?

Any chance we could remove it and present unambiguous API entry points for entity validation ?

fago’s picture

Great to see this one resolved!

ad EntityFormDisplay::validateFormValues()
Any chance we could remove it and present unambiguous API entry points for entity validation ?

It's useful when you just want to use the form display as entity form and embed that somewhere. If you have a regular entity form, you'd not use it. But in that case you'd extend an EntityForm base class so you are fine already. Imho, the docs on it tell that sufficiently.
Thus, I see it useful as helper serving common needs and it's used like that in core by QuickEdit. Not sure how the unambiguous alternative should look like?

They were totally fine for a followup anyway.

k, cu there!

yched’s picture

@fago #146 / EntityFormDisplay::validateFormValues()
I still find it a bit complex that we have "in this case you can do A but in that other case you have to do B instead" for a mission-critical task as entity/field validation, but yeah, I have no better proposal for cases like embedded forms and quickedit, other than letting them figure out the right validation & flag logic on their own - and that would *not* be a better proposal :-)

#2501405-5: Doc adjustments after "Entity forms skip validation of fields that are not in the EntityFormDisplay" tries to make the doc for EFD::validateFormValues() more explicit about when it can be used. Let me know if that works for you.

Status: Fixed » Closed (fixed)

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

amateescu’s picture

I'm not sure why support for flagging errors on form element for entity level constraints was dropped in #102, but I opened an issue to bring it back: #2894634: Allow entity-level validations to flag errors on form elements

quietone’s picture

Published the CR