Problem/Motivation
Schema::copyTable() was implemented because it was used at one point during Drupal 8 development. However it is no longer used, and can be safely removed from core.
Proposed resolution
Remove Schema::copyTable() method.
Remaining tasks
- Write a patch that removes the method, its uses, and tests from MySQL (and SQLite?)
- Write a change record since we're in late beta, and some developer may be confused.
- Consider follow-up issue regarding 8.1.x or 9.0.x, or add to meta discussion around database driver.
User interface changes
None
API changes
Yes. Schema::copyTable() will no longer be available in 8.0.0 for Drupal developers.
Original report by chx
Followup from #2056133: Add db_copy_table_schema The code wanted to be this:
if (!$this->tableExists($source)) {
throw new SchemaObjectDoesNotExistException(String::format("Cannot copy @source to @destination: table @source doesn't exist.", array('@source' => $source, '@destination' => $destination)));
}
if ($this->tableExists($destination)) {
throw new SchemaObjectExistsException(String::format("Cannot copy @source to @destination: table @destination already exists.", array('@source' => $source, '@destination' => $destination)));
}
$info = $this->getPrefixInfo($destination);
return $this->connection->query('CREATE TABLE `' . $info['table'] . '` (LIKE {' . $source . '} INCLUDING ALL)');
Damien has this to say:
Sadly, this is not going to be enough, because indexes and constraints are globally-named on PostgreSQL. The server is likely going to rename those during the copy process, and it will not match our table_name + constraint name convention anymore. PostgreSQL support is critically broken due to several other issues, so let's not hold off this patch with PostgreSQL... but please add a TODO here.
Comment | File | Size | Author |
---|---|---|---|
#9 | 2061879-9.patch | 7.78 KB | Palashvijay4O |
#8 | 2061879-8.patch | 3.76 KB | Palashvijay4O |
Comments
Comment #1
mradcliffeRelating to meta issue.
Comment #2
mradcliffeRemoving tag, added correct/changed component. Sorry.
Comment #3
chx CreditAttribution: chx commentedActually? The parent was motivated by the upgrade path which is no more. Instead of struggling with postgresql, i recommend rolling that back.
Comment #4
mradcliffeContrib may still want this functionality for whatever reason.
It also looks like, if implementing, we need to fix the preg_replace() in Connection::prepareQuery().
Comment #5
catchLet's drop the feature. If we're going to release 8.0.0 with postgres support we need all tests passing, and this could be added back in any minor release if there's a use case for it.
Bumping to critical since it is unless we do #2160433: [policy, no patch] Move PostgreSQL driver support into contrib.
Comment #6
mradcliffeUpdated issue summary. Might be a good Novice critical issue?
Comment #7
chx CreditAttribution: chx commentedI can confirm there never were any usage of this. Perhaps by the time entity storage was done it wasn't needed any more. Yes, this is novice.
Comment #8
Palashvijay4O CreditAttribution: Palashvijay4O commentedDone with the changes as mentioned.
Comment #9
Palashvijay4O CreditAttribution: Palashvijay4O commentedRemoved implementations of copyTable in other Schema classes . Please review .
Comment #10
chx CreditAttribution: chx commentedThanks for the quick patch! It looks correct:
Comment #11
webchickOk cool. We can possibly add this back later in 8.1.x+ if someone can work out a way to make it DB agnostic.
Committed and pushed to 8.0.x. Thanks!
Comment #13
webchick...and unpublished the previous change record at https://www.drupal.org/node/2062137.