Moodle
  1. Moodle
  2. MDL-30327

Block removal should have automatically remove 'config_plugins' settings

    Details

    • Type: Improvement Improvement
    • Status: Closed
    • Priority: Major 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
    • Rank:
      32681

      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.

        Issue Links

          Activity

          Hide
          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
          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
          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
          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
          Petr Škoda added a comment -

          Thanks for the report and detailed explanation!

          Show
          Petr Škoda added a comment - Thanks for the report and detailed explanation!
          Hide
          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
          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
          Petr Škoda added a comment -

          please create a new issue, thanks

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

          (getting on this after confirmation with David)

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

          Created MDL-30387 to clean up core blocks

          Show
          Sam Hemelryk added a comment - Created MDL-30387 to clean up core blocks
          Hide
          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
          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
          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
          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
          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
          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
          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
          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: