Doctrine DBAL
  1. Doctrine DBAL
  2. DBAL-694

Extend AbstractSchemaManager::dropTable() signature

    Details

    • Type: Improvement Improvement
    • Status: Resolved
    • Priority: Major Major
    • Resolution: Won't Fix
    • Affects Version/s: None
    • Fix Version/s: None
    • Component/s: Schema Managers
    • Security Level: All
    • Labels:
      None

      Description

      For now, dropTable($tableName) doesn't take into account useful IF EXISTS flag.

      I can submit PR with myself if this proposal will be accepted.

        Activity

        Hide
        Steve Müller added a comment -

        Nikolay Konev can you please give some information of what you want to achieve with this addition? There are several problems I see here:

        1. I am quite sure a method signature changes are not possible here because of BC break (affects AbstractSchemaManager::dropTable and AbstractPlatform::getDropTableSQL).
        2. IF EXISTS clauses are not supported on all platforms and not SQL standard AFAIK.

        Possible solutions depending on what you want to do:
        1. If you just want to make sure that the DROP statement does not fail because of non-existence in DB you can use AbstractSchemaManager::tryMethod('dropTable', 'table_name_here') which tries to drop the table but does not throw an exception if it failed. Drawbacks here are that other errors besides non-existence are silenced, too and you cannot evaluate if it really got deleted at database side.
        2. Use AbstractSchemaManager::dropTable() and catch Doctrine\DBAL\DBALException. Check for the error code with DBALException::getCode() and evaluate it to the DBALException::ERROR_UNKNOWN_TABLE constant (currently only available in master branch and available in 2.5 when released - see https://github.com/doctrine/dbal/pull/364).
        3. Use AbstractSchemaManager::tableExists method to check if the table exists before calling AbstractSchemaManager::dropTable

        You see there are lots of possibilities already and I am not quite sure if we should bloat the schema manager API with more alias methods (drop*IfExists, create*IfNotExists) that actually don't do anything special.

        Show
        Steve Müller added a comment - Nikolay Konev can you please give some information of what you want to achieve with this addition? There are several problems I see here: 1. I am quite sure a method signature changes are not possible here because of BC break (affects AbstractSchemaManager::dropTable and AbstractPlatform::getDropTableSQL). 2. IF EXISTS clauses are not supported on all platforms and not SQL standard AFAIK. Possible solutions depending on what you want to do: 1. If you just want to make sure that the DROP statement does not fail because of non-existence in DB you can use AbstractSchemaManager::tryMethod('dropTable', 'table_name_here') which tries to drop the table but does not throw an exception if it failed. Drawbacks here are that other errors besides non-existence are silenced, too and you cannot evaluate if it really got deleted at database side. 2. Use AbstractSchemaManager::dropTable() and catch Doctrine\DBAL\DBALException. Check for the error code with DBALException::getCode() and evaluate it to the DBALException::ERROR_UNKNOWN_TABLE constant (currently only available in master branch and available in 2.5 when released - see https://github.com/doctrine/dbal/pull/364 ). 3. Use AbstractSchemaManager::tableExists method to check if the table exists before calling AbstractSchemaManager::dropTable You see there are lots of possibilities already and I am not quite sure if we should bloat the schema manager API with more alias methods (drop*IfExists, create*IfNotExists) that actually don't do anything special.
        Hide
        Nikolay Konev added a comment -

        Thanks, while i used to wait for answer, i took solution #3 for iExists functionality purposes (test suite setUp & tearDown clean-ups).

        And also thanks for DBALException::ERROR_UNKNOWN_TABLE hint — it solves problem finally, without API changes.

        Show
        Nikolay Konev added a comment - Thanks, while i used to wait for answer, i took solution #3 for iExists functionality purposes (test suite setUp & tearDown clean-ups). And also thanks for DBALException::ERROR_UNKNOWN_TABLE hint — it solves problem finally, without API changes.
        Hide
        Nikolay Konev added a comment -

        Found some workarounds, which worth to be documented

        Show
        Nikolay Konev added a comment - Found some workarounds, which worth to be documented
        Hide
        Steve Müller added a comment -

        Nikolay Konev The DBALException error constants are pretty new and need indeed documentation. I will provide that during the next days. Also these exception error codes are converted into an own exception class on the fly (see: https://github.com/doctrine/dbal/pull/458). Not all DBALException::ERROR_* constants will get converted at this time though. Some are still missing but will be added soon. Then you are able to do something like this:

        // Schema manager.
        
        try {
            $sm->dropTable('mytable');
        } catch (\Doctrine\DBAL\Excpetion\TableNotFoundException $e) { // exception class not yet available
            // do something
        }
        

        Besides that do you think there is still anything to do here? Or are those solutions acceptable for you?

        Show
        Steve Müller added a comment - Nikolay Konev The DBALException error constants are pretty new and need indeed documentation. I will provide that during the next days. Also these exception error codes are converted into an own exception class on the fly (see: https://github.com/doctrine/dbal/pull/458 ). Not all DBALException::ERROR_* constants will get converted at this time though. Some are still missing but will be added soon. Then you are able to do something like this: // Schema manager. try { $sm->dropTable('mytable'); } catch (\Doctrine\DBAL\Excpetion\TableNotFoundException $e) { // exception class not yet available // do something } Besides that do you think there is still anything to do here? Or are those solutions acceptable for you?
        Hide
        Nikolay Konev added a comment -

        Solution with tableExists() check is acceptable in my scenario, although it adds some db overhead in my tests.

        Issue does not have sense now.

        Show
        Nikolay Konev added a comment - Solution with tableExists() check is acceptable in my scenario, although it adds some db overhead in my tests. Issue does not have sense now.

          People

          • Assignee:
            Benjamin Eberlei
            Reporter:
            Nikolay Konev
          • Votes:
            0 Vote for this issue
            Watchers:
            2 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved: