Doctrine DBAL
  1. Doctrine DBAL
  2. DBAL-813

Original PDOException is dropped from previous property in wrapper

    Details

    • Type: Bug Bug
    • Status: Resolved
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 2.5
    • Component/s: Drivers
    • Security Level: All
    • Labels:
      None

      Description

      https://github.com/doctrine/dbal/commit/262deeb122d4de1ad893c8a8c944b1c1926317cd#diff-276fe718ac0aa7fc20162d42ae4dc7b0R52

      Is there any case when PDOException has it's own previous? I don't know of any. Even if it had one, it doesn't make sense because you're skipping one important!! piece in chain of exceptions that cause each other

      Do you agree that the ->getPrevious() is wrong and should be removed? Should I send a pullrequest?

        Issue Links

          Activity

          Hide
          Steve Müller added a comment -

          After having added the missing information from the wrapped \PDOException to Doctrine\DBAL\Driver\PDOException in commit:
          https://github.com/doctrine/dbal/blob/b362492900bc59800441e0d9922736fc55bf8c41/lib/Doctrine/DBAL/Driver/PDOException.php#L54-L55
          all information from \PDOException should be retrievable from the Doctrine\DBAL\Driver\PDOException wrapper now. Therefore there should be no change in behaviour in existing applications.
          Filip Procházka Are you okay with the solution? Can this ticket be closed?

          Show
          Steve Müller added a comment - After having added the missing information from the wrapped \PDOException to Doctrine\DBAL\Driver\PDOException in commit: https://github.com/doctrine/dbal/blob/b362492900bc59800441e0d9922736fc55bf8c41/lib/Doctrine/DBAL/Driver/PDOException.php#L54-L55 all information from \PDOException should be retrievable from the Doctrine\DBAL\Driver\PDOException wrapper now. Therefore there should be no change in behaviour in existing applications. Filip Procházka Are you okay with the solution? Can this ticket be closed?
          Hide
          Filip Procházka added a comment - - edited

          I disagree, first of all you're dropping a stack trace of that exception and replacing it with other from higher level, it may not be all that important, but all best practises say that you should always pass the exception to `$previous`.

          There is zero disadvantages when keeping the exception in the chain. The few kilobytes of memory saved means nothing in comparision to backwards compatibility (which was broken without any real benefit).

          Show
          Filip Procházka added a comment - - edited I disagree, first of all you're dropping a stack trace of that exception and replacing it with other from higher level, it may not be all that important, but all best practises say that you should always pass the exception to `$previous`. There is zero disadvantages when keeping the exception in the chain. The few kilobytes of memory saved means nothing in comparision to backwards compatibility (which was broken without any real benefit).
          Hide
          Marco Pivetta added a comment -

          I agree that the original PDOException should be passed in as $previous, since PDOExceptions themselves may or may not have a non-null getPrevious() result.

          Filip Procházka can you provide a pull request for that?

          Show
          Marco Pivetta added a comment - I agree that the original PDOException should be passed in as $previous , since PDOExceptions themselves may or may not have a non-null getPrevious() result. Filip Procházka can you provide a pull request for that?
          Hide
          Filip Procházka added a comment -

          Yes I can! There you go https://github.com/doctrine/dbal/pull/534 let me know if it needs any adjustments.

          Show
          Filip Procházka added a comment - Yes I can! There you go https://github.com/doctrine/dbal/pull/534 let me know if it needs any adjustments.
          Hide
          Doctrine Bot added a comment -

          A related Github Pull-Request [GH-534] was closed:
          https://github.com/doctrine/dbal/pull/534

          Show
          Doctrine Bot added a comment - A related Github Pull-Request [GH-534] was closed: https://github.com/doctrine/dbal/pull/534
          Show
          Marco Pivetta added a comment - merged: https://github.com/doctrine/dbal/commit/646edacd87d5b9d201bf19aaf55ee0e4d5e470c2
          Hide
          Filip Procházka added a comment -

          Thank you very much!

          Show
          Filip Procházka added a comment - Thank you very much!

            People

            • Assignee:
              Steve Müller
              Reporter:
              Filip Procházka
            • Votes:
              1 Vote for this issue
              Watchers:
              4 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved: