Uploaded image for project: 'phpBB3'
  1. phpBB3
  2. PHPBB3-11162

Get rid of $db->sql_return_on_error(true) trickery when splitting/merging topics

    Details

    • Type: Improvement
    • Status: Unverified Fix
    • Priority: Minor
    • Resolution: Fixed
    • Affects Version/s: 3.0.11
    • Fix Version/s: 3.0.12-RC1
    • Component/s: None
    • Labels:
      None

      Description

      Background: https://github.com/phpbb/phpbb3/pull/988

      Instead of potentially doing the update that may fail, add conditions to only update rows that can be updated. The subsequent delete will take care of any left over rows.

      My expectation is that the current code does not update any rows if any of them fail on postgres and updates all rows other than failing ones on mysql.

      3.0 and 3.1 will need separate patches because of https://github.com/phpbb/phpbb3/pull/988.

        Activity

        Hide
        bantu Andreas Fischer added a comment -

        If there exists a database we support where this trick does not work, this should be fixed in 3.0.x. However, I'm not sure how a fix would look like.

        Show
        bantu Andreas Fischer added a comment - If there exists a database we support where this trick does not work, this should be fixed in 3.0.x. However, I'm not sure how a fix would look like.
        Hide
        Oleg Oleg [X] (Inactive) added a comment -

        I incorrectly estimated the fix. A working fix would involve additional select to determine what should not be updated.

        This gets better however. Witness https://github.com/phpbb/phpbb3/pull/1030.

        Configuration read from /home/phpdev/test-phpbb/phpunit.xml.dist
         
        ..FF
         
        Time: 5 seconds, Memory: 6.00Mb
         
        There were 2 failures:
         
        1) phpbb_update_rows_avoiding_duplicates_test::test_trivial_update with data set #2 ('conflict', array(4), 5, 1)
        Failed asserting that '2' matches expected 1.
         
        /home/phpdev/test-phpbb/tests/functions/update_rows_avoiding_duplicates_test.php:69
        /home/pie/apps/git-phpunit-bundle/phpunit.php:44
         
        2) phpbb_update_rows_avoiding_duplicates_test::test_trivial_update with data set #3 ('conflict and no conflict', array(6), 7, 2)
        Failed asserting that '3' matches expected 2.
         
        /home/phpdev/test-phpbb/tests/functions/update_rows_avoiding_duplicates_test.php:69
        /home/pie/apps/git-phpunit-bundle/phpunit.php:44
         
        FAILURES!
        Tests: 4, Assertions: 4, Failures: 2.
        

        I am not seeing any unique constraints on mysql or postgres, therefore I assume all updates will succeed always.

        Show
        Oleg Oleg [X] (Inactive) added a comment - I incorrectly estimated the fix. A working fix would involve additional select to determine what should not be updated. This gets better however. Witness https://github.com/phpbb/phpbb3/pull/1030 . Configuration read from /home/phpdev/test-phpbb/phpunit.xml.dist   ..FF   Time: 5 seconds, Memory: 6.00Mb   There were 2 failures:   1) phpbb_update_rows_avoiding_duplicates_test::test_trivial_update with data set #2 ('conflict', array(4), 5, 1) Failed asserting that '2' matches expected 1.   /home/phpdev/test-phpbb/tests/functions/update_rows_avoiding_duplicates_test.php:69 /home/pie/apps/git-phpunit-bundle/phpunit.php:44   2) phpbb_update_rows_avoiding_duplicates_test::test_trivial_update with data set #3 ('conflict and no conflict', array(6), 7, 2) Failed asserting that '3' matches expected 2.   /home/phpdev/test-phpbb/tests/functions/update_rows_avoiding_duplicates_test.php:69 /home/pie/apps/git-phpunit-bundle/phpunit.php:44   FAILURES! Tests: 4, Assertions: 4, Failures: 2. I am not seeing any unique constraints on mysql or postgres, therefore I assume all updates will succeed always.
        Hide
        Oleg Oleg [X] (Inactive) added a comment -

        ^ pgsql

        Show
        Oleg Oleg [X] (Inactive) added a comment - ^ pgsql
        Hide
        bantu Andreas Fischer added a comment - - edited
        Show
        bantu Andreas Fischer added a comment - - edited MySQL InnoDB fails too. https://gist.github.com/b6268a27fc67c846d4a4
        Show
        Oleg Oleg [X] (Inactive) added a comment - Proposal: https://gist.github.com/ce153a7c7acb9b77300a

          People

          • Assignee:
            Oleg Oleg [X] (Inactive)
            Reporter:
            Oleg Oleg [X] (Inactive)
          • Votes:
            0 Vote for this issue
            Watchers:
            0 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development