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
Comment | File | Size | Author |
---|
Issue fork drupal-3106531
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:
- 3106531--prepare-for changes, plain diff MR !677
Comments
Comment #2
daffie CreditAttribution: daffie commentedAdding #2949229: SQLite findTables Returns Empty Array on External DB. as related, because both issues work on the same class variable
$this->prefixes
.Comment #3
daffie CreditAttribution: daffie commentedAccording to @catch:
Comment #4
daffie CreditAttribution: daffie commentedFirst patch. Lets see what the testbot has to say.
Comment #5
Berdir> 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.
Comment #6
daffie CreditAttribution: daffie commentedI think it would be better do not merge this issue. I think this issue is complicated enough on its own.
Is this the issue you are revering to #2306013: Multisite is a valued feature that will not be deprecated.?
Comment #7
daffie CreditAttribution: daffie commentedSecond patch. Lets see what the testbot has to say.
Comment #8
daffie CreditAttribution: daffie commentedComment #9
daffie CreditAttribution: daffie commentedComment #10
daffie CreditAttribution: daffie commentedComment #11
daffie CreditAttribution: daffie commentedComment #12
daffie CreditAttribution: daffie commentedComment #14
daffie CreditAttribution: daffie commentedThis one should pass the testbot. Still a lot of work needs to be done.
Comment #16
daffie CreditAttribution: daffie commentedComment #18
catchThere'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.
Comment #19
BerdirThere 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.
Comment #20
catchSo 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.
Comment #21
daffie CreditAttribution: daffie commentedCompletely 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.
Comment #23
daffie CreditAttribution: daffie commentedComment #25
daffie CreditAttribution: daffie commentedComment #26
apadernoThe SQLite test failed because of this.
Comment #27
Mokshit06 CreditAttribution: Mokshit06 at Google Code-In commentedI have created an interdiff file for the comment 23 and 25
Comment #28
catchLooks like several sqlite test failures.
Comment #29
mondrakeReroll of #25
Comment #30
mondrakeI 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.
Comment #31
mondrakeLet's see this
Comment #33
mondrakeComment #34
mondrakeComment #35
mondrakeComment #37
daffie CreditAttribution: daffie commentedRemoved a bit too much from the SQLite driver. We need the attach database part. It is for not having prefixes being applied multiple times.
Comment #38
daffie CreditAttribution: daffie commentedComment #39
daffie CreditAttribution: daffie commentedComment #40
mondrake@daffie I have no time for a review now, but just wanted to share this thought:
this could be
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?
Comment #41
daffie CreditAttribution: daffie commentedSome info about the patch from comment #38:
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.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.
Comment #42
mondrakeNot 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?Comment #43
mondrakeHere'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.
Comment #44
daffie CreditAttribution: daffie commented@mondrake: Per-table prefixing has been deprecated since 8.2 and should be removed in 9.0. See: https://www.drupal.org/node/2768219
Comment #45
mondrakeThanks! Added a deprecation test and removed default.settings.php text.
Comment #46
mondrakeComment #47
daffie CreditAttribution: daffie commentedI think it is better to add a hook_requirements() where we generate a REQUIREMENT_ERROR if $info['prefix'] is an array.
I think it is better to change it to:
Can we rename the variable
$this->prefixes
to$this->prefix
and for SQLite add one like$this->extra_prefixes
.Comment #48
Rithesh BK CreditAttribution: Rithesh BK as a volunteer and at Valuebound for Valuebound commentedcurrently working on it ....
Comment #49
Rithesh BK CreditAttribution: Rithesh BK as a volunteer and at Valuebound for Valuebound commentedPlease find the updated patch and interdiff file ......
Comment #50
daffie CreditAttribution: daffie commentedThe patch does not apply, so back to need work.
Comment #51
apadernoIt seems a CI error, not an error in applying the patch, nor test errors.
Comment #52
mondrakeWorking on #47.2
Comment #53
mondrakeSo, 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.
Comment #54
mondrakeComment #56
daffie CreditAttribution: daffie commentedHere 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?
Can we change
$value['prefix']
for$prefix
? It would improve the readability.Here you add a new connection options key "extra_prefix". Could we document and add testing for this.
Comment #57
mondrake@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
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
to become
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.Comment #58
daffie CreditAttribution: daffie commentedIf 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.
Comment #59
catchI 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.
Comment #60
mondrakeBack to normal priority then.
Comment #61
mondrakeComment #62
mondrakeThis, more or less, should do, also #56.2, #56.3, and #47.2
Comment #63
daffie CreditAttribution: daffie commented@mondrake: Thank you for working on this!
@trigger_error()
and only 3@expectedDeprecation
. I think we are missing one@expectedDeprecation
.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
andDrupal\Core\Database\Connection::$prefixReplace
have to be removed andDrupal\Core\Database\Connection::$prefix
has to be created. Or do you want to do that in this issue?Comment #64
mondrakeRe #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.
Comment #65
daffie CreditAttribution: daffie commentedFor #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.
Comment #66
mondrake#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 :)
Comment #67
daffie CreditAttribution: daffie commentedI 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:
Add an extra comment with that the "extra_prefix" key is @internal. Also mentioning it in the CR.
Comment #68
mondrakeEDIT - xposted with #67
Comment #69
mondrake#67
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.
Comment #70
daffie CreditAttribution: daffie commentedComment #71
daffie CreditAttribution: daffie commentedComment #72
daffie CreditAttribution: daffie commentedThis 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.
Comment #73
mondrakeComment #74
mondrakeRerolled after #3120096: Support contrib database driver directories in a fixed location in a module.
Comment #76
mondrakeFixes to failure in #74 - due to new added test cases with legacy array form of the 'prefix' key.
Comment #77
mondrakeRerolled, after #3126658: Support enclosing reserved words with brackets..
Comment #78
mondrakeAdjusted tests to the new start/end identifier quote.
Comment #79
mondrakeRerolled after commit of #3112476: Always set $info['namespace'] on database connection info
Comment #80
mondrakeComment #81
mondrakeThink this #3022403: Warn users about per-table prefix in multisite during requirements hook would still make sense to do.
Comment #82
catchFor 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?
Comment #83
mondrakePer #82.
Comment #85
mondrakeMissed an @expectedDeprecation .
Comment #86
mondrakeIssue title and summary need an update, if we confirm we are pursuing #82 now.
Comment #87
daffie CreditAttribution: daffie commentedComment #88
daffie CreditAttribution: daffie commentedAll the @trigger_error()'s have been removed from the patch.
The issue title and summary are updated.
Back to RTBC.
Comment #89
mondrakeNeeds code style fixes.
Comment #90
mondrakeCS failure in #85 was unrelated to the patch here.
Comment #91
shaktikHi,
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
Comment #92
daffie CreditAttribution: daffie commentedHi @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.
Comment #93
xjmI'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:
(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
@todo
s:(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.)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).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".
Comment #94
mondrakeLet’s do #3022403: Warn users about per-table prefix in multisite during requirements hook first, then.
Comment #97
daffie CreditAttribution: daffie commented#3211780: Deprecate Connection::queryTemporary() has landed.
Comment #98
daffie CreditAttribution: daffie commentedThe 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.
Comment #100
mondrakeRerolled 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.
Comment #101
mondrakeComment #102
mondrakefixes in MR
Comment #103
catchI 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.
Comment #104
Gábor HojtsyMaking 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.
Comment #105
daffie CreditAttribution: daffie commentedThe hook_requirements() needs to be added to the MR, also there are 2 unresolved threads.
Comment #106
andypostThe blocker mentioned in #94 is not landed #3022403: Warn users about per-table prefix in multisite during requirements hook
Comment #107
andypostrevert title change
Comment #108
andypostComment #109
catchI 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().
Comment #110
daffie CreditAttribution: daffie commentedIn #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.
Comment #111
daffie CreditAttribution: daffie commentedComment #112
daffie CreditAttribution: daffie commentedIt would be great to get this in in 9.3, so that we can remove it in D10.
Comment #113
mondrakeRemoved deprecation logic and tried add
hook_requirements
logic in thesystem
module to check for settigns.php files with no longer supported 'prefix' entries in array format.Comment #114
mondrakeComment #115
daffie CreditAttribution: daffie commentedSetting the status back to NW, for the unresolved threads on the MR.
Comment #116
mondrakeOK, 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.
Comment #117
mondrakeFollow-up exists already, #3124382: Remove per-table prefixing.
Comment #118
mondrakeWe're not deprecating anything anymore, so we need IS, IT and CR updated.
Comment #119
mondrakeUsing
writeSettings()
instead of plain adding to the settings.php file.Comment #120
daffie CreditAttribution: daffie commentedUpdates the IS and the CR.
Replied to the thread on the MR.
Comment #121
mondrakeComment #122
mondrakeComment #123
daffie CreditAttribution: daffie commentedIt 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.
Comment #124
mondrakeActually, per-table prefixes are deprecated since Drupal 8.2.0, see below from
default.settings.php
, line 152: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.
Comment #125
mondrakeComment #126
catchThe 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?
Comment #127
mondrakeI've reopened #3022403: Warn users about per-table prefix in multisite during requirements hook which was meant to do #126.1.
Comment #128
mondrakeI 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.
Comment #129
mondrake#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.
Comment #130
daffie CreditAttribution: daffie commentedThe solution that was proposed by @catch does not work, unfortunately. Changing the status back to RTBC.
Comment #131
longwaveI am not sure we should remove the docs from settings.php until the feature is actually removed from core.
Comment #133
catchI 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!
Comment #136
BerdirThis is causing problems, see #3251625: Including settings.php a second time and without same context can result in errors
Comment #137
heddnFWIW, 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']
Comment #138
b_sharpe CreditAttribution: b_sharpe at ImageX commentedHow 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?
Comment #139
b_sharpe CreditAttribution: b_sharpe at ImageX commentedIt 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.
Comment #140
BerdirThere 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.
Comment #141
b_sharpe CreditAttribution: b_sharpe at ImageX commented@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.
Comment #142
BerdirI 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?