Add a database service to the DIC

Crell-Approved.

Erp.

Are we just moving around code here? The diff looks identical on + and -.

Seems harmless enough...

Committed and pushed to 8.x. Thanks!

We're not actually using this anywhere yet. But what happens if I call db_set_active() before calling some methods from a class which has had the database connection injected via the DIC? Will it use the active connection or the original one?

Um. I don't think the DIC is compatible with db_set_active(), honestly. I think what happens is whichever connection is active when you first call the DB out of the DIC will be what gets used for the database and database.slave keys forevermore.

Can we just get rid of db_set_active()? I wanted t do that in Drupal 7, too. It's an ugly legacy approach.

We can get rid of it, I just don't think we can keep both, which is why I re-opened this ;)

++

Uh. What the heck? db_set_active() is an incredibly useful function when pulling in results from external databases. Why would we remove this feature?

Because db_set_active() is toggling global state, which is an increasingly bad idea that is going to break more things as we move forward. Also, you shouldn't be using it anyway even now; you should be using Database::getConnection() to get the connection you want and then calling ->query() et all against it directly.

Can we not simply change the innards of db_set_active() so it just does that?

Also, I don't get this:

"Because db_set_active() is toggling global state"

It's simply calling Database::setActiveConnection(), same as what the patch is replacing calls to db_set_active() with. There's no global state involved..?

The whole point of db_set_active() is to change a global static variable. That is its entire reason to exist. Messing with a global static and using the DIC are not compatible approaches. We need to pick one. We pick the DIC.

What people want to say is that the task here is to remove setActiveConnection itself and db_set_active too. There are extremely few places left, just kill it.

The following two code are equivalent:

db_set_active('students')
db_query($query)
db_set_active()

Database::getConnection('default', 'students')->query($query);

Note: Views already does the latter.

There's another use case where you want to trick an existing module just using db_query to use another database. We need to make sure those wrappers are doing drupal_container()->get('database') and then instead of the superb rare db_set_active one can just change the service in the DIC.

Yep that's what I was after.

One more question, but I don't think we need to do it here, would be good to discuss in a follow-up though.

Why are we hard-coding the default and slave connections here? It feels like it should be OK to loop through the $databases global and register each available connection as a service?

There's another use case where you want to trick an existing module just using db_query to use another database. We need to make sure those wrappers are doing drupal_container()->get('database') and then instead of the superb rare db_set_active one can just change the service in the DIC.

Yep I agree with this too, that's the main use-case for db_set_active() that's not covered by getConnection but since we can do that with the DIC it should be fine.

Since we've already discussed moving the db_* functions out of database.inc, we could move them to bootstrap.inc and replace their guts with container()->get('database') (or whatever). Then they become dumb wrappers for the DIC just like a number of other functions have become. We then deprecate them when we're able to, same as everything else.

catch: Eh, mostly because we went with the simple-for-now approach. I still want to refactor the Database class into a DatabasManager object, not a static class, which would then be more flexible. I'm fine with improving the service keys, and then either exposing database.manager as a service and/or iterating over known connections and creating a database.$key.$target service for each. Actually the latter is probably best, as then we can always change the connection sent to a service by messing with the container.

So it sounds like the remaining tasks for this issue are:

1) Put the DB manager itself in the DIC. (Likely requires making that an object, not a static class.)
2) Put all possible DB connections in the DIC. Probably keep "database" and "database.slave" for the default DB, in addition to whatever else gets added.
3) Move db_*() to bootstrap.inc, and have them wrap drupal_container().
4) Remove db_set_active() and Database::setActiveConnection().

Sound like a plan?

Also refiling since this issue is no longer WSCCI related now that the main connection is in there.

That sounds good to me. If it's easier we could close this and start a new issue as a followup? But since the patch was about 10 lines in the first place it seemed OK to re-open in this case.

Updated the summary accordingly.

Bumping to major since this blocks #1764474: Make Cache interface and backends use the DIC and nearly every other move x to the DIC issue.

beejeebus, if you want to have a crack at this let me know and I'll try to get you up to speed on anything you need for part 1. (The rest should be fairly perfunctory.)

That's where lazy instantiation of connections from info arrays happens, if nothing else. Plus, if you want to mess with multiple database connections at once (migration and custom code?) it may be easier to just ask for the manager directly and get the connections yourself, since then you get to define which keys you want rather than relying on the DIC to get it right. (The DIC configuration will only be right 95% of the time.)

That's good, covers the second, rarer use case of #20 can we eliminate the first use there, completely killing db_set_active and setActiveConnection as well. This would drastically simplify the Database class as well.

Edit: erm, lays the groundwork for the second use case in #20 doesn't completely do it. Yet.

Don't want to disrupt the party, but the original/committed patch in #1 makes the database service available in the kernel only.

The database service has to be available outside of the HttpKernel in early bootstrap already. There's not necessarily a HTTP request/kernel involved.

We need to move the service registration into drupal_container().

Erf. Anything that happens pre-kernel needs to just go away. We've already reached that conclusion, just haven't gotten around to doing it yet.

Rather, I think that's a good argument for the DB manager to be available outside of the container, but once you're in the kernel you should be accessing it (or the connection objects it spawns) from the container.

Regarding the patch, my Dreditor is MIA right now but getConnectionsInfo() is guaranteed to be confused with getConnectionInfo(). :-)

At which point did we decide that the DIC is equal to the HttpKernel? They're separate concepts and components. Saying that one equals the other doesn't make sense to me.

There are plenty of legit use-cases that use the container, but don't want and don't need to bootstrap an HttpKernel. Saying that everything "pre-kernel" should be removed sounds very odd and like a false dichotomy. There's at least Drush, there are individual scripts, and there are unit tests. The latter being what revealed this glitch. (#1774388: Unit Tests Remastered™)

The fix is easy; the lines can be moved as-is, and the database service gets available, without the huge dependency of HttpKernel. That seems to be in line with #1703266: Use a precompiled container for the minimal bootstrap container required outside of page requests

The kernel has nothing to do with this issue. None. Instead of the corebundle you do this in the emergency container built in drupal_container. So here's the battle plan:

1. Add a DB service to drupal_container() per connection. We want a factory here not the direct class to avoid actually parsing the connection array.
2. Make every db_* function use the DIC().
3. Remove db_set_active() and db_set_active because we covered both cases in #20 by now.
4. Investigate how to use the testing DIC to avoid the connection add, rename, remove connections stuff. Discuss elinimating this functionality totally. If one needs it, they can manipulate the DIC directly.
5. Investigate the Database class of what remains. Not much, quite possibly only the factory.

Let's make that a followup. Let's not rush. Getting the database work from the DIC solely instead of the global-like Database class is one step forward.

I went ahead and did the database.inc-killing, since there's already a separate issue for it: #1773822-2: Consolidate low-level procedural helpers somewhere

Is this deprecated in favor of #1811730: Refactor Database to rely on the DIC solely? Should it be?

And it eliminates a major, too. Yay!

Add follow-up todo items.