The dependency on doctrine/annotations should become optional

[stof]

Feature Request

For projects that have migrated from annotations to attributes (as recommended by the Doctrine team), the doctrine/annotations package is unnecessary.
As the Doctrine team is planning to mark the doctrine/annotations package as abandoned (see doctrine/annotations#485), it would be great if this package could stop being a mandatory dependency of the extensions.

[mbabker]

See #2554 as it has a more complete rundown of where the issues lie.

[stof]

since this issue has been created, support for attributes has been implemented.
So for PHP 8+, attributes could be used to solve the issue:

• loggable and translatable provides mapping both using annotations and attributes now
• the helper methods could rely on those existing attributes to register an attribute driver instead of an annotations one
• ExtensionMetadataFactory could be creating a default attribute reader instead.

To me, the main blocker is actually the support for PHP 7.x. Maybe it is time to bump the min PHP version to 8.0 (or maybe 8.1 to be able to use enums in #2675 instead).

Note also that doctrine/orm 3.0 is dropping the annotation driver entirely, so you will have to do this migration to attributes before supporting ORM 3.0 (or making a conditional migration)

[mbabker]

#2559 deals with the helpers.

As you pointed out, attributes exist now, and leaving the annotations in place won't hurt anything for the time being, so that one's adequately covered for now.

I think realistically the remaining hard dependencies are the non-nullable $annotationReader argument of the ExtensionMetadataFactory constructor (including the logic in MappedEventSubscriber::getExtensionMetadataFactory() which tries to create a default annotation reader to provide the factory if one doesn't exist) and inherently changing its getDriver() method to throw if it tries to use an annotation or attribute based driver without that dependency being passed in (as the way the method reads to me right now, if you're using a XML or YAML mapping driver, you should never reach the code paths where the reader would be required, so that's already a bit of an over-reaching constructor requirement).

[tacman]

Any update on this? I don't think Symfony 7 will even support it.

[mbabker]

Symfony won't have a native annotations reader but it won't block your project from upgrading. If you're manually configuring services as your pre-edit comment suggests, you can use the Gedmo\Mapping\Driver\AttributeReader class in those setAnnotationReader method calls.

[tacman]

Is there an alias for that? I can't seem to find it. What should @annotation_reader be?

services:
  gedmo.listener.timestampable:
    class: Gedmo\Timestampable\TimestampableListener
    tags:
      - { name: doctrine.event_subscriber, connection: default }
    calls:
      - [ setAnnotationReader, [ '@annotation_reader' ] ]

[tacman]

I'm hacking in services.yaml, but this is a bit of a mystery to me:

    attr_reader: "@Gedmo\\Mapping\\Driver\\AttributeReader"
    Gedmo\Mapping\Driver\AttributeReader:
        alias: attribute_reader

Etc.

It seems like this would be better as the default, and someone could set the annotation reader if they were using them. There was a significant refactoring of this bundle a while ago to support attributes.

I'm mostly trying to silence deprecation warnings, which are practically blocking the real issues.

[tacman]

This issue is that I'd like to get rid of the warning in 6.4 to turn off annotations in framework.yaml:

framework:
    annotations: { enabled: false }

[mbabker]

This package on its own doesn't provide a Symfony bundle, so that attribute reader won't be automatically set up in a Symfony app (if you're using StofDoctrineExtensionsBundle you can check to see if that bundle's handling any of that already). So defining a new service in your app for that class and updating your setAnnotationReader calls to use that is a step in helping to resolve that deprecation.

As for the Symfony deprecation, on top of updating your service configs for the classes from this package, you'll also need to check your other app services and third-party integrations to see if they have a hard requirement on the doctrine/annotations package in any way before you can turn off the framework's annotations integration.

[stof]

My bundle does not yet handle that, because I just discovered the AttributeReader class in that discussion. But I will definitely handle that in the bundle directly ASAP.

[stof]

@mbabker there is still one place with a hard dependency: MappedEventSubscriber::getDefaultAnnotationReader always creates an annotation reader and not the new attribute one (meaning that the dependency cannot become optional)

[mbabker]

MappedEventSubscriber::getDefaultAnnotationReader() won't get called as long as you've called MappedEventSubscriber::setAnnotationReader() with an attribute reader first. So even that shouldn't be a hard blocker to running a Symfony app without the @annotation_reader service, it's just the one spot that still forces doctrine/annotations to be installed.

[stof]

@mbabker sure. but forcing the package to be installed is what will cause issues once the Doctrine team marks doctrine/annotations as abandoned as people will complain about getting a warning from Composer about the package being abandoned.
Supporting to run in a Symfony project that does not have the annotation_reader service defined is important to support Symfony 7. But this is not the original topic of this issue.

[mbabker]

I'll PR this later, but main...mbabker:DoctrineExtensions:default-reader might be enough to solve that one, too (builds on the same logic as #2559 for deciding defaults).

[stof]

@mbabker this is not the right approach IMO. This default reader will impact the kind of mapping that can be used in project-level entities for the Gedmo mapping. Passing an AttributeReader on PHP 8 would disable the support of annotations on PHP 8 by default, which is a BC break. When doctrine/annotations is installed, the default reader should support annotations. The ExtensionMetadataFactory will make sure to enable support for both annotations and attributes when the provided reader is an annotation reader.

This is different than the ORM mapping driver used for entities defined in the library. For those, we know that both attributes and annotations are provided and we can use either of them with the same result so preferring attributes on PHP 8 is fine.

[stof]

Alternatively, we could make the default annotation reader nullable in the factory, letting it create an AttributeDriver when needed.
Providing the default annotation driver externally was important because of the cached reader to avoid reading the annotations several times. But AttributeDriver does not have this need (the parsing of attributes is done by PHP itself so there is no userland caching of it).
You would still have to default to an annotation reader in case the library exists, for BC reasons.

[mbabker]

I can flip the order on the checks if that'll fix things. Otherwise it turns into a much bigger review trying to make the reader nullable where it's currently not. That was just a very quick idea to push up to see if it's even viable.

[stof]

flipping the conditions should indeed be enough

[mbabker]

#2713 to iterate on the fallback default behavior for the base subscriber class.

[mbabker]

Unless I'm missing something, I think #2728 is the last PR needed before doctrine/annotations can be an optional production dependency.

[dmitryuk]

May be, but composer still requires doctrine/annotations
https://github.com/doctrine-extensions/DoctrineExtensions/blob/main/composer.json#L45

[mbabker]

May be, but composer still requires doctrine/annotations
https://github.com/doctrine-extensions/DoctrineExtensions/blob/main/composer.json#L45

Well aware. It'll be updated in due time.

[mbabker]

#2735 would be the last step here and probably needs the most testing out of every PR that's been involved in decoupling this dependency.

[tacman]

How about a new version that required attributes, and didn't support annotations at all? That is, if someone needed to use annotations, they'd use the current version of the bundle, and the next one could only support Symfony 6.3+?

We could deprecate the use of annotations in this version for developers to prepare.

[mbabker]

It'll happen eventually (especially as Doctrine is deprecating annotations support entirely, including the package used for reading them), it just doesn't need to happen right now (or even anytime soon-ish; both the ORM and MongoDB ODM libraries support annotations on their current majors, and ORM 3 support is still a ways off here, so to me there isn't a need to rush to drop annotation support entirely just yet). Not to mention that there is a LOT of work in the tests that needs to happen so those can run without requiring annotation support.

[tacman]

okay. I thought it might be easier to drop annotations altogether than to support it as an option.

[ttk]

I'm on Symfony 7 and the framework.annotations.enable: false by default. I'm not getting any deprecations warnings, but I noticed that the AnnotationReader class is still being instantiated in the MappedEventSubscriber::getDefaultAnnotationReader() function. As I am using Attributes exclusively, the AnnotationsReader is being used to find annotations that don't exist, using up precious cpu cycles.

My workaround was to modify my kernel as follows:

class Kernel extends BaseKernel
{
  use MicroKernelTrait;

  protected function build(ContainerBuilder $container): void
  {
    $container->addCompilerPass(new class() implements CompilerPassInterface {
      public function process(ContainerBuilder $container): void
      {
         $container->register('gedmo_attribute_reader',\Gedmo\Mapping\Driver\AttributeReader::class);
         $container->getDefinition('stof_doctrine_extensions.listener.timestampable')
          ->addMethodCall('setAnnotationReader',[
            new Reference('gedmo_attribute_reader')
          ]);
      }
    });
  }
}

Ideally it shouldn't require this workaround. Hopefully this will help anyone else who runs into this issue.

[mbabker]

The patches making the annotation reader 100% optional haven't released yet.

Plus, for B/C, when doctrine/annotations is detected, using an annotation reader is the first choice when auto-detecting a reader. So your compiler pass injecting the attribute reader instead of relying on feature detection is actually the best way to address things until a combination of this package's next release (presumably 3.15) and doctrine/annotations is no longer installed in your application at all (be it through any other direct or transient dependency).