Problem/Motivation

#2551549: Deprecate per-table prefixing deprecated per-table prefixing in Drupal 8.2.0, for removal in Drupal 9. This did not happen and therefore we now try to remove it before the release of Drupal 10.0.

A problem is that the SQLite driver uses the $this->prefixes for something else and that functionality should be preserved.

Proposed resolution

The release manager (@catch) made the decision that we will not add a deprecation message to sites that are using per-table prefixing as it would create an enormous number of deprecation messages to such a site. Instead a hook_requirements() warning will be added to warn that support for per-table prefixing will be removed before the release of D10. In D10 the warning will be changed to an error.
For the running of migration tests, will a new connection info key added called: "extra_prefix". This new key is marked as @internal. As such it can be removed at any time from core.

Remaining tasks

None

User interface changes

None

API changes

None

Data model changes

None

Release notes snippet

TBD

CommentFileSizeAuthor
#91 Screen Shot 2020-05-15 at 22.19.59.png126.69 KBshaktik
#85 3106531-85.patch39.51 KBmondrake
#85 interdiff_79-85.txt7.22 KBmondrake
#83 3106531-83.patch39.77 KBmondrake
#83 interdiff_79-83.txt6.55 KBmondrake
#79 3106531-79.patch41.53 KBmondrake
#78 interdiff_77-78.txt3.39 KBmondrake
#78 3106531-78.patch40.43 KBmondrake
#77 3106531-77.patch39.49 KBmondrake
#76 interdiff_74-76.txt1.29 KBmondrake
#76 3106531-76.patch39.27 KBmondrake
#74 3106531-74.patch38.43 KBmondrake
#72 interdiff-3106531-67-72.txt1.44 KBdaffie
#72 3106531-72.patch38.36 KBdaffie
#68 3106531-67.patch38.02 KBmondrake
#68 interdiff_62-67.txt4.31 KBmondrake
#62 interdiff_54-62.txt16.38 KBmondrake
#62 3106531-62.patch35.18 KBmondrake
#54 3106531-54.patch29.94 KBmondrake
#54 interdiff_45-54.txt24.42 KBmondrake
#49 interdiff_45-49.txt17.56 KBRithesh BK
#49 3106531-49.patch28.76 KBRithesh BK
#45 3106531-45.patch10.72 KBmondrake
#45 interdiff_43-45.txt6.88 KBmondrake
#43 3106531-43.patch4.8 KBmondrake
#38 3106531-38.patch45.5 KBdaffie
#37 3106531-37.patch47.26 KBdaffie
#35 3106531-35.patch45.12 KBmondrake
#35 interdiff_34-35.txt645 bytesmondrake
#34 3106531-34.patch45.11 KBmondrake
#34 interdiff_33-34.txt1.76 KBmondrake
#33 interdiff_31-33.txt737 bytesmondrake
#33 3106531-33.patch45.66 KBmondrake
#31 3106531-31.patch45.81 KBmondrake
#31 interdiff_29-31.txt4.84 KBmondrake
#29 3106531-29.patch46.93 KBmondrake
#27 interdiff_23-25.txt1.61 KBMokshit06
#25 3106531-25.patch47.03 KBdaffie
#23 3106531-23.patch46.11 KBdaffie
#21 3106531-21.patch45.31 KBdaffie
#16 3106531-16.patch42.48 KBdaffie
#14 3106531-14.patch42.48 KBdaffie
#12 3106531-12.patch40.54 KBdaffie
#11 3106531-11.patch40.46 KBdaffie
#10 3106531-10.patch37.65 KBdaffie
#9 3106531-9.patch37.11 KBdaffie
#8 3106531-8.patch28.22 KBdaffie
#7 3106531-7.patch23.03 KBdaffie
#4 3106531-4.patch6.06 KBdaffie

Issue fork drupal-3106531

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

daffie created an issue. See original summary.

daffie’s picture

Adding #2949229: SQLite findTables Returns Empty Array on External DB. as related, because both issues work on the same class variable $this->prefixes.

daffie’s picture

Priority: Normal » Critical

According to @catch:

removing bc stuff should be critical though in general.
daffie’s picture

First patch. Lets see what the testbot has to say.

Berdir’s picture

> removing bc stuff should be critical though in general.

Hm, we'd have tons of critical issues then. I'd say it is enough to make this part of #2716163: [META] Remove deprecated classes, methods, procedural functions and code paths outside of deprecated modules on the Drupal 9 branch which is already critical.

We also have #3097879: Remove all @deprecated code in \Drupal\Core\Database, so possibly these two could be merged.

There was also an issue where this deprecation/removal has been considered problematic by some, would need to try and find it.

daffie’s picture

We also have #3097879: Remove all @deprecated code in \Drupal\Core\Database, so possibly these two could be merged.

I think it would be better do not merge this issue. I think this issue is complicated enough on its own.

There was also an issue where this deprecation/removal has been considered problematic by some, would need to try and find it.

Is this the issue you are revering to #2306013: Multisite is a valued feature that will not be deprecated.?

daffie’s picture

daffie’s picture

daffie’s picture

FileSize
37.11 KB
daffie’s picture

FileSize
37.65 KB
daffie’s picture

FileSize
40.46 KB
daffie’s picture

FileSize
40.54 KB

Status: Needs review » Needs work

The last submitted patch, 12: 3106531-12.patch, failed testing. View results

daffie’s picture

Status: Needs work » Needs review
FileSize
42.48 KB

This one should pass the testbot. Still a lot of work needs to be done.

Status: Needs review » Needs work

The last submitted patch, 14: 3106531-14.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

daffie’s picture

Status: Needs work » Needs review
FileSize
42.48 KB

Status: Needs review » Needs work

The last submitted patch, 16: 3106531-16.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

catch’s picture

There's definitely been pushback against removing multi-site, but I don't think there's been any against removing per-table prefixing which only has very, very specific use-cases.

Berdir’s picture

There has been some discussions on the per-table-prefixes too, but mostly it has been mixed up with multisite yes.

It was mentioned a bit in #3004496: Improve multisite compatibility with composer for example, and based on that, #3022403: Warn users about per-table prefix in multisite during requirements hook was created, which didn't happen yet. We could still do that in parallel while removing it in D9.

catch’s picture

So there was one person defending per-table prefixes on #3004496: Improve multisite compatibility with composer, first they insisted it was necessary to pull information from different databases (which is what multiple database connections are for), then they said you can use it as a 'working but risky' solution for sharing content (which was true with Drupal 6, no longer now that we have dynamic entity schema and configurable fields).

I think we can note that objection but continue, since as pointed out there are better solutions for all these things and we've not found a single site actually using per-table prefixes any more yet (I do know of older examples, but nothing from the past five years or so).

We should try to get #3022403: Warn users about per-table prefix in multisite during requirements hook yes.

daffie’s picture

Completely removed the functionality for attaching to external database for SQLite with the prefix connection option.
If somebody, and I doubt that there is such a person, who uses that functionality. It would be better to create a second database connection for the other SQLite database.

Status: Needs review » Needs work

The last submitted patch, 21: 3106531-21.patch, failed testing. View results

daffie’s picture

Status: Needs review » Needs work

The last submitted patch, 23: 3106531-23.patch, failed testing. View results

apaderno’s picture

The SQLite test failed because of this.

1) Drupal\Tests\Scripts\TestSiteApplicationTest::testInstallScript
Failed asserting that 0 is greater than 0.

/var/www/html/core/tests/Drupal/Tests/Scripts/TestSiteApplicationTest.php:137

Mokshit06’s picture

FileSize
1.61 KB

I have created an interdiff file for the comment 23 and 25

catch’s picture

Status: Needs review » Needs work

Looks like several sqlite test failures.

mondrake’s picture

mondrake’s picture

Status: Needs review » Needs work

I think the SQLite failures are due to too much removals. We still need to have attached databases, one per prefix, and to manage the destruction once all tables are dropped. Otherwise the runtime database and the SUT database collide, and havoc happens.

mondrake’s picture

Let's see this

Status: Needs review » Needs work

The last submitted patch, 31: 3106531-31.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

mondrake’s picture

Status: Needs work » Needs review
FileSize
45.66 KB
737 bytes
mondrake’s picture

Status: Needs review » Needs work

The last submitted patch, 35: 3106531-35.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

daffie’s picture

Removed a bit too much from the SQLite driver. We need the attach database part. It is for not having prefixes being applied multiple times.

daffie’s picture

Status: Needs review » Needs work
mondrake’s picture

@daffie I have no time for a review now, but just wanted to share this thought:

+++ b/core/lib/Drupal/Core/Database/Database.php
@@ -211,18 +211,9 @@
     if (empty($info['driver'])) {
       $info = $info[mt_rand(0, count($info) - 1)];
     }
-    // Parse the prefix information.
     if (!isset($info['prefix'])) {
       // Default to an empty prefix.
-      $info['prefix'] = [
-        'default' => '',
-      ];
-    }
-    elseif (!is_array($info['prefix'])) {
-      // Transform the flat form into an array form.
-      $info['prefix'] = [
-        'default' => $info['prefix'],
-      ];
+      $info['prefix'] = '';
     }
     return $info;

this could be

+      $info['prefix'] = $info['prefix'] ?? '';

but here it could be a good place to put a deprecation @trigger_error if $info['prefix'] in input is an array, and maybe convert the 'default' key to the now-single prefix.

Not sure what to do in case of a live site with settings.php having per-table prefixes set. Maybe an error or warning in the status report?

daffie’s picture

Some info about the patch from comment #38:

  1. Added to the class Drupal\Core\Database\Connection a new variable named: $prefix. For storing a single prefix value. Defaults to an empty string.
  2. Also added to the same class a new method called: getPrefix(). It return the value of the $prefix class variable.
  3. Also in the same class the method: tablePrefix() has been deprecated.
  4. In the class Drupal\Core\Database\Driver\sqlite\Connection, I have tried to remove the whole ATTACH DATABASE stuff. It made use of the $prefixes variable. However the code is needed, because it makes it possible to link to tables with the construction: $prefix.$table_name. The added dot gives us the knowledge that the table has been prefixed. If we remove this code the table name can get prefixed multiple times.
  5. Also in the same class the method: attachDatabase() has been added. It makes it possible to add extra prefixes to a SQLite database. It is necessary for testing with migrations.

Unfortunately I have a problem running SQLite tests locally. See: https://github.com/geerlingguy/drupal-vm/issues/2007. If somebody else could fix the last testbot failure, that would be great.

mondrake’s picture

+++ b/core/modules/simpletest/src/TestBase.php
@@ -1236,9 +1236,9 @@ private function restoreEnvironment() {
 
     // Remove all prefixed tables.
     $original_connection_info = Database::getConnectionInfo('simpletest_original_default');
-    $original_prefix = $original_connection_info['default']['prefix']['default'];
+    $original_prefix = $original_connection_info['default']['prefix'];
     $test_connection_info = Database::getConnectionInfo('default');
-    $test_prefix = $test_connection_info['default']['prefix']['default'];
+    $test_prefix = $test_connection_info['default']['prefix'];
     if ($original_prefix != $test_prefix) {
       $tables = Database::getConnection()->schema()->findTables('%');
       foreach ($tables as $table) {

Not sure how do we deal with this now that simpletest has been removed from core. This hunk should to be removed from the patch, but the overall change then would impact the contrib module. Shall we keep $connection_info['default']['prefix']['default'] in core so that contrib will not break on a missing array index, or?

mondrake’s picture

Here's an alternative, lighter, approach, that does not impact API and addresses points raised in #40 and #42.

We still allow the 'prefix' key of the connection options to be either a string or an array. If the array contains anything else than the 'default' key, then we assume that per-table prefixes are defined, and we throw a deprecation error. To cover the case of migrate tests that are adding (wrongly IMHO) an additional prefix only to allow SQLite to attach the original db, we move that prefix to an 'extra_prefix' in the connection options and recombine that in the SQLite connection constructor.

daffie’s picture

+++ b/core/lib/Drupal/Core/Database/Database.php
@@ -211,19 +211,29 @@
+        @trigger_error('Per-table prefixing is deprecated in drupal:9.1.0 and is removed in drupal:10.0.0. See https://www.drupal.org/node/xxx', E_USER_DEPRECATED);

@mondrake: Per-table prefixing has been deprecated since 8.2 and should be removed in 9.0. See: https://www.drupal.org/node/2768219

mondrake’s picture

Thanks! Added a deprecation test and removed default.settings.php text.

mondrake’s picture

daffie’s picture

Status: Needs review » Needs work
  1. +++ b/core/lib/Drupal/Core/Database/Database.php
    @@ -211,19 +211,29 @@
    +    elseif (is_array($info['prefix'])) {
    +      $default = $info['prefix']['default'] ?? '';
    +      $tmp = $info['prefix'];
    +      unset($tmp['default']);
    +      if (count($tmp)) {
    +        @trigger_error('Per-table prefixing is deprecated in drupal:8.2.0 and is removed in drupal:9.0.0. See https://www.drupal.org/node/2768219', E_USER_DEPRECATED);
    +      }
    

    I think it is better to add a hook_requirements() where we generate a REQUIREMENT_ERROR if $info['prefix'] is an array.

  2. +++ b/core/lib/Drupal/Core/Database/Database.php
    @@ -211,19 +211,29 @@
    +    // Transform the flat form into an array form.
    +    $info['prefix'] = [
    +      'default' => $default,
    +    ];
    

    I think it is better to change it to:

        // Transform the flat form into an array form.
        $info['prefix'] = $default;
    
  3. +++ b/core/lib/Drupal/Core/Database/Driver/sqlite/Connection.php
    @@ -71,7 +71,7 @@ public function __construct(\PDO $connection, array $connection_options) {
    -    $prefixes = $this->prefixes;
    

    Can we rename the variable $this->prefixes to $this->prefix and for SQLite add one like $this->extra_prefixes.

  4. @mondrake: If you do not want to remove all the per-table prefixing from core in this issue when do you want to remove it? Or do you want to leave some parts in core?
Rithesh BK’s picture

Assigned: Unassigned » Rithesh BK
Status: Needs work » Active
Issue tags: +VbContribution2020

currently working on it ....

Rithesh BK’s picture

Assigned: Rithesh BK » Unassigned
Status: Active » Needs review
FileSize
28.76 KB
17.56 KB

Please find the updated patch and interdiff file ......

daffie’s picture

Status: Needs review » Needs work

The patch does not apply, so back to need work.

apaderno’s picture

It seems a CI error, not an error in applying the patch, nor test errors.

mondrake’s picture

Assigned: Unassigned » mondrake

Working on #47.2

mondrake’s picture

Version: 9.0.x-dev » 9.1.x-dev
Assigned: mondrake » Unassigned
Status: Needs work » Needs review

So, bumping to 9.1.x at this stage, also because if per-table prefixing was deprecated in 8.2 when there was not yet a proper deprecation policy, and we are reflecting that here, deprecating the array form of the 'prefix' key, per #47.2, even if it makes sense, it's a new thing.

mondrake’s picture

Status: Needs review » Needs work

The last submitted patch, 54: 3106531-54.patch, failed testing. View results

daffie’s picture

  1. +++ b/core/lib/Drupal/Core/Database/Connection.php
    @@ -197,6 +197,17 @@ public function __construct(\PDO $connection, array $connection_options) {
    +        @trigger_error('Per-table prefixing is deprecated in drupal:8.2.0 and is removed in drupal:10.0.0. See https://www.drupal.org/node/2768219', E_USER_DEPRECATED);
    ...
    +        @trigger_error('Passing the table prefix as an array is deprecated in drupal:9.1.0 and is removed in drupal:10.0.0. Pass the prefix as a single string of the database connection \'prefix\' key instead. See https://www.drupal.org/node/2768219', E_USER_DEPRECATED);
    

    Here you are saying that we will be removing per-table prefixing in Drupal 10.0 and if I look at the rest of the patch we are removing per-table prefixing right now. What is it: now or in Drupal 10.0?

  2. +++ b/core/modules/migrate/tests/src/Kernel/Plugin/MigrationPluginListTest.php
    @@ -135,14 +135,14 @@ public function testGetDefinitions() {
    +      $connection_info[$target]['extra_prefix'][$value['prefix']] = $value['prefix'];
    

    Can we change $value['prefix'] for $prefix? It would improve the readability.

  3. +++ b/core/lib/Drupal/Core/Database/Driver/sqlite/Connection.php
    @@ -71,7 +71,7 @@ public function __construct(\PDO $connection, array $connection_options) {
    +    $prefixes = array_merge($this->prefixes, $this->connectionOptions['extra_prefix'] ?? []);
    

    Here you add a new connection options key "extra_prefix". Could we document and add testing for this.

mondrake’s picture

@daffie AFAICS we missed the window to do these removals in D9, so I am proposing to postpone them to D10. I do not know really though, maybe we need release manager input here.

This would leave us with the possibility to properly deprecate in D9. Note we need to deprecate two things:

1) Per-table prefixing, like

   'prefix' => [
     'default'   => 'main_',
     'users'     => 'shared_',
     'sessions'  => 'shared_',
     'role'      => 'shared_',
     'authmap'   => 'shared_',
   ],

this was theoretically deprecated in D8.2, but never enforced. So, you are right, #54 is actually removing the functionality, not just deprecating, and that needs to be fixed back.

2) The normalized, internal use, form of the 'prefix' key as an array, like

   'prefix' => [
     'default'   => 'main_',
   ],

to become

   'prefix' => 'main_',

this is a 'new' thing.

So we need to adjust everything in order to trigger deprecations if per-table prefix or array form is passed, but still retaining current behaviour.

WRT #47.1, I would suggest a separate issue to decide whether we want an hook_requirements() implementation to report about the changes in the structure - that would aim to inform site owners about actions to take, whereas here we are deprecating for developers' actions.

daffie’s picture

If we follow the Drupal deprecation rules we only can deprecate the per-table prefixing and wait for Drupal 10.0 for actually removing the functionality. Unless a release manager says otherwise. I do not like the whole per-table prefixing and I would like to get rid of it. Only to me that shall have to wait for Drupal 10.0. :(
What we also can do is to add the extra_prefix stuff for SQLite and changing migration to using that.

catch’s picture

I think we could still have removed this prior to beta (and backported the proper deprecations to Drupal 8.9) but yes it is really too late now. So it'd be a case of getting the proper deprecations done here, then a 10.0.x issue to actually remove.

mondrake’s picture

Priority: Critical » Normal

Back to normal priority then.

mondrake’s picture

Title: Remove per-table prefixing from core » Properly deprecate per-table prefixing
mondrake’s picture

This, more or less, should do, also #56.2, #56.3, and #47.2

daffie’s picture

Status: Needs review » Needs work

@mondrake: Thank you for working on this!

  1. Now we are adding the connection option key "extra_prefix" as new functionality. We should add testing and documentation for it. Can we deprecate it or is that not possible because migrate is using it?
  2. I find 4 @trigger_error() and only 3 @expectedDeprecation. I think we are missing one @expectedDeprecation.
  3. +++ b/core/lib/Drupal/Core/Database/Connection.php
    @@ -197,8 +197,42 @@ public function __construct(\PDO $connection, array $connection_options) {
    +    // @todo remove logic for 'extra_prefix' in D10.
    
    @@ -318,8 +352,9 @@ public function getConnectionOptions() {
    +   * @todo in D10, remove handling of array-based $prefix
    
    +++ b/core/lib/Drupal/Core/Database/Database.php
    @@ -211,19 +211,31 @@
    +    // @todo remove logic for parsing legacy array form in D10.
    

    Can we create a followup for actually removing the per-table prefixing and linking these @todo's to that followup.
    It is just not only removing deprecated code. The class variables: Drupal\Core\Database\Connection::$prefixes, Drupal\Core\Database\Connection::$prefixSearch and Drupal\Core\Database\Connection::$prefixReplace have to be removed and Drupal\Core\Database\Connection::$prefix has to be created. Or do you want to do that in this issue?

mondrake’s picture

Re #63:

1. We need that key while we have SQLite attaching additional prefix based databases in the constructor, which AFAICS is only due to migrate + the per-table prefixing. The first case is due to stay, the second should be dropped in D10. IMHO we should end up not having that key, rather a specific method in the SQLite to attach additional databases, only for migrate. But all that would require a separate issue, later, I think. I would not document that key full dots and bars - it's more a workaround that we should not encourage to use.
2. Indeed, next patch.
3. Filed #3124382: Remove per-table prefixing that I will link to in the next patch. I would not touch those properties and related methods in D9 cycle because they still have to support multi-prefixing and contrib drivers may rely on those.

daffie’s picture

For #64.1: Can we mark the whole connection options key "extra_prefix" as @internal then and for Drupal 10.0 it will only be there for SQLite.

mondrake’s picture

#65 where to do that, in some docs? What would you suggest, @daffie?
IMHO we should try to get to D10 without the need of that key - rather a generic method that adds the additional database, it will be a no-op for all drivers, with an implementation for SQLite only. Then we could use that method in D9 for looping through the extra_prefix keys, when we drop those in D10 the method will be remaining only for use by migrate. We need a followup here for doing all that :)

daffie’s picture

IMHO we should try to get to D10 without the need of that key

I agree with you for 100%! Only how do we get there.

I like your idea with an implementation for SQLite only. If we do that we can then use another connection optins key. Something like "sqlite_extra_databases". Then it is very clear that this is a SQLite specific thing. Lets do that in a followup and add a @todo with a link to the followup for the 2 migration instances where they are now using the "extra_prefix" key. When core does not use the "extra_prefix" key, we can safely remove it in Drupal 10.0.

For the mark "extra_prefix" as @internal:

+++ b/core/lib/Drupal/Core/Database/Connection.php
@@ -197,8 +197,42 @@ public function __construct(\PDO $connection, array $connection_options) {
+        // If there are keys left besides the 'default' one, we are in a
+        // multi-prefix scenario (for per-table prefixing, or migrations).
+        // In that case, we put the non-default keys in a 'extra_prefix' key
+        // to avoid mixing up with the normal 'prefix', which is a string since
+        // Drupal 9.1.0.

Add an extra comment with that the "extra_prefix" key is @internal. Also mentioning it in the CR.

mondrake’s picture

Status: Needs work » Needs review
FileSize
4.31 KB
38.02 KB

EDIT - xposted with #67

mondrake’s picture

#67

If we do that we can then use another connection optins key. Something like "sqlite_extra_databases".

I need to figure out, but IMHO we should not have a connection key, at all. Actually, I realise that migrate needs that for testing only?

Please let's discuss that in another issue, it will definitely require more discussion.

daffie’s picture

Issue summary: View changes
Related issues: +#3124382: Remove per-table prefixing
daffie’s picture

Issue summary: View changes
daffie’s picture

This patch only add 2 @todo's.

All the code changes look good.
A number of followups are created.
All deprecation messages are tested.
For me it is RTBC.

mondrake’s picture

Issue tags: -Needs change record
mondrake’s picture

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 74: 3106531-74.patch, failed testing. View results

mondrake’s picture

Status: Needs work » Reviewed & tested by the community
FileSize
39.27 KB
1.29 KB

Fixes to failure in #74 - due to new added test cases with legacy array form of the 'prefix' key.

mondrake’s picture

FileSize
39.49 KB
mondrake’s picture

mondrake’s picture

mondrake’s picture

mondrake’s picture

catch’s picture

Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs release manager review

For the reasons given in #3022403: Warn users about per-table prefix in multisite during requirements hook I'm not sure we should actually @trigger_error() here, or if we do maybe only in 9.3.x (or whichever is the last release to introduce new deprecations). hook_requirements() seems more appropriate since this is a site-admin configuration issue not a contrib/custom code one.

Is it possible to do a version of a patch with all the other clean-up but without the actual trigger_error() for now?

mondrake’s picture

Status: Needs review » Needs work

The last submitted patch, 83: 3106531-83.patch, failed testing. View results

mondrake’s picture

mondrake’s picture

Issue title and summary need an update, if we confirm we are pursuing #82 now.

daffie’s picture

Title: Properly deprecate per-table prefixing » Prepare for deprecation of per-table prefixing in 9.3.x
Issue summary: View changes
daffie’s picture

Status: Needs review » Reviewed & tested by the community
Issue tags: -Needs issue summary update

All the @trigger_error()'s have been removed from the patch.
The issue title and summary are updated.
Back to RTBC.

mondrake’s picture

Status: Reviewed & tested by the community » Needs work

Needs code style fixes.

mondrake’s picture

Status: Needs work » Reviewed & tested by the community

CS failure in #85 was unrelated to the patch here.

shaktik’s picture

Hi,
I am not sure why is trying to update/unlink when applying the patch.
warning: unable to unlink 'sites/default/default.settings.php': Permission denied
sites/default/default.settings.php getting some Permission error.
attach screenshot
default.settings.php

daffie’s picture

Hi @shaktik: When you use Drupal in some sort of production/developer environment to run Drupal as a website, the permissions for the mentioned can get changed. Like when you use DrupalVM. Apply the patch to a clean git cloned project, then it will work fine.

xjm’s picture

Status: Reviewed & tested by the community » Needs review

I'm still not sure about the approach of waiting until 9.3 and then deprecating this. Even in 9.3, you'll still be in this situation:

That said, that would result in dozens/hundreds of those deprecation messages for anyone who actually has that, because if it happens, it is a global sitewide thing that won't be easy to get rid of, you can't just update some code but likely have to rearchitecture your site. Assuming someone actually managed to make this work reliable, which I very much doubt, which is why we deprecated it in the first place.

(From @Berdir in the linked issue.) Given the nature of that, and the scope of the change, I think a hook_requirements() warning that in D10 becomes an error is more the way to go. I also am not sure we should wait around until 9.3.x to notify people, since this could be a signifcant amount of work for impacted sites. (Also, there's a high chance that we won't remember to do this in 9.3.x anyway.)

That part of my review might make most of the rest irrelevant, but some formatting issues on the @todos:

  1. +++ b/core/lib/Drupal/Core/Database/Connection.php
    @@ -208,12 +210,49 @@ public function __construct(\PDO $connection, array $connection_options) {
    +    // @todo deprecate the 'prefix' option as an array in 9.3.x.
    
    +++ b/core/lib/Drupal/Core/Database/Database.php
    @@ -216,17 +216,27 @@
    +    // @todo deprecate the 'prefix' option as an array in 9.3.x.
    +    // @todo https://www.drupal.org/project/drupal/issues/3124382 remove logic
    +    // for parsing legacy array form in D10.
    

    (And elsewhere.) The sentence should start with a capitalized word. Also, we should put the link to the followup issue in the @todo (and if it doesn't exist yet, file it.)

  2. +++ b/core/lib/Drupal/Core/Database/Connection.php
    @@ -208,12 +210,49 @@ public function __construct(\PDO $connection, array $connection_options) {
    +    // @todo https://www.drupal.org/project/drupal/issues/3124382 remove logic
    +    // for parsing 'extra_prefix' in D10.
    

    This sentence also should be capitalized. It's also our standard to indent the second and subsequent lines of a @todo (which in inline comments means an extra two spaces between the // and the start of the text).

  3. +++ b/core/lib/Drupal/Core/Database/Database.php
    @@ -216,17 +216,27 @@
    +      // to avoid mixing up with the normal 'prefix', which is a string since
    +      // Drupal 9.1.0.
    

    I don't think we need to mention the release that we expect this to be changed in as part of the comment. I would just remove "since Drupal 9.1.0".

mondrake’s picture

Status: Needs review » Postponed

Version: 9.1.x-dev » 9.2.x-dev

Drupal 9.1.0-alpha1 will be released the week of October 19, 2020, which means new developments and disruptive changes should now be targeted for the 9.2.x-dev branch. For more information see the Drupal 9 minor version schedule and the Allowed changes during the Drupal 9 release cycle.

Version: 9.2.x-dev » 9.3.x-dev

Drupal 9.2.0-alpha1 will be released the week of May 3, 2021, which means new developments and disruptive changes should now be targeted for the 9.3.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

daffie’s picture

daffie’s picture

Status: Postponed » Needs work

The time for warning site owners that per-table prefixing will be deprecated in D10 with a requirements hook should have been done in D9.2 or earlier. As 9.3.x is now open and 9.2.x is now closed, adding the warning is no longer possible. Therefore unpostponing this issue.

The current patch fails to apply.

mondrake’s picture

Status: Needs work » Needs review

Rerolled and changed to MR workflow. However, I doubt this will move on before #3022403: Warn users about per-table prefix in multisite during requirements hook is done.

mondrake’s picture

mondrake’s picture

fixes in MR

catch’s picture

I think we should do hook_requirements() first (and possibly leave things there), since the trigger_error() would run every single request so could flood logs.

Gábor Hojtsy’s picture

Title: Prepare for deprecation of per-table prefixing in 9.3.x » Deprecate per-table database prefixes in Drupal 9.3.x for removal in Drupal 10.0

Making title clearer. Also we can add a check in Upgrade Status for extra measure. Opened #3214337: Warn about per-table database prefixes deprecated in Drupal 9.3.x for removal in Drupal 10.0.

daffie’s picture

Status: Needs review » Needs work

The hook_requirements() needs to be added to the MR, also there are 2 unresolved threads.

andypost’s picture

Title: Deprecate per-table database prefixes in Drupal 9.3.x for removal in Drupal 10.0 » Prepare for deprecation of per-table prefixing in 9.3.x
Issue tags: +Needs release manager review
andypost’s picture

Title: Prepare for deprecation of per-table prefixing in 9.3.x » Deprecate per-table database prefixes in Drupal 9.3.x for removal in Drupal 10.0

revert title change

andypost’s picture

catch’s picture

I think we can probably merge this with #3022403: Warn users about per-table prefix in multisite during requirements hook now since we're probably going to end up with hook_requirements() + upgrade status, and no trigger_error().

daffie’s picture

Issue summary: View changes

In #3106531-103: Notify in Status Report that per-table database prefixes are no longer supported, and will throw errors in Drupal 10.0 and #3106531-109: Notify in Status Report that per-table database prefixes are no longer supported, and will throw errors in Drupal 10.0 the release manager @catch made the decision that there will be no trigger_error() for the deprecation of per-table prefixing. Therefore having a seperate issue for adding a hook_requirements() prior to 9.3 and adding a trigger_error() in another is no longer necessary. Therefore the other/blocking issue is closed.

Back to work on the MR.

daffie’s picture

daffie’s picture

Issue tags: +Needs reroll

It would be great to get this in in 9.3, so that we can remove it in D10.

mondrake’s picture

Removed deprecation logic and tried add hook_requirements logic in the system module to check for settigns.php files with no longer supported 'prefix' entries in array format.

mondrake’s picture

Status: Needs work » Needs review
Issue tags: -Needs reroll
daffie’s picture

Status: Needs review » Needs work

Setting the status back to NW, for the unresolved threads on the MR.

mondrake’s picture

Status: Needs work » Needs review
Issue tags: +Needs followup

OK, added a test for the hook_requirements implementation. I couldn't get to get writeSettings to work, though.

We need a follow-up issue for D10 to tighten up the 'prefix' key to be a string, not an array, and fail hard if not.

mondrake’s picture

Issue tags: -Needs followup

Follow-up exists already, #3124382: Remove per-table prefixing.

mondrake’s picture

We're not deprecating anything anymore, so we need IS, IT and CR updated.

mondrake’s picture

Using writeSettings() instead of plain adding to the settings.php file.

daffie’s picture

Issue summary: View changes
Status: Needs review » Needs work
Issue tags: -Needs issue summary update, -Needs change record updates

Updates the IS and the CR.
Replied to the thread on the MR.

mondrake’s picture

Status: Needs work » Needs review
mondrake’s picture

Title: Deprecate per-table database prefixes in Drupal 9.3.x for removal in Drupal 10.0 » Notify in Status Report that per-table database prefixes are no longer supported, and will throw errors in Drupal 10.0
daffie’s picture

Status: Needs review » Reviewed & tested by the community

It all looks good to me now.
All use of prefix with an array is replaced.
An hook_requirements() warning has been added.
The IS and the CR are in order.
For me it is RTBC.

@mondrake: Thank you for working on this issue.

mondrake’s picture

Issue summary: View changes

Actually, per-table prefixes are deprecated since Drupal 8.2.0, see below from default.settings.php, line 152:

 * Per-table prefixes are deprecated as of Drupal 8.2, and will be removed in
 * Drupal 9.0. After that, only a single prefix for all tables will be
 * supported.

and the deprecation CR was published 5 years ago.

Only, we never enforced that in code, nor did any cleanup in D9. So, we are not deprecating anything here, just preparing for removal.

Updated IS accordingly.

mondrake’s picture

Issue summary: View changes
catch’s picture

Status: Reviewed & tested by the community » Needs review

The issue appears to be doing two things:

1. Adding a hook_requirements() for the status report - this we should do asap.

2. Refactoring the code (and updating docs) to make the support easier to remove in 10.x. Apart from keeping the changes between branches smaller (which might be enough reason), I'm not entirely sure why we need to do that vs. removing support directly in 10.x

Could we split the issue into two?

mondrake’s picture

Status: Needs review » Postponed
mondrake’s picture

I moved the hook_requirements part of the MR over to #3022403: Warn users about per-table prefix in multisite during requirements hook. See you there.

mondrake’s picture

Status: Postponed » Needs review

#3022403: Warn users about per-table prefix in multisite during requirements hook proved that we cannot do the hook_requirements implementation in isolation, refactoring is anyway required, so this is a working solution at this stage. Moving back to NR.

daffie’s picture

Status: Needs review » Reviewed & tested by the community

The solution that was proposed by @catch does not work, unfortunately. Changing the status back to RTBC.

longwave’s picture

I am not sure we should remove the docs from settings.php until the feature is actually removed from core.

  • catch committed d79e4a6 on 9.3.x
    Issue #3106531 by mondrake, daffie, Rithesh BK, catch, xjm: Notify in...
catch’s picture

Status: Reviewed & tested by the community » Fixed

I am not sure we should remove the docs from settings.php until the feature is actually removed from core.

I thought the same initially reading the patch, but since it's site-admin facing documentation as opposed to code documentation, I think we should - we don't want to inform people about this feature at all, we want them to stop using it. For existing sites using the feature, if there are any, they won't read updated settings.php documentation but they will see the hook_requirements() warning. So in the end, it seems like the right change.

Committed d79e4a6 and pushed to 9.3.x. Thanks!

Status: Fixed » Closed (fixed)

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

Berdir’s picture

heddn’s picture

FWIW, parsing the connection options array also changed with this commit. Nothing mentioned in the CR about that. Not sure how to word it either, so leaving a comment here.

$connection_options['prefix']['default'] use to be the prefix when calling $connection_options = Connection::getConnectionOptions(). Now it is is simply:
$connection_options['prefix']

b_sharpe’s picture

How does one split tables into different databases now? We were using prefix to move large tables to a secondary database but I'm not sure how this can be accomplished after 9.3?

b_sharpe’s picture

It appears that using extra_prefix will still allow this to work; however, based on it being marked @internal I'm worried about using that going forward.

The use case here to split certain tables out from the main database, not really prefixing at all but was a supplied function of the prefixes, for example we have a site that has over 10 million entities, so for the ease of not bringing all that cache over from environment to environment, we split out all cache tables to a different database, we also have other tables that are from some contrib modules that produce a large amount of data not needed from environment to environment, thus we split those as well...

I'm aware services can define database arguments; however, this functionality is far more useful as it allows the modules not needing to know what database to use themselves and puts the control to the developer.

Any suggestions/help on this would be greatly appreciated.

Berdir’s picture

There is no direct replacement for this, it was removed on purpose as it allows weird, non-supported use cases like sharing tables between multiple drupal instances (a classic use case was sharing the user table between multiple sites, but with role config, entity queries, caching and so on, that just can't work anymore).

So yes, if you want to move tables elsewhere, you need to work with a separate connection/services.

That said, sharing cache tables sounds super scary if I understood that correctly, what if the data between environments is different?

I recommend using a separate cache backend like redis or memcache for any non-trivial site, they are separate (or not, but again, don't recommend that) fixed-size cache backends that take load away from your database server.

b_sharpe’s picture

@Berdir I think you're misunderstanding. I'm not trying to share data, I'm trying to separate it. Doing a database dump with 40GB of cache that I DONT need in other environments is cumbersome to do and move around.

I realize with cache I can use a separate backend to accomplish this, but many other mechinisms are not set in this way and would need to override a ton of services to set the DB target. We have several "Big Data" tables that are over 100GB and will rarely ever be needed on a local machine or dev/staging, so we'd prefere no to have to dump these and like to keep them separate.

My guess is I should start a new issue thread here as a feature request for separating tables directly as it really has nothing to do with prefixes and just happens to work due to the way SQL can select between DBs on the same server.

Berdir’s picture

I guess I misunderstood then yes, if not exporting those tables is the requirement then one option is to use the --structure-tables-list/key arguments in case you use drush sql-dump.

That doesn't change that this feature has been removed on purpose and is unlikely to be added back and another form to do what you want. Database drivers are pluggable, I guess you could try to implement that yourself?