Uploaded image for project: 'Moodle'
  1. Moodle
  2. MDL-6786

Improve dropping of block and module tables during uninstall

    Details

    • Type: Bug
    • Status: Closed
    • Priority: 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

          Attachments

            Issue Links

              Activity

              Hide
              stronk7 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
              stronk7 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
              poltawski 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
              poltawski 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
              poltawski Dan Poltawski added a comment -

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

              Show
              poltawski Dan Poltawski added a comment - Err, a block named co, would be a better example
              Hide
              dlnsk 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
              dlnsk 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
              dougiamas Martin Dougiamas added a comment -

              Yes +1 for 2.0

              Show
              dougiamas Martin Dougiamas added a comment - Yes +1 for 2.0
              Hide
              skodak 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
              skodak 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
              skodak Petr Skoda added a comment -

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

              Show
              skodak Petr Skoda added a comment - this patch could solve this problem for both blocks and modules, please review it
              Hide
              ganderson 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
              ganderson 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
              skodak Petr Skoda added a comment -

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

              Show
              skodak Petr Skoda added a comment - sending updated patch Eloy, do you think this would be too risky to push into 1.9 ?
              Hide
              stronk7 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
              stronk7 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
              dougiamas 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
              dougiamas 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
              skodak Petr Skoda added a comment -

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

              Show
              skodak Petr Skoda added a comment - oki, thanks - I will retest it today and commit :-D
              Hide
              poltawski 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
              poltawski 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
              poltawski Dan Poltawski added a comment -

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

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

              thanks Dan! sending updated patch

              Show
              skodak Petr Skoda added a comment - thanks Dan! sending updated patch
              Hide
              poltawski Dan Poltawski added a comment -

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

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

              committed into cvs, thanks!

              Show
              skodak Petr Skoda added a comment - committed into cvs, thanks!
              Hide
              aborrow 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
              aborrow 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
              chuang Wen Hao Chuang added a comment -

              +1 here from SFSU too.

              Show
              chuang Wen Hao Chuang added a comment - +1 here from SFSU too.
              Hide
              soflete 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
              soflete 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
              stronk7 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
              stronk7 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:
                    Fix Release Date:
                    3/Mar/08