[PropertyInfo] throw exception if docblock factory does not exist #26265
Conversation
xabbuh
commented
Feb 22, 2018
| Q | A |
|---|---|
| Branch? | 3.4 |
| Bug fix? | yes |
| New feature? | no |
| BC breaks? | no |
| Deprecations? | no |
| Tests pass? | yes |
| Fixed tickets | #26259 |
| License | MIT |
| Doc PR |
| @@ -53,6 +53,10 @@ class PhpDocExtractor implements PropertyDescriptionExtractorInterface, Property | |||
| */ | |||
| public function __construct(DocBlockFactoryInterface $docBlockFactory = null, array $mutatorPrefixes = null, array $accessorPrefixes = null, array $arrayMutatorPrefixes = null) | |||
| { | |||
| if (!class_exists(DocBlockFactoryInterface::class)) { | |||
There was a problem hiding this comment.
Shouldn't this be interface_exists?
There was a problem hiding this comment.
good catch, in fact I wanted to use the DocBlockFactory class here as that's what is used below
|
@xabbuh 👍 I guess this is indeed not a BC break, but this doesn't solve the side effect I've described in #26259 (comment) When you have the phpDocumentor installed locally, because of another dependency like PHPUnit, this would not throw an exception locally or in testing, but it would throw an exception on production without #26260. |
|
That's true, but I don't think we can cover that in Symfony itself. |
|
@xabbuh OK. What's your understanding of how that should be handled in the projects using the component? You should require a library noted in the docs even though you're not using its code directly, but it's used from your dependency? Shouldn't the |
| @@ -53,6 +53,10 @@ class PhpDocExtractor implements PropertyDescriptionExtractorInterface, Property | |||
| */ | |||
| public function __construct(DocBlockFactoryInterface $docBlockFactory = null, array $mutatorPrefixes = null, array $accessorPrefixes = null, array $arrayMutatorPrefixes = null) | |||
| { | |||
| if (!class_exists(DocBlockFactory::class)) { | |||
| throw new \RuntimeException(sprintf('Unable to use the %s class as the phpdocumentor/reflection-docblock package is not installed.', __CLASS__)); | |||
|
This looks as a good improvement to me, but keen on hearing your thoughts on the above suggestion/question! Thanks! |
|
Thank you @xabbuh. |
…t exist (xabbuh) This PR was merged into the 3.4 branch. Discussion ---------- [PropertyInfo] throw exception if docblock factory does not exist | Q | A | ------------- | --- | Branch? | 3.4 | Bug fix? | yes | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | #26259 | License | MIT | Doc PR | Commits ------- 5cfceed throw exception if docblock factory does not exist