Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Symfony 4.3.0 Validator validates interface instead of class with resolve_target_entities #31752

Closed
Selion05 opened this issue May 30, 2019 · 23 comments

Comments

Projects
None yet
6 participants
@Selion05
Copy link

commented May 30, 2019

Symfony version(s) affected: 4.3.0

Description

Upgrading from 4.2.9 to 4.3.0 introduced a bug in one of my projects.

Trying the sym43 branch below results in

In PropertyMetadata.php line 40:
Property "ticketNumber" does not exist in class "App\Entity\CommentInterface"

It seems that because i use resolve_target_entities for my comments the validator is trying to validate the interface and not the actual class.

without resolve_target_entities it's back to normal (see branch sym43-without-interface)

How to reproduce
clone repo

git clone git@github.com:Selion05/symfony43-validator-bug.git && cd symfony43-validator-bug

# works
git checkout sym42 && composer install
php bin/console test

# throws validator exception
git checkout sym43 && composer install
php bin/console test

# works
git checkout sym43-without-interface
php bin/console test

Additional context
debug output

$ php bin/console test -vvv
2019-05-30T19:34:29+00:00 [debug] Notified event "console.command" to listener "Symfony\Component\HttpKernel\EventListener\DebugHandlersListener::configure".
2019-05-30T19:34:29+00:00 [error] Error thrown while running command "test -vvv". Message: "Property "ticketNumber" does not exist in class "App\Entity\CommentInterface""
2019-05-30T19:34:29+00:00 [debug] Notified event "console.error" to listener "Symfony\Bundle\FrameworkBundle\EventListener\SuggestMissingPackageSubscriber::onConsoleError".
2019-05-30T19:34:29+00:00 [debug] Notified event "console.error" to listener "Symfony\Component\Console\EventListener\ErrorListener::onConsoleError".
2019-05-30T19:34:29+00:00 [debug] Command "test -vvv" exited with code "1"
2019-05-30T19:34:29+00:00 [debug] Notified event "console.terminate" to listener "Symfony\Component\Console\EventListener\ErrorListener::onConsoleTerminate".

In PropertyMetadata.php line 40:
                                                                                 
  [Symfony\Component\Validator\Exception\ValidatorException]                     
  Property "ticketNumber" does not exist in class "App\Entity\CommentInterface"  
                                                                                 

Exception trace:
 () at /home/leo/projects/sym43bug/vendor/symfony/validator/Mapping/PropertyMetadata.php:40
 Symfony\Component\Validator\Mapping\PropertyMetadata->__construct() at /home/leo/projects/sym43bug/vendor/symfony/validator/Mapping/ClassMetadata.php:216
 Symfony\Component\Validator\Mapping\ClassMetadata->addPropertyConstraint() at /home/leo/projects/sym43bug/vendor/symfony/doctrine-bridge/Validator/DoctrineLoader.php:81
 Symfony\Bridge\Doctrine\Validator\DoctrineLoader->loadClassMetadata() at /home/leo/projects/sym43bug/vendor/symfony/validator/Mapping/Loader/LoaderChain.php:54
 Symfony\Component\Validator\Mapping\Loader\LoaderChain->loadClassMetadata() at /home/leo/projects/sym43bug/vendor/symfony/validator/Mapping/Factory/LazyLoadingMetadataFactory.php:105
 Symfony\Component\Validator\Mapping\Factory\LazyLoadingMetadataFactory->getMetadataFor() at /home/leo/projects/sym43bug/vendor/symfony/validator/Mapping/Factory/LazyLoadingMetadataFactory.php:148
 Symfony\Component\Validator\Mapping\Factory\LazyLoadingMetadataFactory->mergeConstraints() at /home/leo/projects/sym43bug/vendor/symfony/validator/Mapping/Factory/LazyLoadingMetadataFactory.php:113
 Symfony\Component\Validator\Mapping\Factory\LazyLoadingMetadataFactory->getMetadataFor() at /home/leo/projects/sym43bug/vendor/symfony/validator/Validator/RecursiveContextualValidator.php:316
 Symfony\Component\Validator\Validator\RecursiveContextualValidator->validateObject() at /home/leo/projects/sym43bug/vendor/symfony/validator/Validator/RecursiveContextualValidator.php:141
 Symfony\Component\Validator\Validator\RecursiveContextualValidator->validate() at /home/leo/projects/sym43bug/vendor/symfony/validator/Validator/RecursiveValidator.php:100
 Symfony\Component\Validator\Validator\RecursiveValidator->validate() at /home/leo/projects/sym43bug/src/Command/TestCommand.php:56
 App\Command\TestCommand->execute() at /home/leo/projects/sym43bug/vendor/symfony/console/Command/Command.php:255
 Symfony\Component\Console\Command\Command->run() at /home/leo/projects/sym43bug/vendor/symfony/console/Application.php:939
 Symfony\Component\Console\Application->doRunCommand() at /home/leo/projects/sym43bug/vendor/symfony/framework-bundle/Console/Application.php:87
 Symfony\Bundle\FrameworkBundle\Console\Application->doRunCommand() at /home/leo/projects/sym43bug/vendor/symfony/console/Application.php:273
 Symfony\Component\Console\Application->doRun() at /home/leo/projects/sym43bug/vendor/symfony/framework-bundle/Console/Application.php:73
 Symfony\Bundle\FrameworkBundle\Console\Application->doRun() at /home/leo/projects/sym43bug/vendor/symfony/console/Application.php:149
 Symfony\Component\Console\Application->run() at /home/leo/projects/sym43bug/bin/console:42


@xabbuh

This comment has been minimized.

Copy link
Member

commented May 30, 2019

Does the error disappear with the chances from #31749?

@Selion05

This comment has been minimized.

Copy link
Author

commented May 30, 2019

i removed

framework:
    validation:
        auto_mapping:
           App\Entity\: []

allready before beause it wasn't there in 4.2. I assume this disables it?

@Selion05

This comment has been minimized.

Copy link
Author

commented May 30, 2019

ok just changed DoctrineLoader.php and AddAutoMappingConfigurationPass.php and the error disappears :)

nicolas-grekas added a commit that referenced this issue May 31, 2019

bug #31749 [DoctrineBridge][Validator] do not enable validator auto m…
…apping by default (xabbuh)

This PR was merged into the 4.3 branch.

Discussion
----------

[DoctrineBridge][Validator] do not enable validator auto mapping by default

| Q             | A
| ------------- | ---
| Branch?       | 4.3
| Bug fix?      | yes
| New feature?  | no
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | part of #31715, #31752
| License       | MIT
| Doc PR        |

Commits
-------

a3555fc do not enable validator auto mapping by default
@xabbuh

This comment has been minimized.

Copy link
Member

commented Jun 4, 2019

Can you please check if #31836 helps here?

@dotdevru

This comment has been minimized.

Copy link

commented Jun 4, 2019

Can you please check if #31836 helps here?

I can confirm that issue is gone.

@xabbuh

This comment has been minimized.

Copy link
Member

commented Jun 4, 2019

@dotdevru thank you for the feedback

fabpot added a commit that referenced this issue Jun 4, 2019

bug #31836 [DoctrineBridge] do not process private properties from pa…
…rent class (xabbuh)

This PR was merged into the 4.3 branch.

Discussion
----------

[DoctrineBridge] do not process private properties from parent class

| Q             | A
| ------------- | ---
| Branch?       | 4.3
| Bug fix?      | yes
| New feature?  | no
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | #31715, #31752
| License       | MIT
| Doc PR        |

Commits
-------

adfa1ef do not process private properties from parent class
@spackmat

This comment has been minimized.

Copy link

commented Jun 5, 2019

For others: The provided fix solves the problem for me. But my code triggered this bug only, because I had an Entity with single-table-inheritance and some properties of the base class were marked as private instead of protected. So from the derived subclasses they were not accessible. This was an error in my code triggering this bug here and removing this error solved the problem. So if you experience this, have a closer look at your class inheritance and fix your bugs first. ;)

@Selion05

This comment has been minimized.

Copy link
Author

commented Jun 5, 2019

I'm wondering if the bug is actually solved? As far as I understand it #31749 doesn't enable the auto validation feature by default. But what if somebody wants to use auto validating in combination with doctrine's resolve_target_entities?

@xabbuh

This comment has been minimized.

Copy link
Member

commented Jun 5, 2019

@spackmat You shouldn't have to adapt your code. Can you explain why you think that's necessary?

@Selion05 Do you actually run into any issues using the latest 4.3 branch when the feature is enabled?

@Selion05

This comment has been minimized.

Copy link
Author

commented Jun 5, 2019

@xabbuh Not really sure how to do that with composer so I manually replaced validator and doctrine-bridge from github and enabled auto_mapping like

framework:
    validation:
        email_validation_mode: html5

        # Enables validator auto-mapping support.
        # For instance, basic validation constraints will be inferred from Doctrine's metadata.
        auto_mapping:
           App\Entity\: []
           App\Model\: []

and it works :)

@xabbuh

This comment has been minimized.

Copy link
Member

commented Jun 5, 2019

@Selion05

Something like this should work:

{
    "require": {
        "symfony/doctrine-bridge": "4.3.*@dev"
    }
}
@Selion05

This comment has been minimized.

Copy link
Author

commented Jun 5, 2019

seems solved

@spackmat

This comment has been minimized.

Copy link

commented Jun 5, 2019

@xabbuh my own code was not correct at all and so, from the derived subclasses, the private properties didn't exist and trying to add constraints to them triggered this bug. The provided fix solves this, too by checking if properties exist, but they should have existed all the time within the subclasses. So thanks for that bug, which directed me to this bug in my own code. I don't even know, why I marked the properties as private in the first place. Maybe there are others who made this mistake and now get the error and can solve this now.

@xabbuh

This comment has been minimized.

Copy link
Member

commented Jun 5, 2019

@spackmat I don't understand why you consider marking the properties private as bug. That's why I am asking. Maybe we are still missing something that needs to be fixed.

@spackmat

This comment has been minimized.

Copy link

commented Jun 5, 2019

I have an Entity setting like this:

/**
 * @ORM\Entity
 * @ORM\InheritanceType("SINGLE_TABLE")
 * @ORM\DiscriminatorColumn(name="type", type="string")
 * @ORM\DiscriminatorMap({
 *     "one" = "VariantOne",
 * })
 */
class MySingleTableInheritanceEntity
{
    /**
     * @ORM\Column(name="aProperty", type="string", length=255, nullable=true)
     */
    private $aProperty;
    
    public function setAProperty(string $aproperty): self
    {
        $this->aProperty = $aProperty;
        return $this;
    }
    
    public function getAProperty(): ?string
    {
        return $this->aProperty;
    }
}

and

class VariantOne extends MySingleTableInheritanceEntity
{
    // some other explicit properties
}

This worked fine until now. Before: The Form/PropertyAccessor used the pubilic getters/setters inherited by VariantOne, marking as private or protected made no difference. With this bug: Doctrine bridge tries to add a Constraint to VariantOne on the aProperty property, which is not accessible, because it is set to private in the parent class. So your fix fixes this, because the property_exists()-check fails on aProperty of VariantOne.

I consider setting properties to private in those cases a bug (at least this is not what I want here). Setting to protected also solves the problem, because the Doctrine bridge now has access to it through VariantOne.

@xabbuh

This comment has been minimized.

Copy link
Member

commented Jun 5, 2019

This behaviour of the Doctrine bridge was a bug and was fixed in #31836.

@spackmat

This comment has been minimized.

Copy link

commented Jun 6, 2019

@xabbuh yeah, I know. Just wanted others to also see this bug as an opportunity to discover own bugs like my one with the private properties.

(Thanks for the quick fix btw.)

@xabbuh

This comment has been minimized.

Copy link
Member

commented Jun 6, 2019

I still don't get why setting properties to private should be a bug.

@spackmat

This comment has been minimized.

Copy link

commented Jun 6, 2019

@xabbuh in my case because the parent class is kind of an abstract (= never directly used) class and only the concrete derived classes are the ones used as Entities.

Call it a bug or not, this was not the thing I wanted to implement. For example: The automatic length constraint for properties won't be added by the doctrine bridge, when the properties are declared as private in the parent class. I want my properties to benefit from this, so I consider my old code a bug.

All I wanted is to leave my insight for others in the same situation and stumbling over this issue because their setting also triggered this message.

I didn't want to bind your resources investigating what I consider a bug. So sorry for that.

@xabbuh

This comment has been minimized.

Copy link
Member

commented Jun 6, 2019

Call it a bug or not, this was not the thing I wanted to implement. For example: The automatic length constraint for properties won't be added by the doctrine bridge, when the properties are declared as private in the parent class. I want my properties to benefit from this, so I consider my old code a bug.

Well, but that's the thing I was talking about earlier. The fact this was not working was a bug in 4.3.0 that has been fixed an will be part of 4.3.1.

What should be clear is that Symfony doesn't require you to make your properties protected in parent classes and that we need to fix the framework code instead if there are still issues because of that.

@spackmat

This comment has been minimized.

Copy link

commented Jun 6, 2019

Then it should be implemented, that the length constraint is added to those private properties. I got this Exception on that properties, because the line

$metadata->addPropertyConstraint($mapping['fieldName'], new Length(['max' => $mapping['length']]));

failed on those (in the parent class) private properties. Your fix fixes this, because

elseif (property_exists($className, $mapping['fieldName'])) {

checks for the property and property_exists() is false for the private property of the parent class. As seen in this example:

class A {
    private $aProperty;
}
class B extends A {    
}
property_exists(A::class, 'aProperty'); // true
property_exists(B::class, 'aProperty'); // false

So with the current state, the Length-Constraint is in fact not added to the private properties (that would even fail like before, because your fix only skips adding the Constraint).

@xabbuh

This comment has been minimized.

Copy link
Member

commented Jun 6, 2019

It is added when the parent class is processed. If that doesn't work for you, please create a small example application that allows to reproduce it.

@spackmat

This comment has been minimized.

Copy link

commented Jun 6, 2019

Then everything is okay. I didn't test that, just had a look at your changed code (and had changed the properties to protected before). So thanks again for making that point clear.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.