Inappropriate error detail for UniqueEntity annotation error

[crayner]

Bug Report

Q	A
BC Break	No
Version	All

Summary

I found an error trying to validate the doctrine schema:

php bin/console doctrine:schema:validate --verbose

Mapping
-------

In Constraint.php line 163:

  [Symfony\Component\Validator\Exception\MissingOptionsException]
  The options "fields" must be set for constraint "Symfony\Bridge\Doctrine\Validator\Constraints\UniqueEntity".


Exception trace:
  at F:\websites\crayner\quoll\vendor\symfony\validator\Constraint.php:163
 Symfony\Component\Validator\Constraint->normalizeOptions() at F:\websites\crayner\quoll\vendor\symfony\validator\Constraint.php:108
 Symfony\Component\Validator\Constraint->__construct() at F:\websites\crayner\quoll\vendor\doctrine\annotations\lib\Doctrine\Common\Annotations\DocParser.php:827
 Doctrine\Common\Annotations\DocParser->Annotation() at F:\websites\crayner\quoll\vendor\doctrine\annotations\lib\Doctrine\Common\Annotations\DocParser.php:662
 Doctrine\Common\Annotations\DocParser->Annotations() at F:\websites\crayner\quoll\vendor\doctrine\annotations\lib\Doctrine\Common\Annotations\DocParser.php:354
 Doctrine\Common\Annotations\DocParser->parse() at F:\websites\crayner\quoll\vendor\doctrine\annotations\lib\Doctrine\Common\Annotations\AnnotationReader.php:221
 Doctrine\Common\Annotations\AnnotationReader->getClassAnnotations() at F:\websites\crayner\quoll\vendor\doctrine\annotations\lib\Doctrine\Common\Annotations\CachedReader.php:80
 Doctrine\Common\Annotations\CachedReader->getClassAnnotations() at F:\websites\crayner\quoll\vendor\doctrine\persistence\lib\Doctrine\Persistence\Mapping\Driver\AnnotationDriver.php:177
 Doctrine\Persistence\Mapping\Driver\AnnotationDriver->isTransient() at F:\websites\crayner\quoll\vendor\doctrine\persistence\lib\Doctrine\Persistence\Mapping\Driver\AnnotationDriver.php:245
 Doctrine\Persistence\Mapping\Driver\AnnotationDriver->getAllClassNames() at F:\websites\crayner\quoll\vendor\doctrine\persistence\lib\Doctrine\Persistence\Mapping\Driver\MappingDriverChain.php:107
 Doctrine\Persistence\Mapping\Driver\MappingDriverChain->getAllClassNames() at F:\websites\crayner\quoll\vendor\doctrine\persistence\lib\Doctrine\Persistence\Mapping\AbstractClassMetadataFactory.php:90
 Doctrine\Persistence\Mapping\AbstractClassMetadataFactory->getAllMetadata() at F:\websites\crayner\quoll\vendor\doctrine\orm\lib\Doctrine\ORM\Tools\SchemaValidator.php:68
 Doctrine\ORM\Tools\SchemaValidator->validateMapping() at F:\websites\crayner\quoll\vendor\doctrine\orm\lib\Doctrine\ORM\Tools\Console\Command\ValidateSchemaCommand.php:69
 Doctrine\ORM\Tools\Console\Command\ValidateSchemaCommand->execute() at F:\websites\crayner\quoll\vendor\doctrine\doctrine-bundle\Command\Proxy\ValidateSchemaCommand.php:34
 Doctrine\Bundle\DoctrineBundle\Command\Proxy\ValidateSchemaCommand->execute() at F:\websites\crayner\quoll\vendor\symfony\console\Command\Command.php:258
 Symfony\Component\Console\Command\Command->run() at F:\websites\crayner\quoll\vendor\symfony\console\Application.php:929
 Symfony\Component\Console\Application->doRunCommand() at F:\websites\crayner\quoll\vendor\symfony\framework-bundle\Console\Application.php:96
 Symfony\Bundle\FrameworkBundle\Console\Application->doRunCommand() at F:\websites\crayner\quoll\vendor\symfony\console\Application.php:264
 Symfony\Component\Console\Application->doRun() at F:\websites\crayner\quoll\vendor\symfony\framework-bundle\Console\Application.php:82
 Symfony\Bundle\FrameworkBundle\Console\Application->doRun() at F:\websites\crayner\quoll\vendor\symfony\console\Application.php:140
 Symfony\Component\Console\Application->run() at F:\websites\crayner\quoll\bin\console:42

The issue here is not with the error, but in the fact that the error is not correctly identifying the issue, which is a annotation parse error, causing the Contraint violation. Unfortunately, this error does not identify the entity at fault, and I had to remove ALL UniqueEntity references in 150+ entities and return them one at a time to eventually find the error. The error on the annotation looks like:

...
use Doctrine\ORM\Mapping as ORM;
use Symfony\Bridge\Doctrine\Validator\Constraints\UniqueEntity;
...

 * @ORM\Table(name="Scale",
 *     indexes={@ORM\Index(name="lowest_acceptable",columns={"lowest_acceptable"})},
 *     uniqueConstraints={@ORM\UniqueConstraint(name="name",columns={"name"}),
 *     @ORM\UniqueConstraint(name="abbreviation",columns={"abbreviation"}),
 * })
 * @UniqueEntity{"name"})
 * @UniqueEntity({"abbreviation"})

The error is on the "name" line, which is missing the opening bracket.

Current behavior

The current behaviour is as stated above.

How to reproduce

Remove the opening bracket on any UniqueEntity annotation will duplicate this issue.

Expected behavior

[Doctrine\Common\Annotations\AnnotationException]
  [Syntax Error] Failed to see bracket set at position xxx in class App\Entity\Name.

[greg0ire]

I'm transferring the issue, IMO it does not have to do with the ORM, you could have the same issue with any other kind of annotations.

[greg0ire]

I disagree a little with the expected behavior, I would expect to have the Symfony\Component\Validator\Exception\MissingOptionsException passed as $previous argument of an exception in this package, with a message that would look like this:

Failed getting annotations from App\EntityName, or maybe something more even more precise Failed getting annotations while parsing file X, on line Y.

Possible next step: determine what pieces of information (classname? filename? line number?) we have at each level of this stack trace:

 Symfony\Component\Validator\Constraint->normalizeOptions() at F:\websites\crayner\quoll\vendor\symfony\validator\Constraint.php:108
 Symfony\Component\Validator\Constraint->__construct() at F:\websites\crayner\quoll\vendor\doctrine\annotations\lib\Doctrine\Common\Annotations\DocParser.php:827
 Doctrine\Common\Annotations\DocParser->Annotation() at F:\websites\crayner\quoll\vendor\doctrine\annotations\lib\Doctrine\Common\Annotations\DocParser.php:662
 Doctrine\Common\Annotations\DocParser->Annotations() at F:\websites\crayner\quoll\vendor\doctrine\annotations\lib\Doctrine\Common\Annotations\DocParser.php:354
 Doctrine\Common\Annotations\DocParser->parse() at F:\websites\crayner\quoll\vendor\doctrine\annotations\lib\Doctrine\Common\Annotations\AnnotationReader.php:221
 Doctrine\Common\Annotations\AnnotationReader->getClassAnnotations() at F:\websites\crayner\quoll\vendor\doctrine\annotations\lib\Doctrine\Common\Annotations\CachedReader.php:80

[crayner]

Sound good to me. As long as the message identifies an annotation failure in a particular file at a location in that file, I will be happy. Thanks for directing as well.

[jkufner]

The annotation reader has a method/property reflection object available; therefore, it should be easy to add filename, class name and method/property name into any exception message. However, because showing a filename in production may be undesirable, I would add class and method name only (it should suffice and not reveal any sensitive information).

The line number may be complicated, because the reflection object provides start line of the method, not the docblock, so it would likely be wrong.

[greg0ire]

However, because showing a filename in production may be undesirable

Why would people display exception messages in production?

Anyway, thanks for your insight, I think this is ready to be developed. @crayner , do you want to give it a go? Here is the piece of code that should have a try… catch:

annotations/lib/Doctrine/Common/Annotations/AnnotationReader.php

Lines 140 to 148 in 8fc7b23

	public function getClassAnnotations(ReflectionClass $class)
	{
	$this->parser->setTarget(Target::TARGET_CLASS);
	$this->parser->setImports($this->getClassImports($class));
	$this->parser->setIgnoredAnnotationNames($this->getIgnoredAnnotationNames($class));
	$this->parser->setIgnoredAnnotationNamespaces(self::$globalIgnoredNamespaces);
	return $this->parser->parse($class->getDocComment(), 'class ' . $class->getName());
	}

[crayner]

Thanks @greg0ire , Sorry, I find contributing very difficult as I become upset about reviews that argue about stuff that I have already stated, but people do not read or understand. So, No, I will not move this forward. Thanks for the offer.

[crayner]

If this error ever gets to a production site, the the developer never tested the code @jkufner as this is a HARD error. (i.e. HARD = it will fail 100% of the time, and is caused by the developer IMHO)

[jkufner]

@crayner Yes, however, it may happen after an update to a slightly incompatible version, or after not upgrading the dependencies. We cannot predict any particular context or scenario, so it is better to be careful by default.

[greg0ire]

@crayner no problem, I understand. How about you @jkufner , would you want to give it a try?