search

sonata-project / SonataFormatterBundle

Symfony SonataFormatterBundle
https://docs.sonata-project.org/projects/SonataFormatterBundle
MIT License
81 stars 117 forks source link

Allow test to install doctrine-extensions 2 #711

Closed VincentLanglet closed 2 years ago

VincentLanglet commented 2 years ago

We will rely on the sonata-admin constraint instead.

VincentLanglet commented 2 years ago

Is it not used on this repo directly?

Only here: https://github.com/sonata-project/SonataFormatterBundle/blob/3b215a5c2636bb31a65da1583ce9ff089e6476e9/tests/App/AppKernel.php#L65 It's only added because it's a requirement from SonataAdmin https://docs.sonata-project.org/projects/SonataAdminBundle/en/4.x/getting_started/installation/#enable-the-bundle

So IMHO we should let SonataAdmin decide about the constraint

We can merge this now, and the next release of SonataAdmin will allow doctrine-extensions 2.

jordisala1991 commented 2 years ago

I am not sure about that, is there a real benefit?

If tomorrow SonataAdmin drops the requirements this builds will start failing.

Also I was trying to use automated tools to check wether a package has unused deps or missing deps (we could end up configuring them on github actions), this goes against that too.

The problem is knowing in which cases I should declare a dep or when it does not matter. If you need to configure something of that bundle then you would add it? To me is a rule that is really easy to forget. Easier is to add everything you use (and try to reduce the list by removing code).

Maybe we should focus on check why it is required to enable the bundle. Maybe is something that can be avoided? Twig extensions and form extensions also suffer from the same problem.

To me if the Pr is either remove the line or add a ^2, it wont be that big of a deal.

VincentLanglet commented 2 years ago

I am not sure about that, is there a real benefit?

If tomorrow SonataAdmin drops the requirements this builds will start failing.

Also I was trying to use automated tools to check wether a package has unused deps or missing deps (we could end up configuring them on github actions), this goes against that too.

Yeah I know this tool too.

To me if the Pr is either remove the line or add a ^2, it wont be that big of a deal.

Let's say one day SonataAdmin will drop ^1 we'll have to update here And let's say one day SonataAdmin will support ^3 we'll have to update here too. It seems less work in the futur.

But I can add ^2 if you prefer.

jordisala1991 commented 2 years ago

What happens if you remove it from the AppKernel.php?

VincentLanglet commented 2 years ago

What happens if you remove it from the AppKernel.php?

Service, config, extensions... won't exist if a bundle is not declared.

jordisala1991 commented 2 years ago

Just so we have the info, when you don't register this bundle you get:

RuntimeException: You must register SonataDoctrineBundle to use SonataMediaBundle.

So this requirement is not from the admin, but from the SonataMediaBundle (used in functional tests).