gabordemooij / redbean

ORM layer that creates models, config and database on the fly
https://www.redbeanphp.com
2.31k stars 279 forks source link

Get instance of database connection #813

Closed flip111 closed 4 years ago

flip111 commented 4 years ago

At Multiple database here https://redbeanphp.com/index.php?p=/database

static calls are used to switch databases. Could it be made possible to get an object instance representing the database connection?

gabordemooij commented 4 years ago

Yes, the facade is just a convenient way to access it in a static way. You can work with a more traditional object oriented approach if you follow the non-static guidelines:

https://redbeanphp.com/index.php?p=/non_static

flip111 commented 4 years ago

Could this logic be refactored https://github.com/gabordemooij/redbean/blob/master/RedBeanPHP/Facade.php#L401-L417 so that it can be called non-statically ?

Or maybe a static factory function createToolbox that constructs everything for you like the code triggered by R:setup() but then doesn't set self:: variables but returns an object. Could this be added?

Lynesth commented 4 years ago

I'm not sure I understand what the goal is in the end. What "object" do you want returned exactly ? If the answer is a Toolbox, then you can already do it either by using what @gabordemooij mentionned:

$pdo     = new RPDO( $dsn );
$adapter = new DBAdapter( $pdo );
$writer  = new MySQL( $adapter );
$oodb    = new OODB( $writer );
$toolbox = new ToolBox( $oodb, $adapter, $writer );

Or by doing:

R::addDatabase('mydb', ...);
$toolbox = R::$toolbox['mydb'];
flip111 commented 4 years ago

I don't know why you put object in quotes like it's a strange thing. I'm just referring to normal php objects.

Is it possible to do

R::addDatabase('mydb', ...);
$toolbox1 = R::$toolbox['mydb'];
R::addDatabase('mydb2', ...);
$toolbox2 = R::$toolbox['mydb2'];

then you can already do it

The facade offers a lot of convenience that is not available in object style. To put it more graphically

static objects
setup things yourself no and likely not needed yes
methods setup things for you yes not available
Lynesth commented 4 years ago

Simply saying that you want "R::setup() to return an object" isn't clear to me. That's why I asked you to be more precise in your request and what you are trying to achieve.

What exactly do you want? An instance from a class that already exists in RedBean and that can be used as an object (Toolbox, Adapted, etc) or something else? What do you want to be able to do with that object?

If you want a Facade object so that you could do:

$facade = R::setup(...);
$bean   = $facade->dispense('bean');
$facade->store($bean);

Then it isn't possible and you should refer to this page: https://redbeanphp.com/index.php?p=/non_static to work directly with the different "core" parts of RedBean. The same example would then be something like this:

R::addDatabase('mydb', ...);
$toolbox = R::$toolbox['mydb'];
$redbean = $toolbox->getRedBean();
$bean = $redbean->dispense('bean');
$redbean->store($bean);

Note that no checks are performed to verify anything when you dispense and store in that example.


Regarding your question, yes you can set multiple databases and retrieve the toolbox for each of them using the key passed as the first argument of the addDatabase function.

flip111 commented 4 years ago

All the static methods have as side effect that they store data on the class. They are not pure factory functions. This is a problem for the same reason why people advocate against using the global keyword ... namely that you are "leaking" state in places you don't want.

If you want to use static methods, great everything works fine. If you want to avoid placing state on the class then you end up duplicating code to get to the level of convenience that redbean is offering in the facade class.

Lynesth commented 4 years ago

I don't want to enter a debate regarding static vs non-static here, I'm just trying to understand your issue, but you still didn't answer what it is you want.

flip111 commented 4 years ago

I did already answer this in the second comment. To make it more clearer for you.

Here some logic on how objects get constructed

https://github.com/gabordemooij/redbean/blob/master/RedBeanPHP/Facade.php#L401-L417

Here it gets appointed to the class

https://github.com/gabordemooij/redbean/blob/master/RedBeanPHP/Facade.php#L432

I would like to split this logic so that the logic of the first link is not coupled to the static nature of the second link. This is just an example ... i don't know to how far the extend of convenience that Facade provides.

The solution that you presented before, points back to the current situation of redbean php. I opened this issue to change something about the static nature of some parts of redbean. If you don't want to debate about static vs non-static it's pointless to reply on this issue because it's at the core of what i'm asking. If you want to talk about something else please open your own issue.

Lynesth commented 4 years ago

The object you want returned, what class do you want it to be an instance of?

flip111 commented 4 years ago

Some class that allows queries. Maybe the toolbox is the "highest level" class. I'm not sure.

Lynesth commented 4 years ago

Unfortunatly, the way RedBean is designed, you kinda have to use the Facade if you want to benefit from everything. There's a lot of different classes that all have their own job and all do different type of queries (from lowest to highest):

I left out a bunch of utility classes that are used by the Facade to do Matchup, Look, Tree, etc. and some more that are not really relevant here.

If you want access to the glitter of the Facade, you can only use static functions at the moment. If you want object orientedness only, then you'd have to use the classes I listed above directly, which gives you much less feature and security without having to write a lot of code.

The facade could be rewritten so that it would give access to an instance of itself directly, each instance having it's own toolbox so that each of them is linked to only one database connection. But unless that is something @gabordemooij is willing to do you are left with writing your own class by extending the Facade (and the SimpleFacadeBeanHelper class). It isn't very hard to do to be honest, so maybe it could be offered that as a plugin.


The vast majority of people using RedBean only work with one database at a time, so the whole R class kinda becomes an object for them (if that makes sense) so they don't care about static vs non-static.

And as Gabor once said:

The Facade was never meant to do this sort of thing, it's meant to be used in a monilithic system, like a simple webapp or CMS. The switching approach works fine for most simple scenarios (like a CMS with a backup DB), but you should not rely on it if you use a 'multi database' app. The original idea was that you can resort to the objects behind the facade for more advanced usage.

gabordemooij commented 4 years ago

@flip111 I am not sure why you post this here, is it real business concern or more like a philosophical purism issue?

If it's the first, we try to keep facade methods as thin as possible (most methods are just one line, some are just a couple lines long), unfortunately we cannot do so for addDatabase() because that is just an internal housekeeping function (although if you have suggestions to provide similar functionality elsewhere in a separate class feel free to provide a PR, please note that we have to remain backward compatible btw).

If it's the second (and I suspect it is, because you also mentioned 'globals'), then I have to admit RedBeanPHP is not really a pure object oriented solution and it does not strive to be one.

RedBeanPHP is more like a pragmatic library to help devs make a living by providing easy-to-use ORM functionality without trying to replace SQL (which is what makes most ORM difficult to use). Yes, it is somewhat object oriented, because 11 years ago when I started it, I happened to be somehwat of an OO purist - I regret that now, if I would rewrite RedBeanPHP today, it would not use classes and objects at all.

The reason for this is simple, PHP is not a pure object oriented language, therefore any attempt to move it beyond a certain point becomes tiresome. If this is the reason behind your post, then the solution might be to pick a more object oriented language to begin with. Luckily for you, I have written my own programming language which is just that, pure object oriented: https://citrine-lang.org/ - if you want pure objects, I see you over there ;-)

But hey look on the bright side, you now have a great excuse to use all those previously 'condemned' features of PHP: indulge yourself with globals, gotos and static method. Because of this tiny mental step, they are all allowed as of now! ;-)

flip111 commented 4 years ago

is it real business concern or more like a philosophical purism issue?

It's a real code concern.

unfortunately we cannot do so for addDatabase() because that is just an internal housekeeping function

https://www.redbeanphp.com/index.php?p=/database under "Multiple databases" it's first class presented to user facing API.

most methods are just one line, some are just a couple lines long

It's not surprising then that i ask about the logic of a longer method.

although if you have suggestions to provide similar functionality elsewhere in a separate class feel free to provide a PR

I think this is not a good proposition to start duplicating code in other classes. I was only asking to refactor the addDatabase method so that the logic is no longer tied to the static storage.

Actually it's a rather disappointing proposition. I already made my suggestion here which is move this code

https://github.com/gabordemooij/redbean/blob/master/RedBeanPHP/Facade.php#L401-L417 (more or less these lines, probably should extend to line 430)

somewhere else. Let's first discuss the suggestion before anyone opens up a PR to ... discuss the suggestion?

Whatever redbeanPHP strives to be, i think it should strive to make it more safe to avoid mixing up database connections and also still offer convenience offered in other parts of the API. If that's not something you are interested in you can just say no and close the issue.

Lynesth commented 4 years ago

@flip111 Just submit a PR to factor the part you want in its own function that would return a Toolbox and simply use that function in the addDatabase one afterwards. If it doesn't change anything and just split things that's most likely going to be merged.

gabordemooij commented 4 years ago

@Lynesth @flip111 well it looks simple enough to me. Just send a PR to demonstrate what you have in mind. Keep in mind the following constraints:

If it satisfies those conditions, I'm happy to merge it.

gabordemooij commented 4 years ago

Maybe I should turn this into a general rule, might save some discussions. ;-)

gabordemooij commented 4 years ago

I had to revert the code because it breaks the tests.

gabordemooij commented 4 years ago

https://travis-ci.org/github/gabordemooij/redbean/builds/675758421

gabordemooij commented 4 years ago

Anyway, I agree we should move this to a PR thread, I'll mark this issue as closing soon.

flip111 commented 4 years ago

thanks for merge

gabordemooij commented 4 years ago

@flip111 please note that I had to revert the merge because of the test breakage. Normally Travis-CI checks the on-the-fly but their integration needs to be updated.

flip111 commented 4 years ago

What needs to be done now?

Lynesth commented 4 years ago

@flip111 See comment on your PR.