Skip to content
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.

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

Deprecate the drivers config node #227

Closed
wants to merge 1 commit into from

Conversation

mbabker
Copy link
Contributor

@mbabker mbabker commented Mar 17, 2022

As suggested in #61 (comment), this deprecates the "drivers" config node in favor of feature detection. The node only allows values supported by the resource bundle and causes confusion when folks try to create custom drivers (which can be done with the sylius.grid_driver tag without adding it to that config node), so it really doesn't offer any benefit to existing.

Instead, Symfony's API for detecting features available at compile time is used. If an app has both doctrine/orm and doctrine/doctrine-bundle installed, then the ORM and DBAL drivers will be automatically enabled. If an app has both doctrine/phpcr-odm and doctrine/phpcr-bundle installed, the (deprecated) PHPCR-ODM driver will be automatically enabled. The bundles are included in the dependency list because the driver services have dependencies to services created by the respective bundles, so the presence of the managers alone cannot fulfill the service requirements.

If accepted as is, the "drivers" config node is no longer used by the bundle at all. I guess this could be a minor inconvenience if someone is using this bundle in an application that has both the ORM and PHPCR-ODM (and their bundles) installed and was using the config to disable one of the drivers, but you can accomplish the same with a compiler pass if needed.

On a side note, to have a full suite of test scenarios that actually makes sure these optional integrations stay optional, the CI would really need to run the tests without each of the Doctrine dependencies installed (similar to the steps in the resource bundle CI), but that's going to be a much larger undertaking (especially for the deprecated PHPCR-ODM integration).

@vvasiloi
Copy link
Contributor

About testing, what if you wrap ContainerBuilder::willBeAvailable in a non-static method that could be mocked to return false?
It can be reduced to a DI test asserting that those services were not defined.

@mbabker
Copy link
Contributor Author

mbabker commented Mar 18, 2022

To do that adds a lot of overhead for minimal benefit IMO (as you now have to create a new service class, inject it to the DI extension, test that in isolation, rewrite the functional tests for the class to mock all 4 scenarios needed for full coverage, and then maintain it all as a public API because that “decorator to mock one static call” service is going to be placed somewhere so it’s not being duplicated in several repos). By design, it’s not an API to be mocked, it is making decisions based on the actual environment, which to me means those behaviors are best tested with something more complex than unit tests.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants