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.
- Allow adding entity level constraints for general validation logic that should run for all changes (e.g. entity changed validation): #2429037: Allow adding entity level constraints
- Ensure all constraints are bound to fields (#4.1) only and document that restriction. (requires above task)
- 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?)
- #2105797: Add CompositeConstraintBase so that constraints involving multiple fields, such as CommentNameConstraint, can be discovered
- 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
- 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." - Write tests for the proper behavior of entity validation
- Use these tests to finalize a resolution for this issue (tests will inform implementation approach)
- Write patch to make tests pass (See proposed resolution)
API changes
EntityFormDisplayInterface::flagViolations()
has been addedFieldableEntityInterface::validate()
now returnsEntityConstraintViolationListInterface
instead ofViolationListInterface
(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
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. |
---|
Comment | File | Size | Author |
---|---|---|---|
#137 | 2395831-137.patch | 53.95 KB | dawehner |
#137 | interdiff.txt | 2.86 KB | dawehner |
#137 | 2395831-137.patch | 53.95 KB | dawehner |
#137 | DC8AEF3C7E_201564_83639_leaf_sized.png | 32.61 KB | dawehner |
#134 | interdiff.txt | 10.53 KB | dawehner |
Comments
Comment #1
effulgentsia CreditAttribution: effulgentsia commentedAlthough 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.
Comment #2
effulgentsia CreditAttribution: effulgentsia commentedComment #3
fagoImo, 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.
Comment #4
fagoAs 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?
Comment #5
fagoAction items based on my suggestion in #4:
Comment #6
effulgentsia CreditAttribution: effulgentsia commentedAs 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?
Comment #7
effulgentsia CreditAttribution: effulgentsia commentedIf 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?
Comment #8
fagoI 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.
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.
Comment #9
fagoI quickly verified that changed validation is skipped for terms in forms. Adding this as reason to do this to the issue summary.
Comment #10
larowlanAlso 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
Comment #11
larowlanHow about if EntityFormDisplay also had an array of non-widget field constraints (Added at run-time by the building form)
CommentForm::buildForm could call
So the signature is
Just throwing it out there
Comment #12
fagoComment #13
fagoSince 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.
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.
Comment #14
fagoComment #15
fagoCreated a separate issue for the first task: #2429037: Allow adding entity level constraints.
Comment #16
fagoWorking in #2429037: Allow adding entity level constraints
Comment #17
fagoThinking 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).
Comment #18
fagoFigured 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.
Comment #19
xjmThis issue was discussed with @catch, @alexpott, @webchick, @xjm, and @effulgentsia on Feb. 5, and the branch maintainers confirmed that it is critical.
Comment #20
fagoStarted 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.
Comment #21
fagoNew file including the class/interface.
Comment #22
larowlanLooks 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.
Comment #23
larowlan#2002180: Entity forms skip validation of fields that are edited without widgets was closed as a duplicate of this, updated issue summary.
Comment #24
fagoyeah, 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.
Comment #25
fagoComment #27
fagoI 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.
Comment #28
fagoRestored 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.
Comment #30
fagoComment #32
fagoSo 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.
Comment #33
fagoComment #34
larowlanCan 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.
Comment #35
fagoI 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.
Comment #36
martin107 CreditAttribution: martin107 commentedPatch no longer applies, Needs reroll
Comment #37
cafuego CreditAttribution: cafuego commentedComment #38
cafuego CreditAttribution: cafuego commentedGo go gadget testbot.
Comment #40
cafuego CreditAttribution: cafuego commentedTotaly forgot to git add the new class and interface files. Amended the patch and thus made a much smaller interdiff :-)
Comment #42
klausinewline missing here.
same here.
Otherwise the identified scenarios make sense to me, so let's write tests for them (unit tests?).
Comment #43
martin107 CreditAttribution: martin107 commentedJust 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.
Comment #45
martin107 CreditAttribution: martin107 commentedFixed the interface not the implementation Doh.
Comment #47
Wim LeersDoes 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.
Comment #48
fagoyes, thanks!
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.
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?
Comment #49
fagoAs 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.
Comment #50
Wim LeersI just noticed that #2429037-36: Allow adding entity level constraints already updates Quick Edit to perform entity-level validation. Great!
That's exactly the problem, and more:
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?
Comment #51
fagoYes, 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).
I was more thinking about an option to disable Quick Edit per field and view mode.
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.
Comment #52
Wim LeersThat needs to be automatic. Hence it needs to be inferred (and be inferrable).
That's great :)
Comment #53
manningpete CreditAttribution: manningpete commentedPatch applies, no reroll necessary.
Comment #54
effulgentsia CreditAttribution: effulgentsia commentedFrom 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.
Comment #55
jibranComment #56
YesCT CreditAttribution: YesCT commentedyes. one of the 3 issues this was postponed on got in: #2346373: Data reference validation constraints are applied wrong. (this explains the title change)
Comment #57
fagoChanged #2105797: Add CompositeConstraintBase so that constraints involving multiple fields, such as CommentNameConstraint, can be discovered to be postponed on #2343035: Upgrade validator integration for Symfony versions 2.5+, thus reflecting this here also. Back to 3 :-(
Comment #58
fago#2429037: Allow adding entity level constraints got committed.
Comment #59
jibran#2343035: Upgrade validator integration for Symfony versions 2.5+ got committed.
Comment #60
dawehnerI really like the idea to wrap that specific logic in a new object.
should be FQCN
huch, we don't store the actual violations? Maybe the variable name then confused me a bit.
Its a bit unexpected that this modifies the raw data, especially in connection via add() we somehow loose all the information.
Comment #61
jibran#2105797: Add CompositeConstraintBase so that constraints involving multiple fields, such as CommentNameConstraint, can be discovered got committed.
Comment #62
dawehnerWorking on it.
Comment #63
dawehnerJust 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.
Comment #65
dawehnerJust a really minor improvement so far.
Comment #67
dawehnerThis now could actually pass
Comment #69
dawehnerThanks to the fgm we figured out the remaining failures, yeah!
Comment #70
larowlanLooking great - some comments below
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
?Perhaps 'thus any violations that it does not handle should be processed before this method is invoked'?
How about 'to fields shown in the form as errors on the correct elements'?
replace the = with i.e. ?
Can we switch the word right for the word correct instead? First read I thought it was something to do with position.
is that comma after e.g required?
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
%s/constrains/constraints.
%s/symfony/Symfony
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?
Does this need to pass cacheability upstream? Don't think so, but doesn't hurt to ask.
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
Unneeded?
Should something similar be in POST/PUT/PATCH methods too?
Unused
Comment #71
dawehnerThank you for the review!
No it doesn't, see code and documentation. It works for some of the API method, but not here.
We could indeed ... do you think we should do that? We would have to reimplement the entirety of the constraint violation class.
Validation happens on POST requests, so we should not care about caching at all.
Wait, this happens in the validate() method, which is called from within post() and patch().
Comment #72
jibranIt includes #2486163: Split up .install files for schema, requirements and hook_update_N.
Comment #73
dawehnerThank you jibran!
Comment #74
klausiCan we split this up into multiple lines? The amount of horizontal scrolling to read this is ... painful.
why ContentEntityInterface? Shouldn't this be FieldableEntityInterface? Because that is what we type hint in EntityConstraintViolationList?
Looks like we lost the comment why we have to do special validation stuff for "created" and "name". Please add a comment back.
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!
Comment #75
dawehnerI totally agree.
Oh I did not wanted to change that ...
Comment #76
Wim LeersJust to make sure I understand: this can now safely be removed because it happens in
::validateFormValues()
already, always?(Less code to maintain, yay!)
Comment #77
dawehnerGood 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 inQuickEditFieldForm
Comment #78
klausiI 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?
Should be "violations".
Comment #79
dawehnerThank you for the review again.
Oh yeah, you are absolute right. When I removed the code from the ContentTranslationHandler I thought I have to move it. Good pointer!
Fixed!
Comment #80
fagoThanks for all the updates. Changes look mostely good to me. Improved some comments on the way -> updated patch attached.
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:
Right, stems from an old version. Agreed, let's rename - violationOffsetsByField ? A bit verbose but probably better.
ad #60.3:
Not sure what you mean here, do you mean that the whole method modifies data?
Finally, yes ;-)
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.
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.
Comment #81
fagoUnfortunately I'm afk for the following 5 days (short holiday), however I can help with the remaining once I'm back!
Comment #82
dawehnerYeah 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.
Just curious, does that mean we will have setter methods?
For now this are just the renames / docs improvements. More coming later.
Comment #83
dawehnerThis for now just adds a failing test.
Comment #85
dawehnerThanks to @Wim Leers we have a proper help message.
Comment #87
Wim LeersDidn't find any problems, only nitpicks.
s/array form elements/array of form elements/
s/propery/property/
This sounds kinda strange. What about this?
"All the logic below is necessary to change the property path of the"
s/symfony/Symfony/
Either missing a trailing
[]
, or a wrong comment.(Twice.)
"Customly" is not an actual adjective, but also can't think of an alternative at the moment.
s/WIth/With/
s/invalidate/invalid/
s/field field/field/
"Customly" again.
Missing newline.
80 cols.
Comment #88
dawehnerThank you for the quick feedback!
Thank you for the suggestion!
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.
Went with manually.
Comment #89
jibranVery nice work. Some doc improvements and suggestions.
Can we please explain the function little with some coding example maybe?
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.
Comment #90
dawehnerGood point!
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.Well yeah, fago should really have a look at it and see whether I missed something, for example in terms of test coverage.
Comment #91
dawehnerIn 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.
Comment #92
jibranWell 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?
Comment #93
dawehnerEhem, yeah, that was my intention.
Comment #94
YesCT CreditAttribution: YesCT commentedI'm attempting a reroll since this does not apply.
Comment #95
YesCT CreditAttribution: YesCT commentedseems like just a conflict since #2428795: Translatable entity 'changed' timestamps are not working at all changed a line that this patch was deleting. rerolled.
Comment #96
fagoThanks dawehner for taking care of the test!
nope.
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?
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.
Comment #97
dawehnerI 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
Comment #98
fagohm, 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
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.
Comment #99
webchickDrive-by partial review:
That second $entity->name->value looks like a copy/pasta error?
Comment #100
plachMmh, copy/pasta
Comment #101
dawehnerHaha :)
The intention of that testcode:
was to have a validation which add a violation to either the name field or the type field.
Comment #102
fagook, 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 aLogicException
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?
Comment #104
fagoFixed entity level violations for EFD::validateFormValues().
Comment #105
fagoAlso ran over a small not directly related docu problem: #2492835: Documented type for WidgetBaseInterface::flagErrors() parameter $violations is wrong
Comment #106
dawehnerI really like this kind of code.
The duplication between these two codepaths is a little bit odd, but I guess we can't do anything against it.
Let's use the StringTranslationTrait;
Comment #107
martin107 CreditAttribution: martin107 commentedA 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.:)
Comment #109
martin107 CreditAttribution: martin107 commented> Repository checkout: failed to checkout
looks like testbot went sideways.... will try retesting in a while
Comment #110
dawehner@martin107
Well, this was more about a question, not an actual proposal to change things in regards to point 1)
Comment #111
martin107 CreditAttribution: martin107 commented@dawehner
Forgive me I, don't understand.... by point 1 do you mean #107.1? Should the $violations parameter not be optional?
Comment #114
martin107 CreditAttribution: martin107 commentedThe fixes addresses what seems to be common to many failing tests... sorry for the distraction.
Comment #115
larowlanAsking a few navel-gazing questions here after reviewing again from scratch.
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'.
We could use the same approach here.
Any reason for using the fully qualified class name here?
Doesn't appear to be used?
That sounds painful :-X - perhaps the word 'violated' is redundant here?
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?
Again this couples EntityConstraintViolationList to CompositeConstraintBase, should|could we not embed this logic inside CompositeConstraintBase instead?
There is no other reference to this property in the patch, is this change still needed?
Comment #116
dawehnerI think that is fine given that symfony 3 will not ship with the interface any longer anyway.
Comment #117
Wim LeersNW for #115.
Comment #118
fagoad #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().
Comment #119
dawehnerWell, 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.
Yeah I also consider the composite constraint violations as a really simple dump object
Comment #120
fagoAdded list of API changes to the issue summary and updated remaining tasks (none).
Comment #121
webchickTagging.
Comment #122
dawehnerThat argument is totally fair, I mean, fields have a lot of power, we can't remove that :)
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.
Comment #123
jibranIf 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.
Comment #125
dawehnerAdapted the user interface section, added a change record and fixed the remaining failures.
Comment #126
YesCT CreditAttribution: YesCT commentedinterdiff 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
Comment #127
plachShouldn't this be called
flagFieldViolations()
?Missing space after comma
This will break translatable string parsing.
Description and return value text are confusing: are we dealing with fields or not?
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, whilegetBy*
methods return a new list.Btw, I manually tested this and I got a not so user-friendly error:
Comment #128
dawehnerThank you plach!
What about using
flagWidgetsErrorsFromViolations()
?Fixed
Sure, let's just copy the string value
Yeah this is about entity level violations, clarified the docs ...
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 ...
Comment #130
plachLooks good, thanks.
Comment #131
alexpottDoesn't appear to be used by the new code.
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.
Nice! I think this means we can remove node_last_changed() and its test.
Not used.
Comment #132
dawehnerThank you alex! Will work on the response
Comment #133
alexpottRe #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.
Comment #134
dawehnerAdded some unit test coverage ... I hope this is enough
Comment #135
plachTest coverage looks great, the usual nitpicks:
Not sure about unit tests, but we are missing the method description here and below.
"an entity"
Missing description
[Übernitpick award]
I'd move the blank line below before the comment ;)
"compound"
Comment #136
plachoops
Comment #137
dawehnerYeah I actively, did not added them. The method name describes is properly already and we have used that sanity now in many places :)
Comment #138
plachYay, an award, now my bio does really look good!
Comment #140
fagoThanks. Yeah, this had only indirect test coverage so far - nice to see a dedicated unit test added also. Other improvements look good also!
Comment #141
alexpottWe need a separate CR to document this removal. I documented why I thought this was permissable in beta in #133
Comment #142
dawehnerThere we go: https://www.drupal.org/node/2501343
Comment #143
alexpottThis 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!
Comment #145
yched CreditAttribution: yched commentedCongrats 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 ?
Comment #146
fagoGreat to see this one resolved!
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?
k, cu there!
Comment #147
yched CreditAttribution: yched commented@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.
Comment #149
amateescu CreditAttribution: amateescu for Pfizer, Inc. commentedI'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
Comment #150
quietone CreditAttribution: quietone at PreviousNext commentedPublished the CR