Mark db_*() wrappers in database.inc as @deprecated for 9.x

I am 100% in favor of removing all of the schema API utility wrappers unconditionally.

catch, is this something you'd still allow or do we punt it?

Putting on my radar then.

OK, let's try this.

1) Marks all of the schema-related db_* functions as deprecated. One or two were already unused.
2) Removes all uses of those functions in non-.install files. Per catch, I'm ignoring those on the assumption that code is going to be deleted anyway so I didn't bother converting it.
3) Does not actually remove the functions in question, as that can't happen until the update hooks go bye bye.

In a few places, I also converted some non-schema db_* function calls to methods on an object, mostly for consistency and to ensure that we're using the same connection.

The changes are almost all in test classes (no surprise) or in the update system (Ibid.). In some test classes I directly accessed $this->container. In others I did that in the constructor and persisted the object. Mostly that came down to how many calls there were and if I subjectively felt that it was worth the extra LOC.

In some places I used the container, in others I used Database::getConnection(). For the most part, I used the container but in some test classes there is no container so I had to use Database directly. I probably missed a few places where I should have used Database rather than the container. I will let Mr. Testboto find those for me.

Well, that was embarrassing...

For a moment I was confused that this is not a new service. Internally sure it makes sense to be part of the DB connection, but would it maybe makes sense as a separate accessible service (internal it should asks the db connection)?

Dur. Why am I able to install locally but testbot can't? H8 that...

The way the DB system is setup, it really shouldn't be a service. The schema is designed as a dependent object of the Connection. Refactoring the construction logic around the database layer now that we have a proper DIC, which we didn't in Drupal 7, is something we definitely should do; however, we tried and chx wasn't able to get it to not break testbot due to 15000 connections being created and destroyed. I think we're punting on that for Drupal 8.

7: 1894396-db_schema-wrappers.patch queued for re-testing.

Would it be fair to say we should change the scope of this issue to just mark those functions as @deprecated for D9?

Maybe better to separate which ones to remove before 9.x and which in 8.x
Schema is mostly dead for all field* functions better to remove with schema remains

Any db_*() function is legacy at this point and we want to remove it. I'd be happy if someone just rerolled the last patch to mark them "deprecated, will remove in D9" and fixed up the remaining usages. Hell, even if we don't fix them up let's tag them deprecated.

Yes let's change the scope of the issue and deprecate everything we can in database.inc

Hope you don't mind. :-)

This patch deprecates all functions in database.inc, except for db_ignore_replica() which is not a wrapper function.

_db_create_keys_sql() is deleted because it is unused and it wraps a method that doesn't exist.

This patch adds type hinting in @params and other doc improvements to database.inc, because I had to look them up to be sure about the deprecation suggestions. I only added type hints I needed that were easily provable, and were relatively easy to discover without much work.

However, I also improved the documentation of the classes being wrapped here, and I'll file a follow-up on those.

Followup issue with improved docs: #2533234: Marginally improve docs for database system

+++ b/core/includes/database.inc
@@ -28,23 +28,28 @@
+ * @deprecated as of Drupal 8.0.x, will be removed before Drupal 9.0.0. Instead,
+ *   get a database connection from the container and call query() on it. E.g.
+ *   \Drupal::database()->query($query, $args, $options).

Technically I think it's "will be removed in Drupal 9.0.0", since we can't remove them before that.

Also, the long-form shown here is only useful in procedural code, and procedural code that calls the database is Wrong(tm) IMO because it is coupling to an SQL database outside of a swappable service.

I would rewrite this block as:

@deprecated as of Drupal 8.0.0, will be removed in Drupal 9.0.0. Instead, get a database connection injected into your service from the container and call query() on it. E.g.: $database->query($query, $args, $options);

The same applies to the other docblocks. (Not repeated for brevity.)

The other type fixes look fine, I think.

Hm, true. I'd still prefer to nudge people toward the injected approach. Seems overly verbose though to include both versions. Do we have somewhere to link out to instead of inlining an example each time?

Technically I think it's "will be removed in Drupal 9.0.0", since we can't remove them before that.

If it won't get removed before 9.x, then it's not deprecated. :-)

Also, the long-form shown here is only useful in procedural code, and procedural code that calls the database is Wrong(tm) IMO because it is coupling to an SQL database outside of a swappable service.

Agreed generally. It's the same container under \Drupal, though, or at least should be. In this case \Drupal is a shorthand for 'get it from the container.' The problem is that this docblock can't teach people what a container is.

@deprecated as of Drupal 8.0.0, will be removed in Drupal 9.0.0. Instead, get a database connection injected into your service from the container and call query() on it. E.g.: $database->query($query, $args, $options);

The problem with that is: Where do you get $database? Am I really expected to create a service in order to use the database?

Well that's still going to happen in hook_update_N() at least, even if it's not the common case.

hook_update_N() doesn't get injected with a container? Seems like a follow-up to me.

...

Marking 'needs review' so we can decide these points and then I can make a new patch.

If you're in hook_update_N(), then use Drupal::database() is your only option (other than these functions we're deprecating).

If you're in anything non-esoteric... yes, you should have a service that depends on the database via the container, and your service should be tagged so that it can be swapped out for a non-SQL implementation if appropriate. That is the expectation. Throwing random SQL queries around in unstructured places is what we're trying to get away from. :-) With the new Entity API, Entity Query, State API, KeyValue, Config, etc, quite frankly you probably shouldn't be talking to the database directly very often to begin with. Most modules won't have any SQL in them anymore, and that's a good thing!

"Where do you get $database?" From the constructor, via the container. Like everything else. I agree, teaching people about dependency injection and containers is out of scope for this docblock, but that doesn't mean we should instead be recommending bad practices to people. (And calling \Drupal from within an object is bad practice.) Hence why I recommend just saying $database->query(). The next logical question is "where do I get it?", which should lead people to existing documentation elsewhere on DI et al.

If you're in hook_update_N(), then use Drupal::database() is your only option (other than these functions we're deprecating).

Exactly. Sounds like a follow-up to inject the container that \Drupal is using. :-)

If you're in anything non-esoteric... yes, you should have a service that depends on the database via the container, and your service should be tagged so that it can be swapped out for a non-SQL implementation if appropriate.

OK. Where's that document? Particularly regarding appropriate tags to use.

https://api.drupal.org/api/drupal/core%21lib%21Drupal%21Core%21Database%... is way out of date.

Most modules won't have any SQL in them anymore, and that's a good thing!

+1.

Change notice: https://www.drupal.org/node/2306083

And yeah, those docs need work, too.

Latest change looks good to me. Thanks, Miles!

Updating the title with a more-correct scope change.

Docs are not frozen in beta. Committed 43d4637 and pushed to 8.0.x. Thanks!

publish change record