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.

CommentFileSizeAuthor
#9 2061879-9.patch7.78 KBPalashvijay4O
#8 2061879-8.patch3.76 KBPalashvijay4O
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

mradcliffe’s picture

Component: postgresql database » ajax system
Issue summary: View changes
Issue tags: +PostgreSQL
Parent issue: » #2157455: [Meta] Make Drupal 8 work with PostgreSQL or remove support from core before release

Relating to meta issue.

mradcliffe’s picture

Component: ajax system » postgresql db driver
Issue tags: -PostgreSQL

Removing tag, added correct/changed component. Sorry.

chx’s picture

Actually? The parent was motivated by the upgrade path which is no more. Instead of struggling with postgresql, i recommend rolling that back.

mradcliffe’s picture

Contrib may still want this functionality for whatever reason.

It also looks like, if implementing, we need to fix the preg_replace() in Connection::prepareQuery().

catch’s picture

Title: Implement pgsql Schema::copyTable » Remove Schema::copyTable
Component: postgresql db driver » database system
Priority: Normal » Critical

Let'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.

mradcliffe’s picture

Issue summary: View changes
Issue tags: +Novice

Updated issue summary. Might be a good Novice critical issue?

chx’s picture

I 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.

Palashvijay4O’s picture

Status: Active » Needs review
FileSize
3.76 KB

Done with the changes as mentioned.

Palashvijay4O’s picture

FileSize
7.78 KB

Removed implementations of copyTable in other Schema classes . Please review .

chx’s picture

Status: Needs review » Reviewed & tested by the community

Thanks for the quick patch! It looks correct:

➜  system git:(8.0.x) ✗ git grep copyTable
core/includes/database.inc: * @see \Drupal\Core\Database\Schema::copyTable()
core/includes/database.inc:  return Database::getConnection()->schema()->copyTable($source, $destination);
core/lib/Drupal/Core/Database/Driver/fake/FakeDatabaseSchema.php:  public function copyTable($source, $destination) {
core/lib/Drupal/Core/Database/Driver/mysql/Schema.php:  public function copyTable($source, $destination) {
core/lib/Drupal/Core/Database/Driver/pgsql/Schema.php:  public function copyTable($source, $destination) {
core/lib/Drupal/Core/Database/Driver/sqlite/Schema.php:  public function copyTable($source, $destination) {
core/lib/Drupal/Core/Database/Schema.php:  abstract public function copyTable($source, $destination);
➜  system git:(8.0.x) ✗ curl -s https://www.drupal.org/files/issues/2061879-9.patch|git apply --index
➜  system git:(8.0.x) ✗ git grep copyTable
➜  system git:(8.0.x) ✗ git grep db_copy_table
➜  system git:(8.0.x) ✗  
webchick’s picture

Status: Reviewed & tested by the community » Fixed

Ok 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!

  • webchick committed 3227399 on 8.0.x
    Issue #2061879 by Palashvijay4O, chx: Remove Schema::copyTable
    
webchick’s picture

...and unpublished the previous change record at https://www.drupal.org/node/2062137.

Status: Fixed » Closed (fixed)

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