8000 PropertyInfo does not depend on phpdocumentor/reflection-docblock in require block · Issue #26259 · symfony/symfony · GitHub
[go: up one dir, main page]
Skip to content

PropertyInfo does not depend on phpdocumentor/reflection-docblock in require block #26259

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
hkdobrev opened this issue Feb 21, 2018 · 6 comments
Closed

PropertyInfo does not depend on phpdocumentor/reflection-docblock in require block #26259

hkdobrev opened this issue Feb 21, 2018 · 6 comments
Labels
Bug PropertyInfo Status: Needs Review
Milestone
3.4

Comments

@hkdobrev
Copy link
Contributor
hkdobrev commented Feb 21, 2018 •
edited
Loading
Q A
Bug report? yes
Feature request? no
BC Break report? no
RFC? no
Symfony version 3.4 and 4.0

The PropertyInfo component lists the phpdocumentor/reflection-docblock package as a developer dependency in require-dev:

symfony/src/Symfony/Component/PropertyInfo/composer.json

Lines 29 to 35 in 6651087

"require-dev": {
"symfony/serializer": "~3.4|~4.0",
"symfony/cache": "~3.4|~4.0",
"symfony/dependency-injection": "~3.4|~4.0",
"phpdocumentor/reflection-docblock": "^3.0|^4.0",
"doctrine/annotations": "~1.0"
},

However, the PhpDocExtractor class depends on an interface and a class from that package without you using it directly.

symfony/src/Symfony/Component/PropertyInfo/Extractor/PhpDocExtractor.php

Lines 54 to 56 in 6651087

public function __construct(DocBlockFactoryInterface $docBlockFactory = null, array $mutatorPrefixes = null, array $accessorPrefixes = null, array $arrayMutatorPrefixes = null)
{
$this->docBlockFactory = $docBlockFactory ?: DocBlockFactory::createInstance();

The PropertyInfo component documentation states:

PhpDocExtractor

This extractor depends on the phpdocumentor/reflection-docblock library.

https://symfony.com/doc/current/components/property_info.html#phpdocextractor

I believe when the non-tests related code of the component has a dependency on a package, it should be in require instead of require-dev.

This caused a problem for us in production when we had the documentor package installed in development, but not listed in neither our own require nor require-dev dependencies as we do not use it anywhere in our code.
@hkdobrev hkdobrev mentioned this issue Feb 21, 2018
Closed
@vudaltsov
Copy link
Contributor

Your message is controversial. If you don't use PhpDocExtractor, how could it become a problem?

And what's the difference between dev and prod environments concerning dependency's dev dependencies? They are not installed in both cases. So you should have noticed the problem while development.

@hkdobrev
Copy link
Contributor Author

I'll try to clarify, I see now my usecase is not fully reproducible from above:

mkdir property-info-deps && cd property-info-deps
composer require symfony/property-info ^3
composer require --dev phpunit/phpunit ^6

composer why phpdocumentor/reflection-docblock
# phpspec/prophecy  1.7.5  requires  phpdocumentor/reflection-docblock (^2.0|^3.0.2|^4.0)

composer why phpspec/prophecy
# phpunit/phpunit  6.5.6  requires  phpspec/prophecy (^1.7)

composer.json

{
    "require": {
        "symfony/property-info": "^3"
    },
    "require-dev": {
        "phpunit/phpunit": "^6"
    }
}

phpdocumentor/reflection-docblock is locked at 4.3.0.

Then I could have code like the following:

use Symfony\Component\PropertyInfo\Extractor\PhpDocExtractor;
use Symfony\Component\PropertyInfo\Extractor\ReflectionExtractor;
use Symfony\Component\PropertyInfo\PropertyInfoExtractor;


$listExtractors = [new ReflectionExtractor()];
$phpDocExtractor = new PhpDocExtractor();
$typeExtractors = array_merge($listExtractors, [$phpDocExtractor]);
$descriptionExtractors = [$phpDocExtractor];
new PropertyInfoExtractor($listExtractors, $typeExtractors, $descriptionExtractors);

In the above code, I don't use anything from the phpdocumentor/reflection-docblock package and I don't know it's a dependency at all. I don't have it in my code and I don't have it in my composer.json dependencies. I have it installed because of unrelated dev dependencies and I don't understand the code that I use uses something else when it works in development and testing environment.

If the symfony/property-info package is using an interface and a class by default without requiring me explicitly to pass them or use them somewhere I would expect that it would declare that as its dependency and work when I use its interface.

If I had PHP classes and interfaces in my use imports from dev dependencies or dependencies which are locked in composer.lock, but not listed as explicit dependencies in my code in composer.json, then I would understand the problem is in my project that it doesn't declare its dependencies correctly. But here the symfony/property-info package does not declare its dependencies based on its usage and interface.

@hkdobrev
Copy link
Contributor Author

Your message is controversial. If you don't use PhpDocExtractor, how could it become a problem?

So I use PhpDocExtractor, but I never use anything from phpdocumentor/reflection-docblock and I don't even know about it. However, by using PhpDocExtractor I would expect it has declared its dependencies. Its __construct signature allows to not pass arguments and it should work without hidden dependencies.

@Tobion
Copy link
Contributor
Tobion commented Feb 21, 2018

I see the problem you faced. But phpdocumentor/reflection-docblock is an optional dependency. Making it a requirement for everyone does not seem right either.

@hkdobrev
Copy link
Contributor Author
hkdobrev commented Feb 21, 2018 •
edited
Loading

But phpdocumentor/reflection-docblock is an optional dependency. Making it a requirement for everyone does not seem right either.

@Tobion I understand that and I think the PhpDocExtractor could be changed to depend on an interface of its own and make it a required constructor argument. However, given the current interface, I don't think the dependencies are correct.

Optional dependencies should be injected as an instance and not created on the fly. At least a class_exists would be in order if a fallback was possible.

@xabbuh xabbuh added this to the 3.4 milestone Feb 22, 2018
@xabbuh xabbuh mentioned this issue Feb 22, 2018
Merged
@xabbuh
Copy link
Member
xabbuh commented Feb 22, 2018

see #26265

nicolas-grekas added a commit that referenced this issue Feb 22, 2018
@nicolas-grekas
bug #26265 [PropertyInfo] throw exception if docblock factory does no…
c5f2942 
…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
@hkdobrev hkdobrev mentioned this issue Apr 3, 2018
Closed
@hkdobrev hkdobrev mentioned this issue Mar 25, 2019
Closed
@lamasfoker lamasfoker mentioned this issue Jul 31, 2020
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 PropertyInfo Status: Needs Review
Projects
None yet
Milestone
3.4
Development

No branches or pull requests
7 participants
@javiereguiluz @nicolas-grekas @hkdobrev @Tobion @xabbuh @vudaltsov @carsonbot
0