Moodle
  1. Moodle
  2. MDL-37470

block_manager::add_blocks() ignores block weight parameter and offset

    Details

    • Type: Bug Bug
    • Status: Waiting for integration review
    • Priority: Minor Minor
    • Resolution: Unresolved
    • Affects Version/s: 2.2.6, 2.3.3, 2.4
    • Fix Version/s: 2.7.6, 2.8.4
    • Component/s: Blocks
    • Labels:
    • Testing Instructions:
      Hide

      Testing for admin_bookmarks positioning:

      Check the weight of the admin_bookmarks block (it will be 0): SELECT * FROM

      {block_instances} WHERE parentcontextid = 1 AND blockname = 'admin_bookmarks';
      Apply patch.
      Create new Moodle installation.
      Check the weight of the admin_bookmarks block (it will be 2): SELECT * FROM {block_instances}

      WHERE parentcontextid = 1 AND blockname = 'admin_bookmarks';

      Testing for my use case:

      Add this line to config.php: $CFG->defaultblocks_override = 'participants,calendar_month:activity_modules,news_items,calendar_upcoming';
      Create new course and see participants block between navigation and settings blocks.
      Replace with this line in config.php: $CFG->defaultblocks_override = ',,participants,calendar_month:activity_modules,news_items,calendar_upcoming';
      Attempt to create new course and get errors saying that the block '' does not exist.
      Apply patch.
      Create new course and see participants and calendar_month blocks after the navigation and settings blocks.

      Show
      Testing for admin_bookmarks positioning: Check the weight of the admin_bookmarks block (it will be 0): SELECT * FROM {block_instances} WHERE parentcontextid = 1 AND blockname = 'admin_bookmarks'; Apply patch. Create new Moodle installation. Check the weight of the admin_bookmarks block (it will be 2): SELECT * FROM {block_instances} WHERE parentcontextid = 1 AND blockname = 'admin_bookmarks'; Testing for my use case: Add this line to config.php: $CFG->defaultblocks_override = 'participants,calendar_month:activity_modules,news_items,calendar_upcoming'; Create new course and see participants block between navigation and settings blocks. Replace with this line in config.php: $CFG->defaultblocks_override = ',,participants,calendar_month:activity_modules,news_items,calendar_upcoming'; Attempt to create new course and get errors saying that the block '' does not exist. Apply patch. Create new course and see participants and calendar_month blocks after the navigation and settings blocks.
    • Affected Branches:
      MOODLE_22_STABLE, MOODLE_23_STABLE, MOODLE_24_STABLE
    • Fixed Branches:
      MOODLE_27_STABLE, MOODLE_28_STABLE
    • Pull from Repository:
    • Pull 2.7 Branch:
    • Pull 2.8 Branch:
    • Pull Master Branch:

      Description

      When you use the add_blocks() function, the weight (block positioning) is ignored in the two ways that it can be provided.

        Gliffy Diagrams

          Activity

          Hide
          Jonathan Champ added a comment -

          Here is a patch that uses the initial weight from the weight parameter (fixes the admin_bookmarks positioning that Sam H. added in 2009 to blocks_add_default_system_blocks()) and allows you to customize the weighting offset between blocks so that default_block_override and similar lines can avoid conflicting with the navigation and settings default page blocks.

          Currently, when you create a new course with blocks specified for the left column, the order jumbles the first non-default block in with the two default blocks (navigation and settings).

          Show
          Jonathan Champ added a comment - Here is a patch that uses the initial weight from the weight parameter (fixes the admin_bookmarks positioning that Sam H. added in 2009 to blocks_add_default_system_blocks()) and allows you to customize the weighting offset between blocks so that default_block_override and similar lines can avoid conflicting with the navigation and settings default page blocks. Currently, when you create a new course with blocks specified for the left column, the order jumbles the first non-default block in with the two default blocks (navigation and settings).
          Hide
          Jonathan Champ added a comment -

          Another implementation that I had considered was to set the weight function parameter to null and only if it was set to a non-null value would I specify the weight. Otherwise, if it were possible to simple append the blocks to the page region, that seems like the best bet. Unfortunately, the code from "add_block_at_end_of_default_region()" did not work the same way in the context of populating the initial page blocks during my testing.

          Show
          Jonathan Champ added a comment - Another implementation that I had considered was to set the weight function parameter to null and only if it was set to a non-null value would I specify the weight. Otherwise, if it were possible to simple append the blocks to the page region, that seems like the best bet. Unfortunately, the code from "add_block_at_end_of_default_region()" did not work the same way in the context of populating the initial page blocks during my testing.
          Hide
          Michael de Raadt added a comment -

          Thanks for spotting that and providing a patch.

          It would be really helpful if you could add a set of testing steps.

          Show
          Michael de Raadt added a comment - Thanks for spotting that and providing a patch. It would be really helpful if you could add a set of testing steps.
          Hide
          Michael de Raadt added a comment -

          Sam: I've added you as a watcher on this issue as you seem to have worked on this code in the past. Perhaps you could have a look at the patch provided.

          Show
          Michael de Raadt added a comment - Sam: I've added you as a watcher on this issue as you seem to have worked on this code in the past. Perhaps you could have a look at the patch provided.
          Hide
          Michael Grant added a comment -

          I've turned the patch into a branch for integration, tested it and everything appears to be working. Only fixing this as I tried to use add_blocks and setting the weight, and spending half a day tearing hair out to realise it was this. So did something about it. I don't have permissions to put this in the right place, but.

          Repository: https://github.com/mike-grant/moodle
          Pull 2.7 Branch: MDL-37470_27
          Pull 2.7 Diff URL:https://github.com/mike-grant/moodle/compare/MOODLE_27_STABLE...MDL-37470_27
          Pull 2.8 Branch:MDL-49041_28
          Pull 2.8 Diff URL:https://github.com/mike-grant/moodle/compare/MOODLE_28_STABLE...MDL-37470_28
          Pull Master Branch:MDL-49041
          Pull Master Diff URL:https://github.com/mike-grant/moodle/compare/master...MDL-37470

          Hoping that's all the information that's needed..

          Thanks

          Show
          Michael Grant added a comment - I've turned the patch into a branch for integration, tested it and everything appears to be working. Only fixing this as I tried to use add_blocks and setting the weight, and spending half a day tearing hair out to realise it was this. So did something about it. I don't have permissions to put this in the right place, but. Repository: https://github.com/mike-grant/moodle Pull 2.7 Branch: MDL-37470 _27 Pull 2.7 Diff URL: https://github.com/mike-grant/moodle/compare/MOODLE_27_STABLE...MDL-37470_27 Pull 2.8 Branch: MDL-49041 _28 Pull 2.8 Diff URL: https://github.com/mike-grant/moodle/compare/MOODLE_28_STABLE...MDL-37470_28 Pull Master Branch: MDL-49041 Pull Master Diff URL: https://github.com/mike-grant/moodle/compare/master...MDL-37470 Hoping that's all the information that's needed.. Thanks
          Hide
          Jonathan Champ added a comment -

          Thanks Mike! I'll try and push this up the chain.

          Show
          Jonathan Champ added a comment - Thanks Mike! I'll try and push this up the chain.
          Hide
          CiBoT added a comment -

          Code verified against automated checks.

          Checked MDL-37470 using repository: https://github.com/mike-grant/moodle

          More information about this report

          Show
          CiBoT added a comment - Code verified against automated checks. Checked MDL-37470 using repository: https://github.com/mike-grant/moodle MOODLE_27_STABLE (0 errors / 0 warnings) [branch: MDL-37470_27 | CI Job ] MOODLE_28_STABLE (0 errors / 0 warnings) [branch: MDL-37470_28 | CI Job ] master (0 errors / 0 warnings) [branch: MDL-37470 | CI Job ] More information about this report
          Hide
          Tim Hunt added a comment -

          I am afraid that I can see several problems here:

          blocks_parse_default_blocks_list

          The changes in blocks_parse_default_blocks_list are quite subtle. It took me a while to work out what is going on. The point is that you can define a string like ',,,block_x,,,blocky' to mean block_x has weight 3, and block y has weight 6. However, how is anyone supposed to know that?

          • The format of the strings being parsed needs to be documented somewhere, probably in config-dist.php where this is used, and in the PHPdoc @param string $blocksstr.
          • The PHPdocs for the return-type of blocks_parse_default_blocks_list needs to explain the details of the array that is returned.

          Note that this patch probably introduces an unwanted change to the result. In the past parsing ':block_html' would return array(BLOCK_POS_RIGHT => array('block_html')). Now it will return array(BLOCK_POS_LEFT => array(), BLOCK_POS_RIGHT => array('block_html'))

          • I think we need to undo that unwanted change.
          • blocks_parse_default_blocks_list is exactly the sort of function that should have unit tests, and now would be a really good time to add some.

          add_blocks

          I think the changes to the code are OK. However, existing PHP doc comment is not OK, and now would be a really good time to fix it to explain how the $weight argument works in combination with the $blocks array.

          I hope it is OK for you to resolve these issues before this is pushed forwards for integration.

          Show
          Tim Hunt added a comment - I am afraid that I can see several problems here: blocks_parse_default_blocks_list The changes in blocks_parse_default_blocks_list are quite subtle. It took me a while to work out what is going on. The point is that you can define a string like ',,,block_x,,,blocky' to mean block_x has weight 3, and block y has weight 6. However, how is anyone supposed to know that? The format of the strings being parsed needs to be documented somewhere, probably in config-dist.php where this is used, and in the PHPdoc @param string $blocksstr. The PHPdocs for the return-type of blocks_parse_default_blocks_list needs to explain the details of the array that is returned. Note that this patch probably introduces an unwanted change to the result. In the past parsing ':block_html' would return array(BLOCK_POS_RIGHT => array('block_html')). Now it will return array(BLOCK_POS_LEFT => array(), BLOCK_POS_RIGHT => array('block_html')) I think we need to undo that unwanted change. blocks_parse_default_blocks_list is exactly the sort of function that should have unit tests, and now would be a really good time to add some. add_blocks I think the changes to the code are OK. However, existing PHP doc comment is not OK, and now would be a really good time to fix it to explain how the $weight argument works in combination with the $blocks array. I hope it is OK for you to resolve these issues before this is pushed forwards for integration.
          Hide
          Michael Grant added a comment -

          Thanks for the speedy code review Tim, appreciate it. I'll take a look at them changes tomorrow and see when I could complete them.

          I'm not 100% sure on what the 'final' improvement of the PHP doc comment would look like, but guess that's the point of this.

          Once again, thanks Tim.

          Show
          Michael Grant added a comment - Thanks for the speedy code review Tim, appreciate it. I'll take a look at them changes tomorrow and see when I could complete them. I'm not 100% sure on what the 'final' improvement of the PHP doc comment would look like, but guess that's the point of this. Once again, thanks Tim.
          Hide
          Michael Grant added a comment -

          I've done everything you suggested from the peer review Tim, except for the documentation in config-dist.php. I can't quite think of a succinct way of explaining it!

          Anyone able to add CI tag again for another pass? Thanks

          Show
          Michael Grant added a comment - I've done everything you suggested from the peer review Tim, except for the documentation in config-dist.php. I can't quite think of a succinct way of explaining it! Anyone able to add CI tag again for another pass? Thanks
          Hide
          CiBoT added a comment -
          Show
          CiBoT added a comment - Fails against automated checks. Checked MDL-37470 using repository: https://github.com/mike-grant/moodle MOODLE_27_STABLE (15 errors / 0 warnings) [branch: MDL-37470_27 | CI Job ] phplint (0/0) , php (9/0) , js (0/0) , css (0/0) , phpdoc (5/0) , commit (1/0) , savepoint (0/0) , thirdparty (0/0) , MOODLE_28_STABLE (15 errors / 0 warnings) [branch: MDL-37470_28 | CI Job ] phplint (0/0) , php (9/0) , js (0/0) , css (0/0) , phpdoc (5/0) , commit (1/0) , savepoint (0/0) , thirdparty (0/0) , master (15 errors / 0 warnings) [branch: MDL-37470 | CI Job ] phplint (0/0) , php (9/0) , js (0/0) , css (0/0) , phpdoc (5/0) , commit (1/0) , savepoint (0/0) , thirdparty (0/0) , More information about this report
          Hide
          Tim Hunt added a comment -

          You need to merge the two commits on the branch so there is only one. Until you do, CiBoT won't be happy.

          Show
          Tim Hunt added a comment - You need to merge the two commits on the branch so there is only one. Until you do, CiBoT won't be happy.
          Hide
          CiBoT added a comment -
          Show
          CiBoT added a comment - Fails against automated checks. Checked MDL-37470 using repository: https://github.com/mike-grant/moodle MOODLE_27_STABLE (15 errors / 0 warnings) [branch: MDL-37470_27 | CI Job ] phplint (0/0) , php (9/0) , js (0/0) , css (0/0) , phpdoc (5/0) , commit (1/0) , savepoint (0/0) , thirdparty (0/0) , MOODLE_28_STABLE (15 errors / 0 warnings) [branch: MDL-37470_28 | CI Job ] phplint (0/0) , php (9/0) , js (0/0) , css (0/0) , phpdoc (5/0) , commit (1/0) , savepoint (0/0) , thirdparty (0/0) , master (4 errors / 0 warnings) [branch: MDL-37470 | CI Job ] phplint (0/0) , php (0/0) , js (0/0) , css (0/0) , phpdoc (1/0) , commit (3/0) , savepoint (0/0) , thirdparty (0/0) , More information about this report
          Hide
          Tim Hunt added a comment -

          Sorry, I ran out of time today. If you can try to sort out CiBoT, I will look at this tomorrow. (In case you are wondering, I am in the UK time-zone.)

          Show
          Tim Hunt added a comment - Sorry, I ran out of time today. If you can try to sort out CiBoT, I will look at this tomorrow. (In case you are wondering, I am in the UK time-zone.)
          Hide
          Michael Grant added a comment -

          Hi Tim,

          Whats the best way to squash what will be 3 commits, to one? Merge? Rebase? reset --soft?

          Thanks

          Show
          Michael Grant added a comment - Hi Tim, Whats the best way to squash what will be 3 commits, to one? Merge? Rebase? reset --soft? Thanks
          Hide
          Tim Hunt added a comment -

          This is git, there are many ways you could do it, the important thing is the final result. I would probably use

          git rebase -i master

          Show
          Tim Hunt added a comment - This is git, there are many ways you could do it, the important thing is the final result. I would probably use git rebase -i master
          Hide
          Michael Grant added a comment -

          Here's praying this is the one for CI and you're happy!

          Show
          Michael Grant added a comment - Here's praying this is the one for CI and you're happy!
          Hide
          CiBoT added a comment -

          Code verified against automated checks.

          Checked MDL-37470 using repository: https://github.com/mike-grant/moodle

          More information about this report

          Show
          CiBoT added a comment - Code verified against automated checks. Checked MDL-37470 using repository: https://github.com/mike-grant/moodle MOODLE_27_STABLE (0 errors / 0 warnings) [branch: MDL-37470_27 | CI Job ] MOODLE_28_STABLE (0 errors / 0 warnings) [branch: MDL-37470_28 | CI Job ] master (0 errors / 0 warnings) [branch: MDL-37470 | CI Job ] More information about this report
          Hide
          Tim Hunt added a comment -

          Sorry it took me so long to review this again. it looks spot-on now. Submitting for integration.

          Show
          Tim Hunt added a comment - Sorry it took me so long to review this again. it looks spot-on now. Submitting for integration.
          Hide
          Michael Grant added a comment -

          Thanks for the help Tim! Much appreciated!

          Show
          Michael Grant added a comment - Thanks for the help Tim! Much appreciated!

            People

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

              Dates

              • Created:
                Updated: