phpdocumentor/reflection-docblock dependency

[bobvandevijver]

We're in the process of upgrading to the latest form bundle, and I saw that the latest update of this project pull in phpdocumentor/reflection-docblock as dependency. This causes quite some additional dependencies to be pulled:

However, I believe that this might have been a mistake, and that the phpdocumentor/reflection-docblock should instead have been placed in the require-dev section. Is that correct?

[webda2l]

Hmm, to get the expected behavior (like using this PHPDoc: link), PropertyInfo requires phpdocumentor/reflection-docblock (Symfony docs).

I could move it to require-dev and suggest, but that would require adding a prominent note in the documentation—which might still be overlooked and lead to unexpected issues being opened by some users. So for now, I’m leaning against it.

[bobvandevijver]

I see. I'm was looking into this in context of the translation-form-bundle; as translations are all string based, so should be no need for complex PHPDoc type resolving. I can also not find any usages of the PhpDocExtractor in the source code; both bundles do have one in the tests.

The configuration does reference the property_info service, but that uses the globally configured extractor. So, if I'm correct, that will still not use the phpdoc extractor unless already configured. And if you've configured that, you should have added the phpdocumentor/reflection-docblock to your own composer file. That will automatically register the phpdocblock extractor when available, but is it really necessary? It seems to work fine without it in our use case.

In conclusion, I do believe that moving it to require-dev and suggest is the way to go here, WDYT?

[webda2l]

I understand that in your current case, the dependency phpdocumentor/reflection-docblock is not strictly required, but it may become necessary if someone uses other features of AutoFormBundle. So, the current decision to include this dependency improves DX/UX by preventing unexpected behavior of more advance uses.
However, I agree that this is debatable—it’s not a 100% perfect decision, more like 55% :)

The best approach is maybe for you to submit a PR with the changes you need and use your branch until the decision possibly evolves.

[bobvandevijver]

It also now forces people to install the reflection-docblock in their global property info service, even if they do not want to. I believe it would be a lot better to actually not use the global property info service, but to build a custom one based on configuration specifically for this (or the translation-form) bundle. That way, the service builder can throw an error when a configured dependency is missing (which is an approach taken by Symfony as well for quite some components).

[webda2l]

Good idea regarding the custom property-info service (indeed, I have in mind what APIP does with the serializer service, for example), but my concern/decision remains focused on improving DX/UX by default, handling a broader range of use cases for this bundle than your more limited approach, as previously mentioned.

[bobvandevijver]

You seem to be missing the point. The more default, non-configurable dependencies, the more your package becomes bloated and the worse your DX becomes, especially when you take all supply chain attacks into account you need to be on the lookout for nowadays. Also, when looking at the installs on packagist, I do believe the "simple" case (just strings in combination with the translation-form-bundle) is the main reason that this bundle is being installed.