Problem/Motivation

Mostly everything in core/database.inc is deprecated starting from 8.0 but removal of usage require to @trigger_error() with a link to change record

This is a soft blocker for #2319859: [META] Deprecate contents of database.inc to deprecate remains

Also a soft blocker for lot of issues in #2848161: [meta] Replace calls to deprecated db_*() wrappers

Also this issue may patch all the places according to current deprecation standard https://www.drupal.org/core/deprecation as pointed in #2848817: Replace all calls to db_table_exists, which is deprecated.

Also setting default target is missing at "objectified" db subsystem #2947775: Move setting default target out of db_merge() and other deprecated db_* functions - probably makes sense to mention at in separate CR

Proposed resolution

  1. Create change record mentioning all deprecated db_* functions + #2286235: Deprecate db_ignore_replica() and convert it to service
  2. Identify common pattern of message
  3. add regression test to core/tests/Drupal/KernelTests/Core/Database/DatabaseLegacyTest.php for each function (use group @legacy to annotate exception
  4. Promote in core gdo about that all since 8.6 extra call to trigger_error() will happen for all deprecated functions like

Remaining tasks

- decide on message for exception
- file CR draft listing functions and which of them will start throw exceptions on usage
- create patch to fix all @deprecated content in core/database.inc

User interface changes

no

API changes

no

Data model changes

no

CommentFileSizeAuthor
#20 interdiff-2947946-19-20.txt16.16 KBvoleger
#20 2947946-20.patch90.56 KBvoleger
#19 2947946-19.patch86.97 KBvoleger
#15 interdiff-2947946-11-15.txt85.71 KBvoleger
#15 2947946-15.patch89.98 KBvoleger
#11 interdiff-2947946-09-11.txt906 bytesvoleger
#11 2947946-11.patch67.37 KBvoleger
#9 interdiff-2947946-07-09.txt67.86 KBvoleger
#9 2947946-09.patch68 KBvoleger
#7 2947946-07.patch12.54 KBvoleger

Comments

andypost created an issue. See original summary.

Version: 8.6.x-dev » 8.7.x-dev

Drupal 8.6.0-alpha1 will be released the week of July 16, 2018, which means new developments and disruptive changes should now be targeted against the 8.7.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

voleger’s picture

voleger
he/his
Primary language Ukrainian
Location Ukraine, Rivne
commented

For the legacy tests, we use \Drupal\KernelTests\Core\Database\DatabaseLegacyTest for now. See #2991542: Introduce a DatabaseLegacyTest class for deprecation testing

voleger’s picture

voleger
he/his
Primary language Ukrainian
Location Ukraine, Rivne
commented

Added initial change record for the db_* functions.
https://www.drupal.org/node/2993033
We need to mention all previously published CR related to deprecation of db_* functions.
Also, introduce examples of usage of alternative method to each function.

voleger’s picture

voleger
he/his
Primary language Ukrainian
Location Ukraine, Rivne
commented
Issue summary: View changes
mondrake’s picture

mondrake
Location 🇮🇹
commented
Status: Active » Needs review
Related issues: +#2991337: Document the recommended ways to obtain the database connection object

Edited the CR with a table of deprecated->new functions and examples on how to obtain the database connection objects taken from #2991337: Document the recommended ways to obtain the database connection object.

voleger’s picture

voleger
he/his
Primary language Ukrainian
Location Ukraine, Rivne
commented
StatusFileSize
new 2947946-07.patch12.54 KB

Here the path that fixes the related deprecation messages.
Should we change the original deprecation message for each function if any of them will contain the link to the CR?

mondrake’s picture

mondrake
Location 🇮🇹
commented

I like the idea to refer to the same issue for all the deprecated functions; however, I believe that per the Drupal core deprecation policy, we should also add a @see annotation in the PHPDoc block of all the functions, and the deprecation message in the @trigger_error should end with a '.' (full-stop).

But most important here we should finalise the CR, and I am not sure there is still consensus on the recommended way to get the database connection in the test code.

voleger’s picture

voleger
he/his
Primary language Ukrainian
Location Ukraine, Rivne
commented
StatusFileSize
new 2947946-09.patch68 KB
new interdiff-2947946-07-09.txt67.86 KB

Addressed #8, added @see annotation and fixed full stop signs.

Still blocked by #2991337: Document the recommended ways to obtain the database connection object

Status: Needs review » Needs work

The last submitted patch, 9: 2947946-09.patch, failed testing. View results

voleger’s picture

voleger
he/his
Primary language Ukrainian
Location Ukraine, Rivne
commented
Status: Needs work » Needs review
StatusFileSize
new 2947946-11.patch67.37 KB
new interdiff-2947946-09-11.txt906 bytes

Revert changes in expected deprecation message for db_set_active.

andypost’s picture

andypost
he/him
Primary language Russian
commented
Related issues: +#3024461: Adopt consistent deprecation format for core and contrib deprecation messages

Better to wait for common format in related issue 

+++ b/core/includes/database.inc
@@ -89,9 +89,10 @@ function db_query($query, array $args = [], array $options = []) {
+  @trigger_error('db_query_range() is deprecated in Drupal 8.0.x and will be removed before Drupal 9.0.0. Instead, get a database connection injected into your service from the container and call queryRange() on it. For example, $injected_database->queryRange($query, $from, $count, $args, $options). See https://www.drupal.org/node/2993033.', E_USER_DEPRECATED);

@@ -126,9 +127,10 @@ function db_query_range($query, $from, $count, array $args = [], array $options
+  @trigger_error('db_query_temporary() is deprecated in Drupal 8.0.x and will be removed before Drupal 9.0.0. Instead, get a database connection injected into your service from the container and call queryTemporary() on it. For example, $injected_database->queryTemporary($query, $args, $options). See https://www.drupal.org/node/2993033.', E_USER_DEPRECATED);

I'm sure that dot at the end should be removed to prevent links to be opened with it at the end 2993033.

Version: 8.7.x-dev » 8.8.x-dev

Drupal 8.7.0-alpha1 will be released the week of March 11, 2019, which means new developments and disruptive changes should now be targeted against the 8.8.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

voleger’s picture

voleger
he/his
Primary language Ukrainian
Location Ukraine, Rivne
commented
Title: Create change record for all deprecated db_* functions » [PP-1] Create change record for all deprecated db_* functions
Status: Needs review » Postponed

So lets set a proper status of this issue. Postponed on #3024461: Adopt consistent deprecation format for core and contrib deprecation messages

voleger’s picture

voleger
he/his
Primary language Ukrainian
Location Ukraine, Rivne
commented
Status: Postponed » Needs review
StatusFileSize
new 2947946-15.patch89.98 KB
new interdiff-2947946-11-15.txt85.71 KB

Actually #3024461: Adopt consistent deprecation format for core and contrib deprecation messages already adopted the messages structure and only blocked by the CS rule implementation. so we can go further here.

voleger’s picture

voleger
he/his
Primary language Ukrainian
Location Ukraine, Rivne
commented
Title: [PP-1] Create change record for all deprecated db_* functions » Create change record for all deprecated db_* functions
voleger’s picture

voleger
he/his
Primary language Ukrainian
Location Ukraine, Rivne
commented

The last db_query function calls replaced in the codebase, we need to publish CR.
Updated Introduced in version version value to 8.8.0.

berdir’s picture

berdir
Primary language German
Location Switzerland
commented
Status: Needs review » Needs work

I think the patch above needs to be rerolled?

voleger’s picture

voleger
he/his
Primary language Ukrainian
Location Ukraine, Rivne
commented
Status: Needs work » Needs review
StatusFileSize
new 2947946-19.patch86.97 KB

Reroll

voleger’s picture

voleger
he/his
Primary language Ukrainian
Location Ukraine, Rivne
commented
StatusFileSize
new 2947946-20.patch90.56 KB
new interdiff-2947946-19-20.txt16.16 KB

Fix few CS issues. This one should be ready to review.

The last submitted patch, 19: 2947946-19.patch, failed testing. View results

mondrake’s picture

mondrake
Location 🇮🇹
commented
Status: Needs review » Reviewed & tested by the community

Patch is good cleanup, RTBC. However, the CR still needs #2991337: Document the recommended ways to obtain the database connection object to be complete.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented

Crediting @andypost and @mondrake for reviews.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Reviewed & tested by the community » Fixed

Committed 8a56839 and pushed to 8.8.x. Thanks!

  • alexpott committed 8a56839 on 8.8.x 
    Issue #2947946 by voleger, andypost, mondrake: Create change record for...

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.

jonathan1055’s picture

jonathan1055 commented

There is a typo in the deprecation message for db_select. The example for replcement has ->db_select() when it should be just ->select(). When viewing the deprecation message we get the slightly confusing

db_select() is deprecated in Drupal 8.0.x and will be removed before Drupal 9.0.0. Instead, get a database connection injected into your service from the container and call select() on it. For example, $injected_database->db_select($table, $alias, $options);

The line in the commit above is
https://git.drupalcode.org/project/drupal/commit/8a56839#5a8148978c99bbf...

Would be nice to have the suggested example correct, but I realise that this might be too trivial for a fix in it's own right.