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

Add support to mongodb driver #123

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Add support to mongodb driver #123

wants to merge 2 commits into from

Conversation

promatik
Copy link
Contributor

Fix for Laravel-Backpack/CRUD#4000.

This adds support to MongoDB databases.

@tabacitu
Copy link
Member

tabacitu commented Dec 11, 2021

Since Backpack itself has unofficial support for MongoDB... yeah... I guess we should do this 😀 I just want it written down in history that once again we have to litter our code with conditionals for MongoDB... but yeah... I don't see any way around this... thank you @promatik .

I don't have a way to test this right now so let's please ask @MeerKatDev and/or @pxpm to give this a go, so we can merge it.

Cheers!

@tabacitu tabacitu requested review from pxpm and removed request for tabacitu December 11, 2021 05:52
@MeerKatDev
Copy link

I would avoid setting this \Jenssegers\Mongodb\Eloquent\Model::class as hardcoded model for mongodb, but rather make it the default of a configurable value.

@promatik
Copy link
Contributor Author

@MeerKatDev great suggestion 👌
We don't have a config right now on this package, adding one just to set the Model, I'll let @tabacitu decide on that 😬
(I agree with that btw)

Anyway @MeerKatDev, would you prefer to not have the base Model changed and change it manually?

@MeerKatDev
Copy link

MeerKatDev commented Dec 14, 2021

Anyway @MeerKatDev, would you prefer to not have the base Model changed and change it manually?

I think it's ok to keep it changed, because as of now and I think in the near future, that package is the standard for using mongodb

@tabacitu
Copy link
Member

This package will be rafactored/rewritten in a few months, so I don't think it's worth adding a config file for it. That file would be shortlived anyway. Plus, I bet 6 months from now Jenssegers\Mongodb will still be the de-facto package used for MongoDB, so I don't think these 6 months anybody's going to need to configure that. If they do, they can say so and we'll create a config file then.

I think this PR is ok as it is (all things considered 😅). I'm just waiting for at least one more person to test this and give a thumbs up, and I'll merge it.

Cheers!

@MeerKatDev
Copy link

MeerKatDev commented Dec 21, 2021

Ok, so one other thing I noticed is that there is no casting support. When I normally work with Laravel and MongoDB, objects need to be kept objects, because Laravel converts them to string automatically, and same goes for dates - they should be saved as ISODates.

@skylarr1227
Copy link

so-how do i make this work? same issue, and im not seeing any resolution here as to what was done or what needs to be done to make it work with mongodb....

@pxpm
Copy link
Contributor

pxpm commented Nov 28, 2022

@skylarr1227 you can use this PR as composer require --dev backpack/generators:"dev-mongodb-support as 3.3.99"

I've just merged master here so everything is up-to-date with current version, plus this fixes. I am not a MongoDB user myself, I am concerned I didn't fully understand MongoDB concepts to confidently say this 100% works.

It seems to work, I've tested this PR and it allow to generate without the errors .. but I didn't fully understood some requirements mentioned here.

If you give this branch a go, and this works as you expect, please let us know, maybe it help us to merge this into core with more confidence.

Cheers

@skylarr1227
Copy link

Absolutely, ill try this later tonight when i get home. Sorry i missed you asking <3.

@ShuriZma
Copy link

ShuriZma commented Jan 1, 2024

It's already been ages since this was put up (welp PR from 2021, last comment 2022) but hey. Here comes another tester!
I just pretty much copied the changes and the first thing that came up was that apparently the \Jenssegers\Mongodb\Eloquent\Model has been moved to \MongoDB\Laravel\Eloquent\Model.
After fixing that the commands execute without throwing exceptions.

Other than that the created Model is completely empty ofc and theres nothing to see on the crud.

@pxpm
Copy link
Contributor

pxpm commented Jan 11, 2024

Hey @ShuriZma thanks for the feedback.

We don't "officially" support mongodb, but we aim to have non-blockers or solutions if people want to use it with mongo. AFAIK it works, at least from my simple tests.

Working on this kind of stuff is time consuming. I don't think it makes sense having mentions of mongodb and other engines that we don't "officially" support, and making sure that the crud functions work too.

What I would probably do here was just to either:
a) create mongo db stubs in the package
b) create a tutorial or some gist with those stubs and mention it in docs/readme

I would probably go with option b). As you can see as of today there are already changes related to MongoDB\Laravel\Eloquent\Model.
Us not officially supporting it and really not using it, makes me wary of introducing specific mongo db code on our "supported" code.
We did it in the past, I still regret it until today. It's not a proper solution, it's a patch and those usually come back to bit us in the future.

Sorry this haven't been prioritized, and I don't think this will be in a near future, I just don't wanted to let you without an answer.

So here goes, my proposed solution for people that need this stubs at the moment is to publish the backpack stubs and change them accordingly to your needs

If you use them in multiple projects you can put them in a package that requires generators and has the views changed, then instead of requiring backpack/generators you will require your/package-with-the-changes. Your package will install backpack generators and publish the changed stubs from your package.

If someone puts this in a package more people could benefit from it, if it's maintained we can promote it as our "recommended solution for mongo users".

Cheers

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

Successfully merging this pull request may close these issues.

6 participants