Details

      Description

      The db_tools class has spawned a copy at install/database_update.php, which is not exactly ideal. To make matters worse the updater version has fixes and a different interface to the original version, they should both be carefully combined into the original at includes/db/db_tools.php.

      The build script should then be updated to create a copy of db_tools inside install/database_update.php when packaging.

      This really needs resolving for 3.1 and possibly in 3.0.x.

        Issue Links

          Activity

          Hide
          Oleg Oleg [X] (Inactive) added a comment - - edited

          These are the commits altering the copy in database update. Some of them may already have been applied to db tools.

          d843dbd046fa427d00d30ef06558043e46c6fc98 - applied to both
          e69fe56634225d771d4d47c2151d9828b1be2b5d - applied to both
          d7d96223e7bae7cd60b13c6e7896d95838c3633c - ported
          55e15b4c8eea94f226a0169284bb51d7d9bcbcd4 - applied to both for everything that matters
          96a30afcca3ebd832c9b3083bb5c9a9f2a2dc54b - ported
          54c22ae52a0e18232cac8fed342ea52f2e2a793d - ported
          023760c8b2402418310a3717db8349cac0342e42 - ported, hopefully correctly
          5553cfc2ed81ba9eb571804c431def962720b39e - ported
          870921c87231c1e637de76295914be53b846469a - applied to both but with different whitespace
          1802b9ff9286a7fc24493e71b3432816cbdbfcd8 - ported
          bb191a1d696994c1d27bbd0457457cf538dff7ee - merged in e7b86871
          396af3853fc2d86b255db0f71e56a9f880ee2509 - irrelevant, whitespace only
          af4c2a3eb15fc4318b23dcb7794c230cf3ec2a0f - whitespace only
          

          Show
          Oleg Oleg [X] (Inactive) added a comment - - edited These are the commits altering the copy in database update. Some of them may already have been applied to db tools. d843dbd046fa427d00d30ef06558043e46c6fc98 - applied to both e69fe56634225d771d4d47c2151d9828b1be2b5d - applied to both d7d96223e7bae7cd60b13c6e7896d95838c3633c - ported 55e15b4c8eea94f226a0169284bb51d7d9bcbcd4 - applied to both for everything that matters 96a30afcca3ebd832c9b3083bb5c9a9f2a2dc54b - ported 54c22ae52a0e18232cac8fed342ea52f2e2a793d - ported 023760c8b2402418310a3717db8349cac0342e42 - ported, hopefully correctly 5553cfc2ed81ba9eb571804c431def962720b39e - ported 870921c87231c1e637de76295914be53b846469a - applied to both but with different whitespace 1802b9ff9286a7fc24493e71b3432816cbdbfcd8 - ported bb191a1d696994c1d27bbd0457457cf538dff7ee - merged in e7b86871 396af3853fc2d86b255db0f71e56a9f880ee2509 - irrelevant, whitespace only af4c2a3eb15fc4318b23dcb7794c230cf3ec2a0f - whitespace only
          Hide
          Oleg Oleg [X] (Inactive) added a comment - - edited

          Some git + shell + perl magic:

          for i in `git log --format=oneline 4a36fe5264ed2a2b2073ec1bd3c67f6b0d208cc2...develop-olympus --  phpBB/install/database_update.php  |awk '{print $1}' |perl -e 'undef $/; $x=<>; @x=split(/\s/, $x); print join "\n", reverse @x; print "\n";' `; do clear; echo $i; s $i ; echo 'press key' $i; read $x; done
           
          s () {
          	git show $1 -- phpBB/install/database_update.php
          }
          p () {
          	patch phpBB/includes/db/db_tools.php
          }
          
          

          Show
          Oleg Oleg [X] (Inactive) added a comment - - edited Some git + shell + perl magic: for i in `git log --format=oneline 4a36fe5264ed2a2b2073ec1bd3c67f6b0d208cc2...develop-olympus -- phpBB/install/database_update.php |awk '{print $1}' |perl -e 'undef $/; $x=<>; @x=split(/\s/, $x); print join "\n", reverse @x; print "\n";' `; do clear; echo $i; s $i ; echo 'press key' $i; read $x; done   s () { git show $1 -- phpBB/install/database_update.php } p () { patch phpBB/includes/db/db_tools.php }
          Hide
          Oleg Oleg [X] (Inactive) added a comment - - edited

          https://github.com/phpbb/phpbb3/commit/96a30afcca3ebd832c9b3083bb5c9a9f2a2dc54b is somewhat questionable. If it was done in the updater, and the updater is to use db_tools, then clearly it needs to be in db_tools, but maybe make it conditional.

          https://github.com/phpbb/phpbb3/commit/5553cfc2ed81ba9eb571804c431def962720b39e#L1L2530 (that line) looks like it was meant from the commit message but a comment surely would have been appreciated.

          https://github.com/phpbb/phpbb3/commit/bb191a1d696994c1d27bbd0457457cf538dff7ee adds a size option to add_index but not to add_unique_index. Technically the documentation only allows size on add_index thus it's consistent with the code. But there seems to be no good reason why this was not implemented for unique indexes. In any event I will classify handling of unique indexes as an enhancement and not do it now.

          Some of these commits I just don't understand. They change both db_tools and database_update but make one change in db_tools for every five changes in database_update. What gives?

          Is there a reason why we don't want this in 3.0?

          Show
          Oleg Oleg [X] (Inactive) added a comment - - edited https://github.com/phpbb/phpbb3/commit/96a30afcca3ebd832c9b3083bb5c9a9f2a2dc54b is somewhat questionable. If it was done in the updater, and the updater is to use db_tools, then clearly it needs to be in db_tools, but maybe make it conditional. https://github.com/phpbb/phpbb3/commit/5553cfc2ed81ba9eb571804c431def962720b39e#L1L2530 (that line) looks like it was meant from the commit message but a comment surely would have been appreciated. https://github.com/phpbb/phpbb3/commit/bb191a1d696994c1d27bbd0457457cf538dff7ee adds a size option to add_index but not to add_unique_index. Technically the documentation only allows size on add_index thus it's consistent with the code. But there seems to be no good reason why this was not implemented for unique indexes. In any event I will classify handling of unique indexes as an enhancement and not do it now. Some of these commits I just don't understand. They change both db_tools and database_update but make one change in db_tools for every five changes in database_update. What gives? Is there a reason why we don't want this in 3.0?
          Hide
          Oleg Oleg [X] (Inactive) added a comment -

          The resolution of proliferation: https://github.com/phpbb/phpbb3/pull/175

          This is only a part of this ticket, the other part is build system changes as stated in description to copy the db_tools into the updater. Thus after the pull request is merged this ticket should remain open.

          Show
          Oleg Oleg [X] (Inactive) added a comment - The resolution of proliferation: https://github.com/phpbb/phpbb3/pull/175 This is only a part of this ticket, the other part is build system changes as stated in description to copy the db_tools into the updater. Thus after the pull request is merged this ticket should remain open.
          Hide
          naderman Nils Adermann added a comment -

          Merged pull request.

          Show
          naderman Nils Adermann added a comment - Merged pull request.
          Hide
          Oleg Oleg [X] (Inactive) added a comment - - edited

          The build script should then be updated to create a copy of db_tools inside install/database_update.php when packaging.

          Can db_tools be in a separate file under install subdir or does it have to physically be part of database_update.php?

          Documentation for package building: http://wiki.phpbb.com/Package_Building

          Show
          Oleg Oleg [X] (Inactive) added a comment - - edited The build script should then be updated to create a copy of db_tools inside install/database_update.php when packaging. Can db_tools be in a separate file under install subdir or does it have to physically be part of database_update.php? Documentation for package building: http://wiki.phpbb.com/Package_Building
          Hide
          naderman Nils Adermann added a comment - - edited

          It can be a separate file or even should be, but the packing code needs to be checked to make sure it includes it (although it should automatically).

          Show
          naderman Nils Adermann added a comment - - edited It can be a separate file or even should be, but the packing code needs to be checked to make sure it includes it (although it should automatically).
          Hide
          naderman Nils Adermann added a comment -

          Unfortunately this is not a solution. The database_update.php file (without an r ) is used by the modified files only and patch files packages too. Using either one of those update mechanisms, you do not have the changed db_tools.php file available since we recommend updating the database before the files (as we do in the automated updater). So an additional change to the build script to include the changed db_tools.php file in the package for those updates too, would be necessary.

          Show
          naderman Nils Adermann added a comment - Unfortunately this is not a solution. The database_update.php file (without an r ) is used by the modified files only and patch files packages too. Using either one of those update mechanisms, you do not have the changed db_tools.php file available since we recommend updating the database before the files (as we do in the automated updater). So an additional change to the build script to include the changed db_tools.php file in the package for those updates too, would be necessary.
          Hide
          naderman Nils Adermann added a comment -

          I was mistaken, we do recommend to run the database_update.php file after modifying the files, so we can go ahead and merge this.

          Show
          naderman Nils Adermann added a comment - I was mistaken, we do recommend to run the database_update.php file after modifying the files, so we can go ahead and merge this.

            People

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

              Dates

              • Created:
                Updated:
                Resolved:

                Development