Don't remove abstract plugins #247
Conversation
These abstract definitions are needed for client plugin definitions, even if plugins weren't configured at root level. Removal of unused abstract definitions is job of DI container.
There was a problem hiding this comment.
thanks for this. i think its fine to just leave the abstract definition in - the container is cleaned up by symfony anyways, and abstract definitions should not pose any problem. much less complicated than having to figure out for each abstract plugin if we need it somewhere or not.
|
|
||
| /** | ||
| * @expectedException \Symfony\Component\Config\Definition\Exception\InvalidConfigurationException | ||
| * @expectedExceptionMessage The child node "cache_pool" at path "httplug.clients.test.plugins.0.cache" must be configured. |
There was a problem hiding this comment.
please use $this->expectException and $this->expectExceptionMessage instead of the annotations. the annotations have been deprecated in favor of the method. they have the upsides of being able to use the ::class constant and (not relevant in this case here) of only accepting the exception at the place you actually expect it, not before that.
There was a problem hiding this comment.
There is no instance in this code base where expectExceptionMessage is used. I've followed style of existing code.
Furthermore, expectException is PHPUnit 6+ only.
If you decided to change the code style, please do it yourself with refactoring commit which replaces it in all places at once.
There was a problem hiding this comment.
sorry, my bad. i am working in too many repositories that are at different states of upgrading those things... indeed, its better to be consistent with this. i created #248
|
|
||
| /** | ||
| * @expectedException \Symfony\Component\Config\Definition\Exception\InvalidConfigurationException | ||
| * @expectedExceptionMessage The child node "cache_pool" at path "httplug.plugins.cache" must be configured. |
|
I think this is ready to merge. @Nyholm @fbourigault do you agree with this change? |
|
Thank you! |
These abstract definitions are needed for client plugin definitions, even if plugins weren't configured at root level.
Allows to configure plugin this way
instead of forcing unneeded configuration at httplug.plugins.cache level. See linked issue.