[Config] Handle Service/EventSubscriberInterface in ReflectionClassResource #25976
Conversation
|
Now tested, PR ready. |
877e2b1 to
628aab3
Compare
|
If this is something that should be documented for end users, please add some description about this feature, some simple code example, etc. Otherwise, please ignore my comment. Thanks! |
| throw new InvalidArgumentException(sprintf('Service "%s" must implement interface "%s".', $this->currentId, ServiceSubscriberInterface::class)); | ||
| } | ||
| $this->container->addObjectResource($class); | ||
| $class = $r->name; |
There was a problem hiding this comment.
because the class can be a parameter name that is resolved by getReflectionClass, so we need the resolved name
628aab3 to
c1e0963
Compare
| @@ -114,7 +117,9 @@ private function computeHash() | |||
|
|
|||
| private function generateSignature(\ReflectionClass $class) | |||
| { | |||
| yield $class->getDocComment().$class->getModifiers(); | |||
There was a problem hiding this comment.
$class->getModifiers() can change value at runtime, e.g. on PHP 5.5
| @@ -149,6 +154,16 @@ private function generateSignature(\ReflectionClass $class) | |||
| yield print_r($defaults, true); | |||
| } | |||
| } | |||
| < 8000 /td> |
|
||
| if ($class->isSubclassOf(EventSubscriberInterface::class)) { | |||
There was a problem hiding this comment.
I'm wondering whether this interface coming from another component should really be hardcoded here. This means that any other package providing a similar feature has no way to invalidate the container.
Could we make a reusable implementation (using a different resource probably), which would work for such cases ?
Note that I have such use case in my own project, and I will soon have it in KnpMenuBundle too.
There was a problem hiding this comment.
@stof that's possible, but that's a new feature. Not the target here, but could be done in another PR.
There was a problem hiding this comment.
OK, actually, other packages can invalidate the cache, but suffer from the issue of invalidating them too often too.
Can you work on creating such new feature for 4.1 though ?
There was a problem hiding this comment.
Can you work on creating such new feature for 4.1 though ?
not quite now, please open an issue :)
c1e0963 to
9921f64
Compare
|
|
||
| if (!$r = $this->container->getReflectionClass($class)) { | ||
| throw new InvalidArgumentException(sprintf('Class "%s" used for service "%s" cannot be found.', $class, $this->currentId)); | ||
| } elseif (!$r->isSubclassOf(ServiceSubscriberInterface::class)) { |
| throw new InvalidArgumentException(sprintf('Service "%s" must implement interface "%s".', $id, $interface)); | ||
| if (!$r = $container->getReflectionClass($class)) { | ||
| throw new InvalidArgumentException(sprintf('Class "%s" used for service "%s" cannot be found.', $class, $id)); | ||
| } elseif (!$r->isSubclassOf(EventSubscriberInterface::class)) { |
9921f64 to
67e821b
Compare
|
It's not that weird, it's just a type check, that doesn't make it coupling. |
|
#25984 would happen on 4.1 anyways, isn't it? |
…tionClassResource (nicolas-grekas) This PR was merged into the 3.4 branch. Discussion ---------- [Config] Handle Service/EventSubscriberInterface in ReflectionClassResource | Q | A | ------------- | --- | Branch? | 3.4 | Bug fix? | yes | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | #25906 | License | MIT | Doc PR | - @weaverryan that should fix an issue you reported. Commits ------- 67e821b [Config] Handle Service/EventSubscriberInterface in ReflectionClassResource
@weaverryan that should fix an issue you reported.