Moodle
  1. Moodle
  2. MDL-6786

Improve dropping of block and module tables during uninstall

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Blocker Blocker
    • Resolution: Fixed
    • Affects Version/s: 1.9
    • Fix Version/s: 1.9, 2.0
    • Component/s: Administration, Blocks
    • Labels:
      None
    • Affected Branches:
      MOODLE_19_STABLE
    • Fixed Branches:
      MOODLE_19_STABLE, MOODLE_20_STABLE
    • Rank:
      28063

      Description

      Currently, in the code in charge of block deletion, it's allowed to have TWO types of table names so, for the block XYZ is possible to have both:

      1) mdl_XYZ... tables and
      2) mdl_block_XYZ... tables

      Following http://docs.moodle.org/en/Development:Coding#Database_structures rules, only the 2nd alternative should be possible (to avoid potential block-module name conflicts and so on). But this could break compatibility with some 3rd part blocks that are using the 1st alternative (attendance, shared_files, timetracker...).

      So, for 1.7 we'll allow both naming schemas to coexist but for 2.0 only the 2nd will survive.

      1. safe_table_deletes.patch
        14 kB
        Petr Škoda
      2. safe_table_deletes2.patch
        14 kB
        Petr Škoda
      3. safe_table_deletes3.patch
        14 kB
        Petr Škoda

        Issue Links

          Activity

          Hide
          Eloy Lafuente (stronk7) added a comment -

          For consistency with the rule, all the new upgrade functions (since 1.7) in blocks must be called:

          xmldb_block_BLOCKNAME_upgrade()

          Show
          Eloy Lafuente (stronk7) added a comment - For consistency with the rule, all the new upgrade functions (since 1.7) in blocks must be called: xmldb_block_BLOCKNAME_upgrade()
          Hide
          Dan Poltawski added a comment -

          Indeed I wholeheartly agree with this, i've just come across a problem with the current behaviour and it seems very dangerous..

          A block named 'time', which was dataless. Removing the block caused the deletion of the timezone table.

          I wonder if we want to be more carful about this.. imagine the scenario with a block named 'cf'...

          Show
          Dan Poltawski added a comment - Indeed I wholeheartly agree with this, i've just come across a problem with the current behaviour and it seems very dangerous.. A block named 'time', which was dataless. Removing the block caused the deletion of the timezone table. I wonder if we want to be more carful about this.. imagine the scenario with a block named 'cf'...
          Hide
          Dan Poltawski added a comment -

          Err, a block named co, would be a better example

          Show
          Dan Poltawski added a comment - Err, a block named co, would be a better example
          Hide
          Dmitry Pupinin added a comment -

          Wait for 2.0 isn't good idea because heretofore blocks can drop module's tables!
          I suggest delete tables in two step: first try to search and delete tables mdl_block_XYZ... If even once it success then don't delete mdl_XYZ... otherwise search and delete mdl_XYZ...
          So I could create empty table for example mdl_block_attendance and restrict deleting by block of tables mdl_attendance_* belonging to module!

          Show
          Dmitry Pupinin added a comment - Wait for 2.0 isn't good idea because heretofore blocks can drop module's tables! I suggest delete tables in two step: first try to search and delete tables mdl_block_XYZ... If even once it success then don't delete mdl_XYZ... otherwise search and delete mdl_XYZ... So I could create empty table for example mdl_block_attendance and restrict deleting by block of tables mdl_attendance_* belonging to module!
          Hide
          Martin Dougiamas added a comment -

          Yes +1 for 2.0

          Show
          Martin Dougiamas added a comment - Yes +1 for 2.0
          Hide
          Petr Škoda added a comment -

          maybe we could use the info from all intall.xml files in known locations and drop only tables that are not present there

          Show
          Petr Škoda added a comment - maybe we could use the info from all intall.xml files in known locations and drop only tables that are not present there
          Hide
          Petr Škoda added a comment -

          this patch could solve this problem for both blocks and modules, please review it

          Show
          Petr Škoda added a comment - this patch could solve this problem for both blocks and modules, please review it
          Hide
          Gary Anderson added a comment -

          I learned this lesson the hard way a year ago when I created and then removed in the middle of the school day a block that I called assignment_submissions

          +1

          Show
          Gary Anderson added a comment - I learned this lesson the hard way a year ago when I created and then removed in the middle of the school day a block that I called assignment_submissions +1
          Hide
          Petr Škoda added a comment -

          sending updated patch
          Eloy, do you think this would be too risky to push into 1.9 ?

          Show
          Petr Škoda added a comment - sending updated patch Eloy, do you think this would be too risky to push into 1.9 ?
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Well,

          your patch seems to be really clever, if I don't remember wrong it does this:

          1) Attempt to delete tables by looking to the corresponding module/block install.xml file
          2) If no install.xml file exists, delete like now (by name) but excluding ALL the tables defined in all the rest of install.xml files.

          I really think it's a good way to fix this problem once and for ever. B-) I have nothing against it under 1.9 at all (after a bit of real testing). IMO if working (and I think it works) will make things safer.

          So +1 for it, although I'd recommend MD to perform final vote. If so, I can test / apply it while you work in other, more critical, tasks.

          Ciao

          Show
          Eloy Lafuente (stronk7) added a comment - Well, your patch seems to be really clever, if I don't remember wrong it does this: 1) Attempt to delete tables by looking to the corresponding module/block install.xml file 2) If no install.xml file exists, delete like now (by name) but excluding ALL the tables defined in all the rest of install.xml files. I really think it's a good way to fix this problem once and for ever. B-) I have nothing against it under 1.9 at all (after a bit of real testing). IMO if working (and I think it works) will make things safer. So +1 for it, although I'd recommend MD to perform final vote. If so, I can test / apply it while you work in other, more critical, tasks. Ciao
          Hide
          Martin Dougiamas added a comment -

          Sounds perfect and the patch looks good to me at a quick skim (but I've not tested it). I think this can go into Moodle 1.9

          +1

          Show
          Martin Dougiamas added a comment - Sounds perfect and the patch looks good to me at a quick skim (but I've not tested it). I think this can go into Moodle 1.9 +1
          Hide
          Petr Škoda added a comment -

          oki, thanks - I will retest it today and commit :-D

          Show
          Petr Škoda added a comment - oki, thanks - I will retest it today and commit :-D
          Hide
          Dan Poltawski added a comment -

          I have empty $CFG->prefix and get:
          Warning: strpos() [function.strpos]: Empty delimiter. in /var/www/moodle/lib/ddllib.php on line 703

          Show
          Dan Poltawski added a comment - I have empty $CFG->prefix and get: Warning: strpos() [function.strpos] : Empty delimiter. in /var/www/moodle/lib/ddllib.php on line 703
          Hide
          Dan Poltawski added a comment -

          Also missing in admin/modules.php:
          require_once($CFG->libdir.'/ddllib.php');

          Show
          Dan Poltawski added a comment - Also missing in admin/modules.php: require_once($CFG->libdir.'/ddllib.php');
          Hide
          Petr Škoda added a comment -

          thanks Dan! sending updated patch

          Show
          Petr Škoda added a comment - thanks Dan! sending updated patch
          Hide
          Dan Poltawski added a comment -

          Looks good now - this patch will help me sleep easier at night!

          Show
          Dan Poltawski added a comment - Looks good now - this patch will help me sleep easier at night!
          Hide
          Petr Škoda added a comment -

          committed into cvs, thanks!

          Show
          Petr Škoda added a comment - committed into cvs, thanks!
          Hide
          Anthony Borrow added a comment -

          Petr - +1 - I think that 3rd party contributions should be expected to hang with current practices especially when it comes to naming of tables. I plan on providing 3rd party contributions with branches for each major Moodle release so code could be fixed. I would rather see 3rd party contributions patched than to hold up Moodle development. As we continue to move forward I think it is important that we look at contributed code for new, fresh ideas but also as a training ground for developing new developers. The more contributed code is encouraged to conform with the standards used in core the better. Peace - Anthony

          Show
          Anthony Borrow added a comment - Petr - +1 - I think that 3rd party contributions should be expected to hang with current practices especially when it comes to naming of tables. I plan on providing 3rd party contributions with branches for each major Moodle release so code could be fixed. I would rather see 3rd party contributions patched than to hold up Moodle development. As we continue to move forward I think it is important that we look at contributed code for new, fresh ideas but also as a training ground for developing new developers. The more contributed code is encouraged to conform with the standards used in core the better. Peace - Anthony
          Hide
          Wen Hao Chuang added a comment -

          +1 here from SFSU too.

          Show
          Wen Hao Chuang added a comment - +1 here from SFSU too.
          Hide
          Leandro Peralta added a comment -

          Well, about the naming convention, I'm working on a block module with several tables, and I have a problem with the XMLDB editor. Considering I have to name my tables as 'block_blockname_table_name', I have not many chartacters left for the names of my tables. I manually modify install.xml and it works perfectly, but I then cannot open it with the XMLDB editor. Is there any reason for setting the table name limit to 28?

          Show
          Leandro Peralta added a comment - Well, about the naming convention, I'm working on a block module with several tables, and I have a problem with the XMLDB editor. Considering I have to name my tables as 'block_blockname_table_name', I have not many chartacters left for the names of my tables. I manually modify install.xml and it works perfectly, but I then cannot open it with the XMLDB editor. Is there any reason for setting the table name limit to 28?
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Absolutely yes!

          In order to support all 4 official DBs (MySQL, PostgreSQL, MSSQL and Oracle), the XMLDB editor enforces that rule, because Oracle has a hard limit of 30cc.

          30 - 2 chars for prefixes = the 28cc limit to be cross-db.

          Ciao

          Show
          Eloy Lafuente (stronk7) added a comment - Absolutely yes! In order to support all 4 official DBs (MySQL, PostgreSQL, MSSQL and Oracle), the XMLDB editor enforces that rule, because Oracle has a hard limit of 30cc. 30 - 2 chars for prefixes = the 28cc limit to be cross-db. Ciao

            People

            • Votes:
              3 Vote for this issue
              Watchers:
              7 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved: