Problem/Motivation

If you use the Schema::dropTable() method the returned value should be the result of the query, but the returned query value is not verified and even if the query fail the method return TRUE.

mysql implementation:

    $this->connection->query('DROP TABLE {' . $table . '}');
    return TRUE;

postgre implementation:

    $this->connection->query('DROP TABLE {' . $table . '}');
    $this->resetTableInformation($table);
    return TRUE;

sqlite implementation:

    $this->connection->tableDropped = TRUE;
    $this->connection->query('DROP TABLE {' . $table . '}');
    return TRUE;

Proposed resolution

Its true that you always verify:

    if (!$this->tableExists($table)) {
      return FALSE;
    }

But you can't assume that if the table exist this will be delete with the code:

$this->connection->query('DROP TABLE {' . $table . '}');

So, you need to be sure that the table was dropped.

Remaining tasks

Test the patch.

User interface changes

None.

API changes

None.

Data model changes

None.

CommentFileSizeAuthor
#6 2862344-5.patch4.9 KBdaffie
#6 2862344-5-tests-only.patch2.72 KBdaffie
#4 return_the_returned_query_value_from_schema_droptable-2862344-4.patch2.35 KBadriancid
#2 return_the_returned_query_value_from_schema_droptable-2862344-2.patch2.57 KBadriancid
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

adriancid created an issue. See original summary.

adriancid’s picture

adriancid
Primary language Spanish
Location Montreal, Canada
CreditAttribution: adriancid as a volunteer and commented
Status: Active » Needs review
FileSize
return_the_returned_query_value_from_schema_droptable-2862344-2.patch2.57 KB
daffie’s picture

daffie CreditAttribution: daffie commented
Status: Needs review » Needs work
Issue tags: +API addition, +Needs change record

Looks good.

For all 3 methods:

+++ b/core/lib/Drupal/Core/Database/Driver/mysql/Schema.php
@@ -378,13 +378,22 @@ public function renameTable($table, $new_name) {
+    if($this->tableExists($table)) {
+      return FALSE;
+    }
+    else {
+      return TRUE;
+    }

Can be rewritten to:

    if($this->tableExists($table)) {
      return FALSE;
    }
    return TRUE;

Because of the API addition there needs to be a change record.

adriancid’s picture

adriancid
Primary language Spanish
Location Montreal, Canada
CreditAttribution: adriancid as a volunteer and commented
Status: Needs work » Needs review
FileSize
return_the_returned_query_value_from_schema_droptable-2862344-4.patch2.35 KB
adriancid’s picture

adriancid
Primary language Spanish
Location Montreal, Canada
CreditAttribution: adriancid as a volunteer and commented
Issue summary: View changes
daffie’s picture

daffie CreditAttribution: daffie commented
FileSize
2862344-5-tests-only.patch2.72 KB
2862344-5.patch4.9 KB

I removed some spaces and tabs that where out of place.
I also moved some blank lines for better readability.
Made no code changes to the patch from comment #4.
I have added testing.
The part of the patch without the tests is for me RTBC.

There is still a change record necessary. See https://www.drupal.org/list-changes/drupal.

@adriancid: If you think that my tests are good enough you can put the status to RTBC. Do you like to make the change record?

The last submitted patch, 6: 2862344-5-tests-only.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 6: 2862344-5.patch, failed testing.

daffie’s picture

daffie CreditAttribution: daffie commented
Status: Needs work » Needs review
Issue tags: -Needs change record

Added the change record.

The last submitted patch, 6: 2862344-5-tests-only.patch, failed testing.

The last submitted patch, 6: 2862344-5-tests-only.patch, failed testing.

adriancid’s picture

adriancid
Primary language Spanish
Location Montreal, Canada
CreditAttribution: adriancid as a volunteer and commented

@daffie I just read the change record and I see that you says that is for Drupal 8.4 but here the issue says 8.3, this will be committed to the 8.3 or 8.4 branch?

https://www.drupal.org/node/2862512

adriancid’s picture

adriancid
Primary language Spanish
Location Montreal, Canada
CreditAttribution: adriancid as a volunteer and commented
Status: Needs review » Reviewed & tested by the community
alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott as a volunteer and at Chapter Three commented
Status: Reviewed & tested by the community » Needs review

If the query fails why wouldn't an exception be thrown?

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott as a volunteer and at Chapter Three commented
Status: Needs review » Postponed (maintainer needs more info)

Discussed with @catch - both of us are in agreement the use-case and how this is actually useful needs to be demonstrated. Basically how are we ever going to trigger the return FALSE? All I can see is that we're going to do an extra query for no reason.

adriancid’s picture

adriancid
Primary language Spanish
Location Montreal, Canada
CreditAttribution: adriancid as a volunteer and commented

@alexpott we can do something like this to return TRUE or FALSE:

  public function dropTable($table) {
   ...
    try {
      $this->connection->query('DROP TABLE {' . $table . '}');
      return TRUE;
    }
    catch (\Exception $e) {
      return FALSE;
    }
  }

or like this to throw the exception:

    try {
      $this->connection->query('DROP TABLE {' . $table . '}');
      return TRUE;
    }
    catch (\Exception $e) {
      throw new SchemaException('Error ...');
    }
  }

I don't know what do you think

Version: 8.3.x-dev » 8.4.x-dev

Drupal 8.3.6 was released on August 2, 2017 and is the final full bugfix release for the Drupal 8.3.x series. Drupal 8.3.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.4.0 on October 4, 2017. (Drupal 8.4.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.4.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.5.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.4.x-dev » 8.5.x-dev

Drupal 8.4.4 was released on January 3, 2018 and is the final full bugfix release for the Drupal 8.4.x series. Drupal 8.4.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.5.0 on March 7, 2018. (Drupal 8.5.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.5.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.6.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

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

Drupal 8.5.6 was released on August 1, 2018 and is the final bugfix release for the Drupal 8.5.x series. Drupal 8.5.x will not receive any further development aside from security fixes. Sites should prepare to update to 8.6.0 on September 5, 2018. (Drupal 8.6.0-rc1 is available for testing.)

Bug reports should be targeted against the 8.6.x-dev branch from now on, and new development or disruptive changes should 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.

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

Drupal 8.6.x will not receive any further development aside from security fixes. Bug reports should be targeted against the 8.8.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.9.x-dev branch. For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

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

Drupal 8.8.7 was released on June 3, 2020 and is the final full bugfix release for the Drupal 8.8.x series. Drupal 8.8.x will not receive any further development aside from security fixes. Sites should prepare to update to Drupal 8.9.0 or Drupal 9.0.0 for ongoing support.

Bug reports should be targeted against the 8.9.x-dev branch from now on, and new development or disruptive changes should be targeted against the 9.1.x-dev branch. For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

pameeela’s picture

pameeela CreditAttribution: pameeela commented

@alexpott can you weigh in on #16?

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

Drupal 8 is end-of-life as of November 17, 2021. There will not be further changes made to Drupal 8. Bugfixes are now made to the 9.3.x and higher branches only. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

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

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

Drupal 9.3.15 was released on June 1st, 2022 and is the final full bugfix release for the Drupal 9.3.x series. Drupal 9.3.x will not receive any further development aside from security fixes. Drupal 9 bug reports should be targeted for the 9.4.x-dev branch from now on, and new development or disruptive changes should be targeted for the 9.5.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

smustgrave’s picture

smustgrave CreditAttribution: smustgrave at Mobomo commented

@alexpott good to close?

smustgrave’s picture

smustgrave CreditAttribution: smustgrave at Mobomo commented
Status: Postponed (maintainer needs more info) » Closed (outdated)

closing as outdated.

If still an issue please reopen.