Decouple `AttributeReader` from `doctrine/annotations`

[mbabker]

Q	A
Bug report?	no
Feature request?	yes
BC Break report?	no
RFC?	no

The JMS\Serializer\Metadata\Driver\AttributeDriver\AttributeReader class has a hard dependency to the doctrine/annotations package in that it implements the annotation reader interface and requires a reader to be provided. It looks like this is because attributes support was added by decorating a Doctrine\Common\Annotations\Reader implementation to inject it into the JMS\Serializer\Metadata\Driver\AnnotationDriver class, which rightfully is dependent on an annotation reader. It would be great if this class could function without the annotations package.

As far as downstream impact goes, in one of my more complex Symfony apps, this implementation blocks me from disabling the annotations integration at the framework level (this class requires an annotation reader, the serializer bundle registers this class as a service requiring the annotation_reader service, and the annotation_reader service is not enabled if the framework.annotations.enabled config is set to false).

[goetas]

Hi, I think that it would be an improvement. do you think you can provide a PR?

[mbabker]

I'd have to think of something that doesn't end up being a pain in the neck to maintain.

I ended up cheating in that app and copied the JMS\Serializer\Metadata\Driver\AnnotationDriver class into my app as App\Serializer\Metadata\Driver\AttributeDriver, removed the annotation reader argument, and just inlined all the attribute handling stuff instead of using a separate reader (and then overloaded the jms_serializer.metadata_driver service definition since that hardcodes the list of drivers for the chain instead of using service tags).

With B/C considerations, you can't drop implementing the interface from JMS\Serializer\Metadata\Driver\AttributeDriver\AttributeReader, so that change is out of the question.

Maybe something like this can work to create separate drivers for annotations and attributes without duplicating a lot of logic?

namespace JMS\Serializer\Metadata\Driver;

use JMS\Serializer\Expression\CompilableExpressionEvaluatorInterface;
use JMS\Serializer\Naming\PropertyNamingStrategyInterface;
use JMS\Serializer\Type\Parser;
use JMS\Serializer\Type\ParserInterface;
use Metadata\ClassMetadata as BaseClassMetadata;
use Metadata\Driver\DriverInterface;

abstract class AnnotationOrAttributeDriver implements DriverInterface
{
    use ExpressionMetadataTrait;

    /**
     * @var ParserInterface
     */
    private $typeParser;

    /**
     * @var PropertyNamingStrategyInterface
     */
    private $namingStrategy;

    public function __construct(
        PropertyNamingStrategyInterface $namingStrategy,
        ?ParserInterface $typeParser = null,
        ?CompilableExpressionEvaluatorInterface $expressionEvaluator = null,
    ) {
        $this->typeParser = $typeParser ?: new Parser();
        $this->namingStrategy = $namingStrategy;
        $this->expressionEvaluator = $expressionEvaluator;
    }

    public function loadMetadataForClass(\ReflectionClass $class): ?BaseClassMetadata
    {
        // All the setup stuff before processing class annotations

        foreach ($this->getClassAnnotations($class) as $annot) {
            // The existing class annotations foreach loop
        }

        foreach ($class->getMethods() as $method) {
            if ($method->class !== $name) {
                continue;
            }

            $methodAnnotations = $this->getMethodAnnotations($method);

            foreach ($methodAnnotations as $annot) {
                // The existing method annotations foreach loop
            }
        }

        if (!$excludeAll) {
            foreach ($class->getProperties() as $property) {
                if ($property->class !== $name || (isset($property->info) && $property->info['class'] !== $name)) {
                    continue;
                }

                $propertiesMetadata[] = new PropertyMetadata($name, $property->getName());
                $propertiesAnnotations[] = $this->getPropertyAnnotations($property);
            }

            foreach ($propertiesMetadata as $propertyKey => $propertyMetadata) {
                // The existing property metadata foreach loop
            }
        }

        return $classMetadata;
    }

    /**
     * @return list<object>
     */
    abstract protected function getClassAnnotations(\ReflectionClass $class): array;

    /**
     * @return list<object>
     */
    abstract protected function getMethodAnnotations(\ReflectionMethod $method): array;

    /**
     * @return list<object>
     */
    abstract protected function getPropertyAnnotations(\ReflectionProperty $property): array;
}

namespace JMS\Serializer\Metadata\Driver;

use Doctrine\Common\Annotations\Reader;
use JMS\Serializer\Expression\CompilableExpressionEvaluatorInterface;
use JMS\Serializer\Naming\PropertyNamingStrategyInterface;
use JMS\Serializer\Type\Parser;
use JMS\Serializer\Type\ParserInterface;

class AnnotationDriver extends AnnotationOrAttributeDriver
{
    /**
     * @var Reader
     */
    private $reader;

    public function __construct(
        Reader $reader,
        PropertyNamingStrategyInterface $namingStrategy,
        ?ParserInterface $typeParser = null,
        ?CompilableExpressionEvaluatorInterface $expressionEvaluator = null,
    ) {
        parent::__construct($namingStrategy, $typeParser, $expressionEvaluator);

        $this->reader = $reader;
    }

    /**
     * @return list<object>
     */
   protected function getClassAnnotations(\ReflectionClass $class): array
    {
        return $this->reader->getClassAnnotations($class);
    }

    /**
     * @return list<object>
     */
   protected function getMethodAnnotations(\ReflectionMethod $method): array
    {
        return $this->reader->getMethodAnnotations($method);
    }

    /**
     * @return list<object>
     */
   protected function getPropertyAnnotations(\ReflectionProperty $property): array
    {
        return $this->reader->getPropertyAnnotations($property);
    }
}

namespace JMS\Serializer\Metadata\Driver;

class AttributeDriver extends AnnotationOrAttributeDriver
{
    /**
     * @return list<object>
     */
   protected function getClassAnnotations(\ReflectionClass $class): array
    {
        return array_map(
            static fn (\ReflectionAttribute $attribute) => $attribute->newInstance(),
            $class->getAttributes(),
        );
    }

    /**
     * @return list<object>
     */
   protected function getMethodAnnotations(\ReflectionMethod $method): array
    {
        return array_map(
            static fn (\ReflectionAttribute $attribute) => $attribute->newInstance(),
            $method->getAttributes(),
        );
    }

    /**
     * @return list<object>
     */
   protected function getPropertyAnnotations(\ReflectionProperty $property): array
    {
        return array_map(
            static fn (\ReflectionAttribute $attribute) => $attribute->newInstance(),
            $property->getAttributes(),
        );
    }
}

With this, JMS\Serializer\Metadata\Driver\AttributeDriver\AttributeReader can be deprecated in favor of the JMS\Serializer\Metadata\Driver\AttributeDriver. There'd still be some work to do elsewhere in the package to make the Annotations package an optional integration, (i.e. the JMS\Serializer\Builder\DriverFactoryInterface having a mandatory annotation reader argument), but at least this change paves a way forward for wiring up the bundle without the annotation_reader service in Symfony apps (as that's not using the builder API to create a serializer).

[goetas]

done in https://github.com/schmittjoh/serializer/pull/1469

[bcremer]

Can we get a step further and also remove doctrine/annotations from the require section and have it as an optional dependency.

That would potentially introduce some BC issues for users relying on annotations as they'd need to require doctrine/annotations in their projects explicitly.

A good exception message could push them in the right direction.

WDYT?

[goetas]

im good with it, but how to make sure that jms/serializer does not need doctrine annotations?

[mbabker]

There's some stuff with the builder classes that needs to be made to work without Annotations being installed (including a potentially B/C breaking type widening change on a param in the DriverFactoryInterface). But, yeah, eventually the goal is to have the Annotations package be optional.