The attribute reader should not trigger an exception for other attributes

[DaanBiesterbos]

Bug Report

Jetbrains offers several attributes to help adding metadata to the code. For example, #[Deprecated], #[ArrayShape] and others. After adding some of these to the code an exception was thrown by this bundle's attribute reader. I believe this to be a bug. Attribute readers from Doctrine or Symfony don't have this problem.

Error message: Attribute class "JetBrains\PhpStorm\Deprecated" not found Error FQCN: \Error

Q	A
BC Break	no
Bundle version	2.3.1
Symfony version	*
PHP version	>= 8

Summary

This bundle should not break when I use the attributes from jetbrains.

Current behavior

When I use the #[Deprecated] attribute for example, the attribute reader in this bundle will throw an exception. This happens on line 60 where the attribute class is instantiated.

How to reproduce

Just add one of the attributes to a property. Possibly, the class must be an entity that that is using this bundle.

    #[ORM\Column(type: "string")]
    #[Deprecated()]
    protected ?string $helloWorld;

Expected behavior

Exception is not thrown.

Suggested solution

I would suggest to add a try catch block. I already verified that this change would fix the issue.

        foreach ($attributes as $attribute) {
            $attributeName = $attribute->getName();
            try {
                $instance = $attribute->newInstance();
            } catch(\Error $e) {
                 continue;
            }

            if (!$instance instanceof AnnotationInterface) {
                continue;
            }

            $instances[$attributeName] = $instance;
        }

[garak]

Catching errors doesn't seem the best solution, it could hide some other kind of problems. I remember Doctrine has a way to exclude annotations while reading them, we should investigate if a similar mechanism exists for attributes.

[DaanBiesterbos]

It is true that catching the error would possibly hide problems related to instantiating attributes from within the bundle. I did not feel like hardcoding the (root) namespace was any better though. How would you feel about a "allowedNamespaces" constructor argument. We can initially just pass the Vich namespace. If needed we can add a setting to the bundle to add additional namespaces. I do like the approach of only instantiating attributes that are relevant as well. In applications with many attributes it seems a bit wasteful to instantiate all of them. Even though its problably milliseconds we are taling about.

If you agree on the solution I may create a PR for it.

[garak]

Sure, go ahead