Moodle

Improve dropping of block and module tables during uninstall

Details

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

Attachments

  1. Text File
    safe_table_deletes.patch
    17/Nov/07 11:13 PM
    14 kB
    Petr Škoda (skodak)
  2. Text File
    safe_table_deletes2.patch
    03/Jan/08 7:21 AM
    14 kB
    Petr Škoda (skodak)
  3. Text File
    safe_table_deletes3.patch
    03/Jan/08 7:25 PM
    14 kB
    Petr Škoda (skodak)

Issue Links

This issue will help resolve:
MDL-4791 Potential Problem? Trivial Closed

Activity

Ascending order - Click to sort in descending order
Hide
Permalink
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
Permalink
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
Permalink
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
Permalink
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
Permalink
Martin Dougiamas added a comment -

Yes +1 for 2.0

Show
Martin Dougiamas added a comment - Yes +1 for 2.0
Hide
Permalink
Petr Škoda (skodak) 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 (skodak) 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
Permalink
Petr Škoda (skodak) added a comment -

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

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

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

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

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

Show
Petr Škoda (skodak) added a comment - oki, thanks - I will retest it today and commit :-D
Hide
Permalink
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
Permalink
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
Permalink
Petr Škoda (skodak) added a comment -

thanks Dan! sending updated patch

Show
Petr Škoda (skodak) added a comment - thanks Dan! sending updated patch
Hide
Permalink
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
Permalink
Petr Škoda (skodak) added a comment -

committed into cvs, thanks!

Show
Petr Škoda (skodak) added a comment - committed into cvs, thanks!
Hide
Permalink
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
Permalink
Wen Hao Chuang added a comment -

+1 here from SFSU too.

Show
Wen Hao Chuang added a comment - +1 here from SFSU too.
Hide
Permalink
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
Permalink
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

Dates

  • Created:
    Updated:
    Resolved:
Atlassian JIRA (v4.4.3#663-r165197) | Report a problem
Powered by a free Atlassian JIRA open source license for Moodle. Try JIRA - bug tracking software for your team.