A couple adjustments after #2395831: Entity forms skip validation of fields that are not in the EntityFormDisplay, that got committed while I was reviewing it (well, I was very late to the party :-/)
Only minor stuff, mostly doc :
-
The phpdoc for EntityFormDisplayInterface::flagWidgetsErrorsFromViolations() states :
"Other, non-field related violations are passed through and set as form errors according to the property path of the violations"
but it seems the implementation doesn't do this, and only cares about field-related violations -
EntityFormDisplay::movePropertyPathViolationsRelativeToField()
The phpdoc and typehint says it receives a EntityConstraintViolationListInterface, which is technically true, but the whole entity validate chain works with the more business-specific EntityConstraintViolationListInterface, so we could avoid breaking that mental model ?
Likewise, the method could return an EntityConstraintViolationListInterface rather than the broader parent ?(Feel free to shoot that one down)
- EntityConstraintViolationListInterface::getByField() doc should say it returns an EntityConstraintViolationListInterface, like the other methods ? And actually, don't we use "@return static" for those cases ?
-
EntityConstraintViolationListInterface::getFieldNames() phpdoc :
nitpick : s/violated fields/fields that have violations/ - maybe it's just me, but "violated fields" feels a bit loaded.
Comment | File | Size | Author |
---|---|---|---|
#22 | 2501405-22.patch | 6.11 KB | djsagar |
| |||
#22 | interdiff_9-22.txt | 606 bytes | djsagar |
#9 | 2501405_validation_2395831_followup-5.patch | 5.77 KB | mgifford |
#5 | interdiff.txt | 2.78 KB | yched |
#5 | 2501405_validation_2395831_followup-5.patch | 5.77 KB | yched |
Comments
Comment #1
yched CreditAttribution: yched as a volunteer commentedPatch for the above
Comment #2
dawehnerHuh?
Tehnically its a $this, isn't it?
Comment #3
yched CreditAttribution: yched as a volunteer commented@dawehner :
1. See item 1 in the IS. This doc is saying things that, AFAICT, the code doesn't actually do.
2. True, that was unintended
Comment #4
fagoRight - that changed :/ Thanks for catching it!
Yep, that should be a ECVOLI as well.
I'm not sure on that. The contract should only be that the new list implements the same interface, but not necessarily the same class the original one - right? I do not see a reason to care about the specific implementation here.
Comment #5
yched CreditAttribution: yched as a volunteer commentedRegarding the phpdoc for EntityFormDisplayInterface::flagWidgetsErrorsFromViolations(), I'm not sure either what this paragraph means :
AFAICT the implementation only cares about errors on widgets in the display, the others are just ignored.
I removed it as well, but I might very well be missing something, so feedback welcome :-)
Also, tried to clarify the cases when the (now seldom used) EntityFormDisplayInterface::validateFormValues() is applicable, after @fago's explanation in #2395831-146: Entity forms skip validation of fields that are not in the EntityFormDisplay
This also fixes a doc bug, EntityFormDisplayInterface::flagViolations() doesn't exist, it's flagWidgetsErrorsFromViolations() in the final patch that got in.
Comment #6
yched CreditAttribution: yched as a volunteer commented@fago :
#4.2 :
The pasted diff is not clear on which exact method you're talking about :-)
But in the hypothetical case where someone writes a new class implementing EntityConstraintViolationListInterface with special features, it seems reasonable to expect that the interface methods that "produce another list based on some primary list" guarantee the new list has the same special features than the one you started from ?
Comment #7
fagohm, thinking about classes I'd say no - the interface is the only thing that should count. But the special features could be in an extended interface also, so yeah - I guess static makes sense. I'm convinced :)
Comment #8
yched CreditAttribution: yched as a volunteer commentedCool :-)
RTBC then ? Any feedback on the sentences I removed from EntityFormDisplayInterface::flagWidgetsErrorsFromViolations() ? (See item 1 in the IS)
Comment #9
mgiffordRe-uploading patch for the bots.
Comment #21
smustgrave CreditAttribution: smustgrave at Mobomo commentedBelieve it or not the patch in #9 still applies. Wonder if someone could take a look and see if it still make sense.
Comment #22
djsagar CreditAttribution: djsagar at OpenSense Labs commentedThe patch in #9 was applying but it was failing custom commands.
Fixed the CCF for #9 for 10.1.x.
Please review.
Comment #23
gaurav-mathur CreditAttribution: gaurav-mathur at Dotsquares Ltd. commentedVerified and tested patch #22. Patch applied successfully on drupal version 10.1.x
Comment #25
joachim CreditAttribution: joachim as a volunteer commentedLGTM.
@gaurav-mathur feel free to set an issue's status to RTBC if you think the patch is ok.
Comment #26
xjmThat said, @gaurav-mathur, a patch applying is not criteria for it to be RTBC (nor worth commenting about since the testing infrastructure already tells us if it applies), and there is nothing to really "test" here since it is a documentation patch.
The information we need from a reviewer for a docs patch is for the reviewer to read the documentation, ensure it is accurate by studying the documented API, and make sure it is clear without formatting or grammatical errors.
Comment #27
xjmThanks for working on this docs issue; it's good to see an issue of @yched's from back in the day finally getting close to completion. :)
I don't understand what it means for the method to be a "shortcut".
Do we mean:
Or does "shortcut" mean something else in this context?
"It" in this section could refer to the form, or the method, or
EntityFormDisplay
. Also, is this saying that the method shouldn't be used in the circumstances described? Maybe:(Where I've used a bulleted list above, use
-
lists, indented correctly with whitespace.)Again, only make this change if that's actually all true. I'm not sure if it is or not from reading the patch. A good docs reviewer will look up the API methods involved, their implementations, usages, etc., and document in the issue comment how the docs were verified.
This sentence is listed in the IS as one to remove, so that's OK.
This is, technically, a BC break. We need to do this with a deprecation that throws a
@trigger_error()
in the standard format if the passed value is not aEntityConstraintViolationListInterface
, and then change the typehint in a D11 followup.I'm not sure this is correct. This is the code for the main implementation of the method:
If it were a
@return static
, I would expect it to return$this
, not a new object of the same type. I'm actually not sure though; anyone know one way or the other, or have a reference?We might need subsystem maintainer review here to verify the accuracy of the docs. However, in the meanwhile, I encourage anyone interested in working on it to study the API closely so they can provide feedback on whether or not the docs are correct. Document the specifics of how you checked the accuracy of the documentation. Thanks!