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

      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.

        Gliffy Diagrams

        1. safe_table_deletes.patch
          14 kB
          Petr Skoda
        2. safe_table_deletes2.patch
          14 kB
          Petr Skoda
        3. safe_table_deletes3.patch
          14 kB
          Petr Skoda

          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 Skoda 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 Skoda 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 Skoda added a comment -

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

            Show
            Petr Skoda 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 Skoda added a comment -

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

            Show
            Petr Skoda 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 Skoda added a comment -

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

            Show
            Petr Skoda 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 Skoda added a comment -

            thanks Dan! sending updated patch

            Show
            Petr Skoda 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 Skoda added a comment -

            committed into cvs, thanks!

            Show
            Petr Skoda 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:
                8 Start watching this issue

                Dates

                • Created:
                  Updated:
                  Resolved: