Bug #50585

Validation fails if nested properties of same type are present

Added by Martin Lipp about 2 years ago. Updated about 1 year ago.

Status:Resolved Start date:2013-07-31
Priority:Must have Due date:
Assigned To:Bastian Waidelich % Done:

100%
Category:Validation
Target version:-
PHP Version: Complexity:
Has patch:No Affected Flow version:Flow 2.0.0

Description

The GenericObjectValidator dumps all prior calculated validation results, when validating a nested property which has the same class name as the validated object.

Quick example:

class Office {
    /**
     * @var string
     * @Flow\Validate(type="StringLength", options={ "minimum"=3, "maximum"=100 })
     * @Flow\Validate(type="NotEmpty")
     * @ORM\Column(length=100)
     */
    protected $title;

    /**
     * @var \Doctrine\Common\Collections\Collection<\Company\Project\Domain\Model\OpeningHours>
     * @ORM\OneToMany(mappedBy="office")
     */
    protected $openingHours;

    ...

class OpeningHours {
    /**
     * @var \Company\Project\Domain\Model\Office
     * @ORM\ManyToOne(inversedBy="openingHours")
     */
    protected $office;

    ...

When creating a new Office with a form including one ore more openingHours, all validation errors of properties defined prior to $openingHours are dumped and therefore not displayed as errors. If no other validation error after the $openingHours property occurs, the createAction is executed with no error messages (but not persisted, seems that the validation here works). In the above example an empty $title would pass the validation.

I've tracked it down to the GenericObjectValidator and the ValidatorResolver:
The ValidatorResolver only creates a new baseValidatorConjunction, if none exists for this class name. Otherwise it uses the existing one. Since the GenericObjectValidator's validate() method resets its member $this->result on every call, all results of prior Office properties are deleted as soon as it comes to the validation of the openingHours' $office property.
This does not happen in one of my older projects (with FLOW3 1.1), because there the GenericObjectValidator does not store its results in a member variable, but only calculates it inside the method.
Also it only seems to happen inside collections.

Related issues

related to TYPO3.Flow - Task #46340: Improve validation to speed up Flow Resolved 2013-03-15

Associated revisions

Revision 09756f36
Added by Helmut Hummel almost 2 years ago

[BUGFIX] Fix validation if nested properties of same type are present

The performance optimization in #46340
changed the isValid() method of the
GenericObjectValidator to use the result
property while recursing through the properties
of the to be validated object.

Since GenericObjectValidator is reused, it could
be the case that this result set is reset to
empty value.

Revert this change and set the result property
after all properties of the to be validated
object are validated.

Resolves: #50585
Related: #46340
Releases: 2.0, master
Change-Id: I178ac02e7bcfc9748b9da4a74f51314a34717e66

Revision a6e5f720
Added by Helmut Hummel over 1 year ago

[BUGFIX] Fix validation if nested properties of same type are present

The performance optimization in #46340
changed the isValid() method of the
GenericObjectValidator to use the result
property while recursing through the properties
of the to be validated object.

Since GenericObjectValidator is reused, it could
be the case that this result set is reset to
empty value.

Revert this change and set the result property
after all properties of the to be validated
object are validated.

Resolves: #50585
Related: #46340
Releases: 2.0, master
Change-Id: I178ac02e7bcfc9748b9da4a74f51314a34717e66

History

#1 Updated by Martin Lipp almost 2 years ago

I was able to reproduce the bug in the TYPO3.Blog package. Simply add the following form elements to the Post/New.html form:

<f:form.textfield id="comments.0.author" property="comments.0.author" />
<f:form.textfield id="comments.0.emailaddress" property="comments.0.emailAddress" />
<f:form.textarea id="comments.0.content" property="comments.0.content" rows="10" cols="50"/>

Now you are able to add one comment directly when creating a post. When submitting the empty form, only the validation errors for the comment are shown. No errors for the post's title, post's author, etc. are displayed anymore. This is because the models are of the same structure as in my above example (Office being Post, and OpeningHours being Comment). Filling the form elements for the comment only, the validation passes and a new Post is created (with an empty title, no content text, ...).

A possible solution would be, to skip the validation of the $post property in the Comment model, if and only if the Comment is created as a nested property of a collection. I'm guessing this could be achieved by checking the mappedBy annotation of the Post's $comments property and skipping the property in Comment which is defined by the mappedBy annotation. This should be safe, since $post is just a back reference to the currently created Post (where the validation happens anyway).

#2 Updated by Bastian Waidelich almost 2 years ago

  • Status changed from New to Accepted
  • Assigned To set to Bastian Waidelich

This is a severe bug and I can reproduce it.

#3 Updated by Helmut Hummel almost 2 years ago

Martin Lipp wrote:

This does not happen in one of my older projects (with FLOW3 1.1), because there the GenericObjectValidator does not store its results in a member variable, but only calculates it inside the method.

Are you sure you mean FLOW3 1.1 and not 1.0?
The GenericObjectValidator looks quite similar in 1.1 and 2.0

#4 Updated by Martin Lipp almost 2 years ago

Helmut Hummel wrote:

Martin Lipp wrote:

This does not happen in one of my older projects (with FLOW3 1.1), because there the GenericObjectValidator does not store its results in a member variable, but only calculates it inside the method.

Are you sure you mean FLOW3 1.1 and not 1.0?
The GenericObjectValidator looks quite similar in 1.1 and 2.0

Yes. I checked again it works fine with FLOW3 1.1. I also quickly migrated the package to Flow 2.0, where the bug appears immediately. But you're right there is no difference in the GenericObjectValidator (don't know what I saw back then). The bug must be somewhere else, although it is true that - in many cases - $this->results is not empty when entering the validate() method, where it is overwritten with an empty result.

#5 Updated by Gerrit Code Review almost 2 years ago

  • Status changed from Accepted to Under Review

Patch set 1 for branch master has been pushed to the review server.
It is available at https://review.typo3.org/24218

#6 Updated by Gerrit Code Review almost 2 years ago

Patch set 2 for branch master has been pushed to the review server.
It is available at https://review.typo3.org/24218

#7 Updated by Helmut Hummel almost 2 years ago

Martin Lipp wrote:

Yes. I checked again it works fine with FLOW3 1.1. I also quickly migrated the package to Flow 2.0, where the bug appears immediately. But you're right there is no difference in the GenericObjectValidator (don't know what I saw back then). The bug must be somewhere else, although it is true that - in many cases - $this->results is not empty when entering the validate() method, where it is overwritten with an empty result.

Can you check if the patch in Gerrit works for you? Thanks.

#8 Updated by Helmut Hummel almost 2 years ago

Breaking change was in #46340

#9 Updated by Gerrit Code Review almost 2 years ago

Patch set 3 for branch master has been pushed to the review server.
It is available at https://review.typo3.org/24218

#10 Updated by Martin Lipp almost 2 years ago

Helmut Hummel wrote:

Martin Lipp wrote:

Yes. I checked again it works fine with FLOW3 1.1. I also quickly migrated the package to Flow 2.0, where the bug appears immediately. But you're right there is no difference in the GenericObjectValidator (don't know what I saw back then). The bug must be somewhere else, although it is true that - in many cases - $this->results is not empty when entering the validate() method, where it is overwritten with an empty result.

Can you check if the patch in Gerrit works for you? Thanks.

It works (at least patch 2 does).

#11 Updated by Gerrit Code Review almost 2 years ago

Patch set 4 for branch master has been pushed to the review server.
It is available at https://review.typo3.org/24218

#12 Updated by Helmut Hummel almost 2 years ago

Martin Lipp wrote:

Helmut Hummel wrote:

Can you check if the patch in Gerrit works for you? Thanks.

It works (at least patch 2 does).

Thanks.

Patch 3 basically does the same (keeping the validation results of the top level object, when recursing through its properties).

#13 Updated by Helmut Hummel almost 2 years ago

  • Status changed from Under Review to Resolved
  • % Done changed from 0 to 100

#14 Updated by Gerrit Code Review over 1 year ago

  • Status changed from Resolved to Under Review

Patch set 1 for branch 2.0 of project Packages/TYPO3.Flow has been pushed to the review server.
It is available at https://review.typo3.org/25875

#15 Updated by Gerrit Code Review over 1 year ago

Patch set 2 for branch 2.0 of project Packages/TYPO3.Flow has been pushed to the review server.
It is available at https://review.typo3.org/25875

#16 Updated by Helmut Hummel about 1 year ago

  • Status changed from Under Review to Resolved

Also available in: Atom PDF