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

Block removal should have automatically remove 'config_plugins' settings

    Details

    • Type: Improvement
    • Status: Closed
    • Priority: Major
    • Resolution: Fixed
    • Affects Version/s: 2.1.2
    • Fix Version/s: 2.2
    • Component/s: Blocks
    • Labels:
    • Database:
      Any
    • Testing Instructions:
      Hide

      Install a block that uses the config_plugins method of storing global configuration.
      Set some global config data.
      Verify the data is in the config_plugins table.
      Uninstall the block.
      Verify that the data is / is not removed from the config_plugins.

      Note - some blocks may have already installed a 'before_delete' function to avoid this problem. Test should be done with a block that does not explicitly remove its own settings.

      Show
      Install a block that uses the config_plugins method of storing global configuration. Set some global config data. Verify the data is in the config_plugins table. Uninstall the block. Verify that the data is / is not removed from the config_plugins. Note - some blocks may have already installed a 'before_delete' function to avoid this problem. Test should be done with a block that does not explicitly remove its own settings.
    • Workaround:
      Hide

      Blocks must be coded with specific 'before_delete' function to remove this data themselves.

      Show
      Blocks must be coded with specific 'before_delete' function to remove this data themselves.
    • Affected Branches:
      MOODLE_21_STABLE
    • Fixed Branches:
      MOODLE_22_STABLE
    • Pull from Repository:
    • Pull Master Branch:
      w46_MDL-30327_m22_blockconfig

      Description

      If a block is using the plugins config method for storing its global configuration values, they are left in the 'config_plugins' table unless code has been specifically provided in the 'before_delete' method to remove them. These should be remove automatically when the block is uninstalled, using the Moodle 'unset_all_config_for_plugin' function. It appears that blocks do not use the same 'uninstall_plugin' function that modules and other plugins do, or this would happen.

        Gliffy Diagrams

          Issue Links

            Activity

            Hide
            mchurch Mike Churchward added a comment -

            I believe the correct fix is to either:

            1. Use 'uninstall_plugin' to remove blocks, or
            2. Add code to '/admin/blocks.php' for the delete action that specifically removes the data via unset_all_config_for_plugins function.
            Show
            mchurch Mike Churchward added a comment - I believe the correct fix is to either: Use 'uninstall_plugin' to remove blocks, or Add code to '/admin/blocks.php' for the delete action that specifically removes the data via unset_all_config_for_plugins function.
            Hide
            salvetore Michael de Raadt added a comment -

            Hi, Mike.

            Thanks for suggesting that. I hope you don't mind that I turned it into an improvement.

            Show
            salvetore Michael de Raadt added a comment - Hi, Mike. Thanks for suggesting that. I hope you don't mind that I turned it into an improvement.
            Hide
            skodak Petr Skoda added a comment -

            Thanks for the report and detailed explanation!

            Show
            skodak Petr Skoda added a comment - Thanks for the report and detailed explanation!
            Hide
            samhemelryk Sam Hemelryk added a comment -

            Thanks Petr - this has been integrated now.
            I did note that there are a couple of things that could be cleaned up in core blocks now (before_delete functions). Is there an issue to do this or shall I create one?

            Cheers
            Sam

            Show
            samhemelryk Sam Hemelryk added a comment - Thanks Petr - this has been integrated now. I did note that there are a couple of things that could be cleaned up in core blocks now (before_delete functions). Is there an issue to do this or shall I create one? Cheers Sam
            Hide
            skodak Petr Skoda added a comment -

            please create a new issue, thanks

            Show
            skodak Petr Skoda added a comment - please create a new issue, thanks
            Hide
            stronk7 Eloy Lafuente (stronk7) added a comment -

            (getting on this after confirmation with David)

            Show
            stronk7 Eloy Lafuente (stronk7) added a comment - (getting on this after confirmation with David)
            Hide
            samhemelryk Sam Hemelryk added a comment -

            Created MDL-30387 to clean up core blocks

            Show
            samhemelryk Sam Hemelryk added a comment - Created MDL-30387 to clean up core blocks
            Hide
            stronk7 Eloy Lafuente (stronk7) added a comment -

            Oki, thanks for making me this to take ages, grrrr.

            It seems that we have NOT any block in core using config_plugins, but the section_links one, that is storing settings there using block/section_links instead of proper component name.

            So I'm going to test this by adding some records there by hand (using proper component name) and then deleting the block plugin.

            We should move all blocks to use proper cfg asap, IMO. 2.3 could be a good target. Fell free to create the issue.

            Ciao

            Show
            stronk7 Eloy Lafuente (stronk7) added a comment - Oki, thanks for making me this to take ages, grrrr. It seems that we have NOT any block in core using config_plugins, but the section_links one, that is storing settings there using block/section_links instead of proper component name. So I'm going to test this by adding some records there by hand (using proper component name) and then deleting the block plugin. We should move all blocks to use proper cfg asap, IMO. 2.3 could be a good target. Fell free to create the issue. Ciao
            Hide
            stronk7 Eloy Lafuente (stronk7) added a comment -

            Sweet, to I tried by adding by hand some records to the section_links block (using proper component names), and when uninstalling the block I got one horrible DML error (MDL-30392). Going to re-test this after fixing the other.

            Show
            stronk7 Eloy Lafuente (stronk7) added a comment - Sweet, to I tried by adding by hand some records to the section_links block (using proper component names), and when uninstalling the block I got one horrible DML error ( MDL-30392 ). Going to re-test this after fixing the other.
            Hide
            stronk7 Eloy Lafuente (stronk7) added a comment -

            Oki, I've added some records by hand using component name: block_section_links (apart from the 4 records present with incorrect blocks/section_links.

            And after deleting the block from the manage blocks page, all those records are deleted, the ones using component names by the new code and the incorrect ones by the before_delete() method.

            So passing this, thanks!

            Show
            stronk7 Eloy Lafuente (stronk7) added a comment - Oki, I've added some records by hand using component name: block_section_links (apart from the 4 records present with incorrect blocks/section_links. And after deleting the block from the manage blocks page, all those records are deleted, the ones using component names by the new code and the incorrect ones by the before_delete() method. So passing this, thanks!
            Hide
            stronk7 Eloy Lafuente (stronk7) added a comment -

            Closing as fixed, many thanks for your effort!

            Note that the changes related to master (2.2beta) have been already sent upstream. But the stable ones will be part of next weeklies (Wed/Thu) as usual.

            Ciao

            Show
            stronk7 Eloy Lafuente (stronk7) added a comment - Closing as fixed, many thanks for your effort! Note that the changes related to master (2.2beta) have been already sent upstream. But the stable ones will be part of next weeklies (Wed/Thu) as usual. Ciao

              People

              • Votes:
                1 Vote for this issue
                Watchers:
                0 Start watching this issue

                Dates

                • Created:
                  Updated:
                  Resolved:
                  Fix Release Date:
                  5/Dec/11