APY / APYDataGridBundle

Symfony Datagrid Bundle
MIT License
493 stars 343 forks source link

Don't require doctrine/mongodb #991

Closed plfort closed 6 years ago

plfort commented 6 years ago

We should not put "doctrine/mongodb" in the "require" section of composer.json, but in "require-dev" and in "suggest".

DonCallisto commented 6 years ago

Well, why? If this bundle supports mongodb - as it does - it should have the same requirements of doctrine/orm. What am I missing?

plfort commented 6 years ago

Because doctrine/mongodb requires "ext-mongo". I don't want to install it if I don't use it.

DonCallisto commented 6 years ago

So, the same should be true for doctrine/orm, isn't it? I mean, what if I use mongo and I don't want to use doctrine/orm: AFAIK doctrine/orm can't handle directly mongo, am I wrong?

plfort commented 6 years ago

Yes

DonCallisto commented 6 years ago

Well, it makes sense but I still be doubtful about constraint versions of those packages: how can we handle them?

plfort commented 6 years ago

I don't know exactly how to handle it, maybe the best way would be to split the project into 3 part (common,orm,odm) But for now we can just remove these 2 dependencies of the "require" section, add it in "require-dev" and let the user handle this manually (as before)

DonCallisto commented 6 years ago

But for now we can just remove these 2 dependencies of the "require" section, add it in "require-dev" and let the user handle this manually (as before)

Yes, maybe we should document it in the README

Moreover Travis CI should be fixed and doctrine/orm should be required "manually"

daum commented 6 years ago

Just ran into this issue as well we don't use mongodb but now can't get to the latest version of this because it required mongo.

DonCallisto commented 6 years ago

@plfort will you fix this or do I need to assign this issue to me?

I can fix it tomorrow if you want, otherwise, as this suggestion comes from you, you can take it directly. Let me know.

plfort commented 6 years ago

I wouldn't have time to deal with it this weekend, you can take it

DonCallisto commented 6 years ago

I will ask you for a review ASAP

DonCallisto commented 6 years ago

@daum @plfort read this https://matthiasnoback.nl/2014/04/theres-no-such-thing-as-an-optional-dependency/

I totally agree with Matthias: we should split our project. As long as it lives this way, we cannot get a rid of mongo or doctrine.

Can I close this?

plfort commented 6 years ago

This is exactly what I said:

I don't know exactly how to handle it, maybe the best way would be to split the project into 3 parts (common,orm,odm)

But in a first time we must remove this deps and let users handle it, as it

DonCallisto commented 6 years ago

But in a first time we must remove this deps and let users handle it, as it

I really don't agree: if the project stays as it is, we cannot let the user handle it.

plfort commented 6 years ago

Why can't we let the user handle it ? It was the case before you added this deps. You added this deps when you added tests to the project, IMO you should have added this in the require-dev section. We can't force people to install ext-pdo or ext-mongo if they don't need it. I agree that is not ideal but this is necessary until we split the project

DonCallisto commented 6 years ago

We can't force people to install ext-pdo or ext-mongo if they don't need it.

I don't want users to install the deps. I want users to install the deps with compatible versions. I don't want to have a list of issues that I cannot reproduce due to different combinations. That's the main reason behind my strong disagreement.

As you can see in #772: we are too "permissive" with doctrine/orm so we should "block" it untill the fix. If we remove doctrine/orm from requirements, we could face a lot of this problems and, moreover, it could be a nightmare to debug.

Don't you agree?

plfort commented 6 years ago

I agree but what do you suggest ?

DonCallisto commented 6 years ago

I would keep them as requirements or at least to be prepared to face a lot of issues due to non-compatible libraries. I don't know what's the better here: all seems to be a wrong choice but, maybe, we should checkout other bundle/libraries on GitHub that are not splitted (like the blogpost example) and see how the majority of them tackle this problem.

akerouanton commented 6 years ago

Hey here, I'm one of the current Gaufrette maintainer. I stumbled upon this issue while answering to a question on Symfony's slack earlier today. To use this package without mongo extension, I advised them to add doctrine/mongodb-odm to the replace section of their composer.json. This is a dirty hack, but it does the job.

As you have mentioned Matthias' post, which is talking about us, I would tell you what we did some time ago to solve the exact same problem you're facing. We thought about the split suggested in the blog post, but weren't really satisfied by this idea because we thought it would hurt maintainability, especially as we were (and still are) trying to do some refacto.

So rather than that, we decided to publish as many metapackages as adapters we maintain. These metapackages are github repos with only a composer.json file that requires knplabs/gaufrette plus libraries specific to each adapter (take a look here). In that way, the library doesn't have "hard" dependencies on any extension or sdk.

I think you could do something like that, it's quite easy and needs almost no special care (well, you have to remember they exist, and update their requirements/tags).

DonCallisto commented 6 years ago

Hi @NiR- thank you for your the suggestion. Just one question: if we publish two metapackages (one for doctrine, the other one for mongo), should we also remove the "main" (this) one from packagist? I mean, if anyone install this directly I see two possible issues:

But if we remove this (the main package) aswell, we will break a lot of installation that requires this package as a dependency.

What will you suggest, taking into account these notes?

BTW, if it's not possible to exclude this - as I suppose it won't - would you suggest to remove both mongo and doctrine from requirements?

Thanks in advance for your opinion.

slince commented 6 years ago

@DonCallisto Just like monolog's approach, it only requires the most basic, if you use other providers to suggest their own installation; you know that we almost do not use MongoDB

DonCallisto commented 6 years ago

I've drafted this https://github.com/APY/APYDataGridBundle/releases/tag/3.1.2