symfony / symfony

The Symfony PHP framework
https://symfony.com
MIT License
29.64k stars 9.43k forks source link

[Validator] Cannot declare class ConstraintViolationAssertion #50149

Closed enumag closed 1 year ago

enumag commented 1 year ago

Symfony version(s) affected

6.2.9

Description

I encountered this error:

Error: Cannot declare class Symfony\Component\Validator\Test\ConstraintViolationAssertion, because the name is already in use

How to reproduce

First I need to explain what caused this. It's very nasty and took a while to figure out.

In our app we have some internal code generation tools that need to find all implementations for a given interface. So I wrote this:

<?php

declare(strict_types=1);

use Composer\Autoload\ClassLoader;
use ReflectionClass;

final class Autoloading
{
    public static function getClassLoader(): ClassLoader
    {
        $file = (new ReflectionClass(ClassLoader::class))->getFileName();
        \assert($file !== false);
        $loader = include \dirname($file, 2).'/autoload.php';
        \assert($loader instanceof ClassLoader);

        return $loader;
    }

    /**
     * @template T of object
     *
     * @param class-string<T> $interface
     *
     * @return iterable<int, class-string<T>>
     */
    public static function findImplementations(string $interface, string $namespacePrefix): iterable
    {
        $loader = self::getClassLoader();

        foreach ($loader->getClassMap() as $type => $path) {
            if (! str_starts_with($type, $namespacePrefix)) {
                continue;
            }

            if (is_subclass_of($type, $interface) && ! (new \ReflectionClass($type))->isAbstract()) {
                yield $type;
            }
        }
    }
}

This is of course used only at compile-time, never at runtime. And yes this indeed requires composer dump-autoloader --optimize, otherwise Composer's classmap may be missing some classes. The namespace prefix is there because I didn't want to search through Composer libraries.

This worked just fine until I realized that our production Dockerfile lacks --no-dev when running composer install. So I tried to add it which caused the is_subclass_of() above to fail occasionally when loading a class with non-existent parent - for instance our custom PHPStan rules were causing a problem because PHPStan\Rules\Rule is undefined due to --no-dev.

So I added something like this in the middle of the foreach. I simplified it here to make the code shorter but the idea should be clear:

            if (! class_exists($type, false) && ! interface_exists($type, false)) {
                try {
                    class_exists($type);
                } catch (\Error $e) {
                    continue;
                }
            }

Basically if the class wasn't already loaded I try to do so and if it fails due to undefined parent I catch the Error and skip the class.

And here is when it started to crash on the fatal error above - redeclaration of ConstraintViolationAssertion.

Why? Well you see we of course have a couple of custom validator constraints. And naturally we have tests for them. These tests extend Symfony\Component\Validator\Test\ConstraintValidatorTestCase. However ConstraintViolationAssertion is declared in the same file as ConstraintValidatorTestCase.

What happens here is that when I find such test in the classmap and try to load it Composer tries to load ConstraintValidatorTestCase and then tries to load it's parent PHPUnit\Framework\TestCase which doesn't exist. So autoloading fails, causes an Error which I catch and skip the class. So far so good.

However when I find a second such test in the classmap it causes the redeclaration of ConstraintViolationAssertion class. It's because even though ConstraintValidatorTestCase couldn't be correctly loaded due to missing parent when autoloading its file, ConstraintViolationAssertion was loaded anyway and can't be declared again.

Redeclaration however doesn't cause an \Error and isn't catchable so on the second test that implements ConstraintValidatorTestCase it will just crash on a fatal error.

Possible Solution

Sorry for long explanation, but this issue wasn't easy to explain. Anyway now it depends on you if you want to fix this problem in Symfony by splitting the classes to separate files or if you consider it too much of an edge-case to bother with it.

But either way there is a couple of workarounds I can do on my side:

Let me know if you want me to send a pull request to split the ConstraintValidatorTestCase.php into multiple files. Otherwise feel free to just close this issue. Maybe it will help someone in the future when facing the same problem.

Additional Context

No response

derrabus commented 1 year ago

In our app we have some internal code generation tools that need to find all implementations for a given interface.

I would strongly suggest that you run this kind of magic on your own code only. At any time we might add more internal classes like this one and break your tool again.

I think we should close this issue.

enumag commented 1 year ago

I would strongly suggest that you run this kind of magic on your own code only.

As I already wrote, I of course only run it on our own classes. However this restriction isn't sufficient because our classes include tests for our constraint validators and those need to extend Symfony's ConstraintValidatorTestCase.

stof commented 1 year ago

I would say that the solution would be that your code remembers the list of failed loading to avoid retrying them later for another test.

enumag commented 1 year ago

I would say that the solution would be that your code remembers the list of failed loading to avoid retrying them later for another test.

@stof I actually do that, that's one of the things I removed while simplifying the code. It doesn't help because we have multiple validator tests and there is no way to tell they are validator tests without loading them and loading them causes the issue. Remembering the first one has no effect on loading the second and the problem still appears.

derrabus commented 1 year ago

As I already wrote, I of course only run it on our own classes. However this restriction isn't sufficient because our classes include tests for our constraint validators and those need to extend Symfony's ConstraintValidatorTestCase.

So exclude your tests? Since all of them will directly or indirectly extend PHPUnit's test case class, you won't be able to process any of them anyway as long as PHPUnit is not installed.

enumag commented 1 year ago

@derrabus Yes, I ended up excluding my tests from the docker image by filtering them out in .dockerignore - which I already mentioned as a workaround in my first post.

As I said it's fine if you just want to close this without resolving it as I can work around it. I just posted it anyway so that you're aware of the problem and so that if anyone else runs into it they can read what's causing it and how to solve it.

derrabus commented 1 year ago

Okay, let's do that. Interesting find, but the way you use our classes is not really how they're meant to be used.

enumag commented 1 year ago

The only thing I'm doing with your ConstraintValidatorTestCase is extending it to test our own constraints which I believe is exactly what it's meant for as it isn't marked as internal.

The rest is a nasty side-effect of how PHP's autoloading works.