Skip to content

[DependencyInjection] ContainerBuilder::getReflectionClass() does not support interfaces, if symfony/config package not present #58821

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Closed
donquixote opened this issue Nov 9, 2024 · 1 comment · Fixed by #58822
Closed

[DependencyInjection] ContainerBuilder::getReflectionClass() does not support interfaces, if symfony/config package not present #58821

donquixote opened this issue Nov 9, 2024 · 1 comment · Fixed by #58822
Labels
Bug DependencyInjection Status: Needs Review

Comments

@donquixote
Copy link
Contributor

Symfony version(s) affected

7.2.0

Description

The ContainerBuilder::getReflectionClass() method does not support interfaces when 'symfony/config' package not present.

Given an interface MyInterface..
With symfony/config package present, $builder->getReflectionClass(MyInterface::class) will return a \ReflectionClass object for that interface.
With symfony/config package missing, $builder->getReflectionClass(MyInterface::class) will return false.

This has an impact on AutowirePass, if the class on a service definition is an interface.

How to reproduce

This problem manifests in two levels.

Low-level manifestation

<?php

use Symfony\Component\DependencyInjection\ContainerBuilder;

require_once __DIR__ . '/vendor/autoload.php';

interface I {}

$rc = (new ContainerBuilder())->getReflectionClass(I::class);

// With symfony/config: ReflectionClass.
// Without symfony/config: NULL
print get_debug_type($rc) . "\n";

Higher level manifestation in autowire

<?php

use Symfony\Component\DependencyInjection\ContainerBuilder;
use Symfony\Component\DependencyInjection\Definition;

require_once __DIR__ . '/vendor/autoload.php';

class Dependency {}
interface ServiceInterface {}
class Service implements ServiceInterface {}
class Factories {
  public static function createService(
    Dependency $dependency,
  ): ServiceInterface {
    return new Service();
  }
}

$builder = new ContainerBuilder();

$definition = new Definition(Dependency::class);
$builder->setDefinition(Dependency::class, $definition);

$definition = new Definition(ServiceInterface::class);
$definition->setAutowired(true);
$definition->setFactory([Factories::class, 'createService']);
$definition->setPublic(true);
$builder->setDefinition('service', $definition);

$builder->compile();

$service = $builder->get('service');

print get_debug_type($service) . "\n";

Possible Solution

In ContainerBuilder::getReflectionClass(), add a || interface_exists($class)`.

        try {
            if (isset($this->classReflectors[$class])) {
                $classReflector = $this->classReflectors[$class];
            } elseif (false && class_exists(ClassExistenceResource::class)) {
                $resource = new ClassExistenceResource($class, false);
                $classReflector = $resource->isFresh(0) ? false : new \ReflectionClass($class);
            } else {
                $classReflector = (class_exists($class) || interface_exists($class)) ? new \ReflectionClass($class) : false;
            }
        } catch (\ReflectionException $e) {
            if ($throw) {
                throw $e;
            }
        }

Additional Context

No response
@donquixote donquixote added the Bug label Nov 9, 2024
donquixote added a commit to donquixote/symfony that referenced this issue Nov 9, 2024
@donquixote
Issue symfony#58821: [DependencyInjection] Attempt to break tests.
e8727eb
@donquixote
Copy link
Contributor Author

Currently, the code in question is not even covered by tests. At least not any test in the github actions.

See
donquixote@e8727eb
https://github.com/donquixote/symfony/actions/runs/11759803112
https://github.com/donquixote/symfony/actions/runs/11759803115
I suppose one of these would fail if the code was covered.

I think the problem here is that all the tests run in the full monorepo, which means that class_exists(ClassExistenceResource::class) always returns TRUE.

If we want to add tests for this, we need to rethink overall how we run tests in symfony.

@donquixote donquixote mentioned this issue Nov 9, 2024
Merged
donquixote added a commit to donquixote/symfony that referenced this issue Nov 10, 2024
@donquixote
Issue symfony#58821: [DependencyInjection] Support interfaces in Cont…
1c3690b 
…ainerBuilder::getReflectionClass().
donquixote added a commit to donquixote/symfony that referenced this issue Nov 10, 2024
@donquixote
Issue symfony#58821: [DependencyInjection] Attempt to break tests.
4465f72
nicolas-grekas pushed a commit to donquixote/symfony that referenced this issue Nov 20, 2024
@donquixote @nicolas-grekas
Issue symfony#58821: [DependencyInjection] Support interfaces in Cont…
6166e8f 
…ainerBuilder::getReflectionClass().
nicolas-grekas added a commit that referenced this issue Nov 20, 2024
@nicolas-grekas
bug #58822 [DependencyInjection] Fix checking for interfaces in Conta…
cf20e09 
…inerBuilder::getReflectionClass() (donquixote)

This PR was merged into the 5.4 branch.

Discussion
----------

[DependencyInjection] Fix checking for interfaces in ContainerBuilder::getReflectionClass()

| Q             | A
| ------------- | ---
| Branch?       | 5.4
| Bug fix?      | yes
| New feature?  | no
| Deprecations? | no
| Issues        | Fix #58821.
| License       | MIT

Currently, `ContainerBuilder::getReflectionClass()` supports interfaces only if `symfony/config` package is present.
With this fix, it will do so without `symfony/config`.

> Always add tests and ensure they pass.

Not sure how to do this.
All tests run in the monorepo, with all packages present, so `class_exists(ClassExistenceResource::class)` will always be TRUE.
To make this testable, we should make the condition dependent on resource tracking.

> Never break backward compatibility (see https://symfony.com/bc).

This is TBD.
This change would enable autowire for some services where this was previously not the case.

Commits
-------

6166e8f Issue #58821: [DependencyInjection] Support interfaces in ContainerBuilder::getReflectionClass().
nicolas-grekas added a commit that referenced this issue Nov 20, 2024
@nicolas-grekas
Merge branch '5.4' into 6.4
70f7d05 
* 5.4:
  Issue #58821: [DependencyInjection] Support interfaces in ContainerBuilder::getReflectionClass().
  Dynamically fix compatibility with doctrine/data-fixtures v2
  [HttpKernel] Ensure HttpCache::getTraceKey() does not throw exception
  don't call EntityManager::initializeObject() with scalar values
  [Validator] review italian translations
  Update PR template
nicolas-grekas added a commit that referenced this issue Nov 20, 2024
@nicolas-grekas
Merge branch '6.4' into 7.1
597e48c 
* 6.4:
  fix: ignore missing directory in isVendor()
  [OptionsResolver] Allow Union/Intersection Types in Resolved Closures
  Issue #58821: [DependencyInjection] Support interfaces in ContainerBuilder::getReflectionClass().
  Dynamically fix compatibility with doctrine/data-fixtures v2
  [HttpKernel] Ensure HttpCache::getTraceKey() does not throw exception
  don't call EntityManager::initializeObject() with scalar values
  [Validator] review italian translations
  Update PR template
nicolas-grekas added a commit that referenced this issue Nov 20, 2024
@nicolas-grekas
Merge branch '7.1' into 7.2
e2f2a96 
* 7.1:
  fix: ignore missing directory in isVendor()
  [OptionsResolver] Allow Union/Intersection Types in Resolved Closures
  Issue #58821: [DependencyInjection] Support interfaces in ContainerBuilder::getReflectionClass().
  Dynamically fix compatibility with doctrine/data-fixtures v2
  [HttpKernel] Ensure HttpCache::getTraceKey() does not throw exception
  don't call EntityManager::initializeObject() with scalar values
  make RelayProxyTrait compatible with relay extension 0.9.0
  [Validator] review italian translations
  Update PR template
nicolas-grekas added a commit that referenced this issue Nov 20, 2024
@nicolas-grekas
Merge branch '7.2' into 7.3
f194b1b 
* 7.2:
  Revert "bug #58937 [FrameworkBundle] Don't auto-register form/csrf when the corresponding components are not installed (nicolas-grekas)"
  Fix merge
  fix: ignore missing directory in isVendor()
  [OptionsResolver] Allow Union/Intersection Types in Resolved Closures
  Issue #58821: [DependencyInjection] Support interfaces in ContainerBuilder::getReflectionClass().
  Dynamically fix compatibility with doctrine/data-fixtures v2
  [HttpKernel] Ensure HttpCache::getTraceKey() does not throw exception
  don't call EntityManager::initializeObject() with scalar values
  [FrameworkBundle] Don't auto-register form/csrf when the corresponding components are not installed
  make RelayProxyTrait compatible with relay extension 0.9.0
  [Validator] review italian translations
  Update PR template
@donquixote donquixote mentioned this issue Nov 27, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
Bug DependencyInjection Status: Needs Review
Projects
None yet
Milestone
No milestone
3 participants
@donquixote @nicolas-grekas @carsonbot