Skip to content

[Validator] The PropertyInfoLoader should return early when disabled #32278

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.

Sign up for GitHub

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

Jump to bottom
Closed
wants to merge 1 commit into from
Closed

[Validator] The PropertyInfoLoader should return early when disabled #32278

wants to merge 1 commit into from

Conversation

alanpoulain
Copy link
Contributor

Q A
Branch? 4.3
Bug fix? yes
New feature? no
BC breaks? no
Deprecations? no
Tests pass? yes
Fixed tickets n/a
License MIT
Doc PR n/a

Related to: #31749 and doctrine/DoctrineBundle#988.

If the auto-mapping was partially enabled for one resource (the Doctrine one), then the property info loader was applied for all the resources.

For instance, considering this configuration:

    validation:
        auto_mapping:
            App\Entity\Foo: ['doctrine.orm.default_entity_manager.validator_loader']

If there is a Bar entity, then the loader is applied too.

@alanpoulain alanpoulain mentioned this pull request Jun 29, 2019
Closed
dunglas
dunglas reviewed Jun 29, 2019
View reviewed changes
src/Symfony/Component/Validator/Mapping/Loader/PropertyInfoLoader.php
@@ -47,7 +47,7 @@ public function __construct(PropertyListExtractorInterface $listExtractor, Prope
public function loadClassMetadata(ClassMetadata $metadata)
{
$className = $metadata->getClassName();
if (null !== $this->classValidatorRegexp && !preg_match($this->classValidatorRegexp, $className)) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If null is passed, it means that it should be always executed IIRC.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think it should be the case, especially with: https://github.com/symfony/symfony/pull/31749/files#diff-0bd78268e7d70f726415dc3175eaa595R76

@xabbuh
Copy link
Member

xabbuh commented Jun 30, 2019

I guess we need to investigate why the PropertyInfoLoader is registered at all instead of trying to fiddle with the regex here. If I do not misunderstand the issue, this part should never be called here.

@nicolas-grekas nicolas-grekas added this to the 4.3 milestone Jun 30, 2019
@alanpoulain
Copy link
Contributor Author

The code to remove the PropertyInfoLoader service is here:

symfony/src/Symfony/Bundle/FrameworkBundle/DependencyInjection/FrameworkExtension.php

Lines 1263 to 1265 in 835f6b0

if (!$propertyInfoEnabled || !$config['auto_mapping'] || !class_exists(PropertyInfoLoader::class)) {
$container->removeDefinition('validator.property_info_loader');
}

You can see it's the case only when there is no auto-mapping configuration at all.

@nicolas-grekas nicolas-grekas modified the milestones: 4.3, 4.4 Feb 1, 2020
@fabpot
Copy link
Member

fabpot commented Aug 20, 2020

@xabbuh @dunglas As you are both skeptical about this change, can you confirm that this should be closed?

@fabpot
Copy link
Member

fabpot commented Sep 1, 2020

Closing for now as it's not clear if this is the right change.

@fabpot fabpot closed this Sep 1, 2020
@alanpoulain alanpoulain mentioned this pull request Dec 21, 2021
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dunglas dunglas dunglas left review comments

Assignees
No one assigned
Labels
Bug Status: Needs Review Validator
Projects
None yet
Milestone
4.4
Development

Successfully merging this pull request may close these issues.
6 participants
@alanpoulain @xabbuh @fabpot @dunglas @nicolas-grekas @carsonbot