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.
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

yched’s picture

Status: Active » Needs review
FileSize
4.13 KB

Patch for the above

dawehner’s picture

  1. +++ b/core/lib/Drupal/Core/Entity/Display/EntityFormDisplayInterface.php
    @@ -168,8 +168,7 @@ public function validateFormValues(FieldableEntityInterface $entity, array &$for
    -   * ignored. Other, non-field related violations are passed through and set as
    -   * form errors according to the property path of the violations.
    +   * ignored.
    

    Huh?

  2. +++ b/core/lib/Drupal/Core/Entity/EntityConstraintViolationListInterface.php
    @@ -77,12 +77,12 @@ public function filterByFields(array $field_names);
    -   * @return $this
    +   * @return static
        */
       public function filterByFieldAccess(AccountInterface $account = NULL);
     
    

    Tehnically its a $this, isn't it?

yched’s picture

@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

fago’s picture

but it seems the implementation doesn't do this, and only cares about field-related violations

Right - that changed :/ Thanks for catching it!

  1. +++ b/core/lib/Drupal/Core/Entity/EntityConstraintViolationListInterface.php
    @@ -33,7 +33,7 @@ public function getEntityViolations();
    -   * @return \Symfony\Component\Validator\ConstraintViolationListInterface
    +   * @return static
    

    Yep, that should be a ECVOLI as well.

  2. +++ b/core/lib/Drupal/Core/Entity/EntityConstraintViolationListInterface.php
    @@ -47,7 +47,7 @@ public function getByField($field_name);
    -   * @return \Drupal\Core\Entity\EntityConstraintViolationListInterface
    +   * @return static
    

    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.

yched’s picture

Regarding the phpdoc for EntityFormDisplayInterface::flagWidgetsErrorsFromViolations(), I'm not sure either what this paragraph means :

   * This method processes all violations passed, thus any violations not
   * related to fields of the form display must be processed before this method
   * is invoked.

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.

yched’s picture

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

fago’s picture

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

yched’s picture

Cool :-)

RTBC then ? Any feedback on the sentences I removed from EntityFormDisplayInterface::flagWidgetsErrorsFromViolations() ? (See item 1 in the IS)

mgifford’s picture

Version: 8.0.x-dev » 8.1.x-dev

Drupal 8.0.6 was released on April 6 and is the final bugfix release for the Drupal 8.0.x series. Drupal 8.0.x will not receive any further development aside from security fixes. Drupal 8.1.0-rc1 is now available and sites should prepare to update to 8.1.0.

Bug reports should be targeted against the 8.1.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.2.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.1.x-dev » 8.2.x-dev

Drupal 8.1.9 was released on September 7 and is the final bugfix release for the Drupal 8.1.x series. Drupal 8.1.x will not receive any further development aside from security fixes. Drupal 8.2.0-rc1 is now available and sites should prepare to upgrade to 8.2.0.

Bug reports should be targeted against the 8.2.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.3.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.2.x-dev » 8.3.x-dev

Drupal 8.2.6 was released on February 1, 2017 and is the final full bugfix release for the Drupal 8.2.x series. Drupal 8.2.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.3.0 on April 5, 2017. (Drupal 8.3.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.3.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.4.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.3.x-dev » 8.4.x-dev

Drupal 8.3.6 was released on August 2, 2017 and is the final full bugfix release for the Drupal 8.3.x series. Drupal 8.3.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.4.0 on October 4, 2017. (Drupal 8.4.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.4.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.5.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.4.x-dev » 8.5.x-dev

Drupal 8.4.4 was released on January 3, 2018 and is the final full bugfix release for the Drupal 8.4.x series. Drupal 8.4.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.5.0 on March 7, 2018. (Drupal 8.5.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.5.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.6.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.5.x-dev » 8.6.x-dev

Drupal 8.5.6 was released on August 1, 2018 and is the final bugfix release for the Drupal 8.5.x series. Drupal 8.5.x will not receive any further development aside from security fixes. Sites should prepare to update to 8.6.0 on September 5, 2018. (Drupal 8.6.0-rc1 is available for testing.)

Bug reports should be targeted against the 8.6.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.7.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.6.x-dev » 8.8.x-dev

Drupal 8.6.x will not receive any further development aside from security fixes. Bug reports should be targeted against the 8.8.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.9.x-dev branch. For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

Version: 8.8.x-dev » 8.9.x-dev

Drupal 8.8.7 was released on June 3, 2020 and is the final full bugfix release for the Drupal 8.8.x series. Drupal 8.8.x will not receive any further development aside from security fixes. Sites should prepare to update to Drupal 8.9.0 or Drupal 9.0.0 for ongoing support.

Bug reports should be targeted against the 8.9.x-dev branch from now on, and new development or disruptive changes should be targeted against the 9.1.x-dev branch. For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

Version: 8.9.x-dev » 9.2.x-dev

Drupal 8 is end-of-life as of November 17, 2021. There will not be further changes made to Drupal 8. Bugfixes are now made to the 9.3.x and higher branches only. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 9.2.x-dev » 9.3.x-dev

Version: 9.3.x-dev » 9.4.x-dev

Drupal 9.3.15 was released on June 1st, 2022 and is the final full bugfix release for the Drupal 9.3.x series. Drupal 9.3.x will not receive any further development aside from security fixes. Drupal 9 bug reports should be targeted for the 9.4.x-dev branch from now on, and new development or disruptive changes should be targeted for the 9.5.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

smustgrave’s picture

Believe it or not the patch in #9 still applies. Wonder if someone could take a look and see if it still make sense.

djsagar’s picture

The patch in #9 was applying but it was failing custom commands.
Fixed the CCF for #9 for 10.1.x.
Please review.

gaurav-mathur’s picture

Verified and tested patch #22. Patch applied successfully on drupal version 10.1.x

Version: 9.4.x-dev » 9.5.x-dev

Drupal 9.4.9 was released on December 7, 2022 and is the final full bugfix release for the Drupal 9.4.x series. Drupal 9.4.x will not receive any further development aside from security fixes. Drupal 9 bug reports should be targeted for the 9.5.x-dev branch from now on, and new development or disruptive changes should be targeted for the 10.1.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

joachim’s picture

Status: Needs review » Reviewed & tested by the community

LGTM.

@gaurav-mathur feel free to set an issue's status to RTBC if you think the patch is ok.

xjm’s picture

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

xjm’s picture

Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs subsystem maintainer review

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

  1. +++ b/core/lib/Drupal/Core/Entity/Display/EntityFormDisplayInterface.php
    @@ -119,16 +119,21 @@ public function buildForm(FieldableEntityInterface $entity, array &$form, FormSt
    +   * This method is a shortcut for:
    

    I don't understand what it means for the method to be a "shortcut".

    Do we mean:

    This method does two things:

    Or does "shortcut" mean something else in this context?

  2. +++ b/core/lib/Drupal/Core/Entity/Display/EntityFormDisplayInterface.php
    @@ -119,16 +119,21 @@ public function buildForm(FieldableEntityInterface $entity, array &$form, FormSt
    +   * When the form contains elements about the entity that are not managed by
    +   * the EntityFormDisplay, it should invoke entity validation directly, flag
    +   * the violations for those elements manually, and then call
    +   * \Drupal\Core\Entity\Display\EntityFormDisplayInterface::flagWidgetsErrorsFromViolations()
    +   * to let the EntityFormDisplay flag the violations on the fields it manages.
    +   * This latter flow is typically what ContentEntityForms use.
    

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

    When a form contains elements, this method should not be used. Instead, such forms (including typical ContentEntityForms) should:

    • invoke entity validation directly,
    • flag the violations for those elements manuallly,
    • and then call [method name] to let the EntityFormDisplay flag the violations on the fields it manages.

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

  3. +++ b/core/lib/Drupal/Core/Entity/Display/EntityFormDisplayInterface.php
    @@ -163,8 +168,7 @@ public function validateFormValues(FieldableEntityInterface $entity, array &$for
    -   * ignored. Other, non-field related violations are passed through and set as
    -   * form errors according to the property path of the violations.
    

    This sentence is listed in the IS as one to remove, so that's OK.

  4. +++ b/core/lib/Drupal/Core/Entity/Entity/EntityFormDisplay.php
    @@ -277,14 +276,14 @@ public function flagWidgetsErrorsFromViolations(EntityConstraintViolationListInt
    -  protected function movePropertyPathViolationsRelativeToField($field_name, ConstraintViolationListInterface $violations) {
    -    $new_violations = new ConstraintViolationList();
    +  protected function movePropertyPathViolationsRelativeToField($field_name, EntityConstraintViolationListInterface $violations) {
    

    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 a EntityConstraintViolationListInterface, and then change the typehint in a D11 followup.

  5. +++ b/core/lib/Drupal/Core/Entity/EntityConstraintViolationListInterface.php
    @@ -28,7 +28,7 @@ public function getEntityViolations();
    -   * @return \Symfony\Component\Validator\ConstraintViolationListInterface
    +   * @return static
        *   The violations of the given field.
    
    @@ -42,7 +42,7 @@ public function getByField($field_name);
    -   * @return \Drupal\Core\Entity\EntityConstraintViolationListInterface
    +   * @return static
    

    I'm not sure this is correct. This is the code for the main implementation of the method:

    public function getByField($field_name) {
      return $this
        ->getByFields([
        $field_name,
      ]);
    }

    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!

Version: 9.5.x-dev » 11.x-dev

Drupal core is moving towards using a “main” branch. As an interim step, a new 11.x branch has been opened, as Drupal.org infrastructure cannot currently fully support a branch named main. New developments and disruptive changes should now be targeted for the 11.x branch. For more information, see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.