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

Clean theme: Coding ERROR detected when trying to export calendar

    Details

    • Testing Instructions:
      Hide
      1. Login as Admin and select Clean theme
      2. If not already added ADD a Calendar block to the front page and configure as required.
      3. In any page where the Calendar block appears CLICK on the Month which opens in a separate page.
      4. TEST that when clicking on the EXPORT button, that is below the centre calendar on that page, that there is NO error message, and that the check-boxes and settings to export are now visible in the new page that loads.
      Show
      Login as Admin and select Clean theme If not already added ADD a Calendar block to the front page and configure as required. In any page where the Calendar block appears CLICK on the Month which opens in a separate page. TEST that when clicking on the EXPORT button, that is below the centre calendar on that page, that there is NO error message, and that the check-boxes and settings to export are now visible in the new page that loads.
    • Affected Branches:
      MOODLE_25_STABLE, MOODLE_26_STABLE
    • Fixed Branches:
      MOODLE_25_STABLE
    • Pull Master Branch:
      MDL-40652_master

      Description

      See image for error and stack trace.

        Gliffy Diagrams

          Issue Links

            Activity

            Hide
            lazydaisy Mary Evans added a comment -

            Stack trace:

            Debug info:
            Error code: codingerror
            Stack trace:
             
                line 884 of \lib\blocklib.php: coding_exception thrown
                line 984 of \lib\blocklib.php: call to block_manager->check_region_is_known()
                line 997 of \lib\blocklib.php: call to block_manager->ensure_instances_exist()
                line 315 of \lib\blocklib.php: call to block_manager->ensure_content_created()
                line 1232 of \lib\outputrenderers.php: call to block_manager->get_content_for_region()
                line 3053 of \lib\outputrenderers.php: call to core_renderer->blocks_for_region()
                line 96 of \theme\clean\layout\columns3.php: call to core_renderer->blocks()
                line 847 of \lib\outputrenderers.php: call to include()
                line 777 of \lib\outputrenderers.php: call to core_renderer->render_page_layout()
                line 115 of \calendar\export.php: call to core_renderer->header()

            Show
            lazydaisy Mary Evans added a comment - Stack trace: Debug info: Error code: codingerror Stack trace:   line 884 of \lib\blocklib.php: coding_exception thrown line 984 of \lib\blocklib.php: call to block_manager->check_region_is_known() line 997 of \lib\blocklib.php: call to block_manager->ensure_instances_exist() line 315 of \lib\blocklib.php: call to block_manager->ensure_content_created() line 1232 of \lib\outputrenderers.php: call to block_manager->get_content_for_region() line 3053 of \lib\outputrenderers.php: call to core_renderer->blocks_for_region() line 96 of \theme\clean\layout\columns3.php: call to core_renderer->blocks() line 847 of \lib\outputrenderers.php: call to include() line 777 of \lib\outputrenderers.php: call to core_renderer->render_page_layout() line 115 of \calendar\export.php: call to core_renderer->header()
            Hide
            lazydaisy Mary Evans added a comment -

            Normally I would say that class="block-region" is missing from the container where the blacks are loaded, but since the new changes in Bootstrap themes, it is hard to decipher how or what, if anything, is added to the blocks in moodle/lib/blocklib.php. And since the exception is thrown from the block manager in that lib/blocklib.php, I would hazard a guess and say that it looks very much like my hunch is correct, and class="block-region" is indeed missing!

            Show
            lazydaisy Mary Evans added a comment - Normally I would say that class="block-region" is missing from the container where the blacks are loaded, but since the new changes in Bootstrap themes, it is hard to decipher how or what, if anything, is added to the blocks in moodle/lib/blocklib.php. And since the exception is thrown from the block manager in that lib/blocklib.php, I would hazard a guess and say that it looks very much like my hunch is correct, and class="block-region" is indeed missing!
            Hide
            lazydaisy Mary Evans added a comment -

            Adding watchers...

            Show
            lazydaisy Mary Evans added a comment - Adding watchers...
            Hide
            lazydaisy Mary Evans added a comment -

            OK...just checked the source code for columns3.php Standard layout, which is the layout used when exporting calendar, and find that although block-region class is there the actual ID is different than usual. Normally it is id="region-pre", however it has been rewritten as id="block-region-side-pre" which may account for the error.

            I don't see any real need for the change as the container would normally have had id="region-pre" so I think we need to change it back to that before we run into more problems of our own making.

            Show
            lazydaisy Mary Evans added a comment - OK...just checked the source code for columns3.php Standard layout, which is the layout used when exporting calendar, and find that although block-region class is there the actual ID is different than usual. Normally it is id="region-pre", however it has been rewritten as id="block-region-side-pre" which may account for the error. I don't see any real need for the change as the container would normally have had id="region-pre" so I think we need to change it back to that before we run into more problems of our own making.
            Hide
            gb2048 Gareth J Barnard added a comment -

            Bumped into this the other day regarding the Id I think it's used in the YUI bootstrap JS - will correct later as on ipod.

            Show
            gb2048 Gareth J Barnard added a comment - Bumped into this the other day regarding the Id I think it's used in the YUI bootstrap JS - will correct later as on ipod.
            Hide
            lazydaisy Mary Evans added a comment - - edited

            As I think there is a new learning curve heading our way, I have just added Jon Papaioannou who looks like he was instrumental in coding the calendar at some time in the past.

            Show
            lazydaisy Mary Evans added a comment - - edited As I think there is a new learning curve heading our way, I have just added Jon Papaioannou who looks like he was instrumental in coding the calendar at some time in the past.
            Hide
            lazydaisy Mary Evans added a comment - - edited

            OK...on first inspection of core/calendar/renderer.php it looks like the renderer is trying to add a 'fake' block in BLOCK_POS_LEFT or BLOCK_POS_RIGHT and since these are defined in blocklib.php as 'side-pre' and 'side-post' respectively, and the fact that these two 'compass points' are not recorded anywhere on the page at the time that core/calendar/renderer.php is busy rewriting the standard layout of that page, then it is obvious it is going to throw a wobbler, as in the ERROR displayed above.

            It works OK in Moodle standard themes possibly because side-pre and side-post are named on the page.

            I think we need to look at the way we are loading blocks in lib/outputrenderers.php for the Bootstrap themes, in that the aside container should already be in place on each of the layout pages, then the blocks for that region added when needed, and if it needs a container to do this, we could use a div with class="region-content" instead which is safer. And if it needs another ID then use the "block-region-side-pre" etc.,

            Show
            lazydaisy Mary Evans added a comment - - edited OK...on first inspection of core/calendar/renderer.php it looks like the renderer is trying to add a 'fake' block in BLOCK_POS_LEFT or BLOCK_POS_RIGHT and since these are defined in blocklib.php as 'side-pre' and 'side-post' respectively, and the fact that these two 'compass points' are not recorded anywhere on the page at the time that core/calendar/renderer.php is busy rewriting the standard layout of that page, then it is obvious it is going to throw a wobbler, as in the ERROR displayed above. It works OK in Moodle standard themes possibly because side-pre and side-post are named on the page. I think we need to look at the way we are loading blocks in lib/outputrenderers.php for the Bootstrap themes, in that the aside container should already be in place on each of the layout pages, then the blocks for that region added when needed, and if it needs a container to do this, we could use a div with class="region-content" instead which is safer. And if it needs another ID then use the "block-region-side-pre" etc.,
            Hide
            lazydaisy Mary Evans added a comment -

            On further exploration of this problem I found that in Afterburner theme (see image - MDL-40652-Afterburner) there are NO side blocks on that export page just a short list of checkboxes. So since the page layout for calender/export.php uses 'standard' changing this to 'base', which by default is without blocks, works without errors in the Clean theme (see image - MDL-40652-Clean).

            Show
            lazydaisy Mary Evans added a comment - On further exploration of this problem I found that in Afterburner theme (see image - MDL-40652 -Afterburner) there are NO side blocks on that export page just a short list of checkboxes. So since the page layout for calender/export.php uses 'standard' changing this to 'base', which by default is without blocks, works without errors in the Clean theme (see image - MDL-40652 -Clean).
            Hide
            lazydaisy Mary Evans added a comment -

            Adding images

            Show
            lazydaisy Mary Evans added a comment - Adding images
            Hide
            lazydaisy Mary Evans added a comment -

            Gareth, please can you Peer Review this for me if you have a few minutes? Cheers
            Mary

            Show
            lazydaisy Mary Evans added a comment - Gareth, please can you Peer Review this for me if you have a few minutes? Cheers Mary
            Hide
            gb2048 Gareth J Barnard added a comment -

            Dear Mary Evans,

            Will do.

            Gareth

            Show
            gb2048 Gareth J Barnard added a comment - Dear Mary Evans , Will do. Gareth
            Hide
            lazydaisy Mary Evans added a comment -

            It's amazing how it fixes this problem and yet does not affect those themes that use Canvas as a parent, so different themes have different layouts. Or rather those that take the lead from Canvas and have blocks in base $THEME->layouts where technically no blocks should be.

            Show
            lazydaisy Mary Evans added a comment - It's amazing how it fixes this problem and yet does not affect those themes that use Canvas as a parent, so different themes have different layouts. Or rather those that take the lead from Canvas and have blocks in base $THEME->layouts where technically no blocks should be.
            Hide
            gb2048 Gareth J Barnard added a comment -

            Tested in M25 branch. Works as expected. No code / style issues.

            Show
            gb2048 Gareth J Barnard added a comment - Tested in M25 branch. Works as expected. No code / style issues.
            Hide
            gb2048 Gareth J Barnard added a comment -

            Dear Mary Evans,

            I've peer reviewed the patch but have no power to take it any further.

            Cheers,

            Gareth

            Show
            gb2048 Gareth J Barnard added a comment - Dear Mary Evans , I've peer reviewed the patch but have no power to take it any further. Cheers, Gareth
            Hide
            gb2048 Gareth J Barnard added a comment -

            With regards 'Canvas' I think its a matter of interaction between renderers and themes. Perhaps there should be more separation with less assumptions on both parts.

            Show
            gb2048 Gareth J Barnard added a comment - With regards 'Canvas' I think its a matter of interaction between renderers and themes. Perhaps there should be more separation with less assumptions on both parts.
            Hide
            lazydaisy Mary Evans added a comment -

            Many thanks Gareth.

            Show
            lazydaisy Mary Evans added a comment - Many thanks Gareth.
            Hide
            stronk7 Eloy Lafuente (stronk7) added a comment -

            Integrated, thanks!

            Show
            stronk7 Eloy Lafuente (stronk7) added a comment - Integrated, thanks!
            Hide
            ankit_frenz Ankit Agarwal added a comment -

            No error was seen, on specified pages during testing.
            passing.
            Thanks

            Show
            ankit_frenz Ankit Agarwal added a comment - No error was seen, on specified pages during testing. passing. Thanks
            Hide
            damyon Damyon Wiese added a comment -

            Moodle has many old functions,
            And although they cause no malfunction,
            There comes a day,
            When they get deprecated away,
            And get and put on the list for expulsion.

            Thanks for all the reports/testing/fixes this week. This issue has been sent upstream.

            Show
            damyon Damyon Wiese added a comment - Moodle has many old functions, And although they cause no malfunction, There comes a day, When they get deprecated away, And get and put on the list for expulsion. Thanks for all the reports/testing/fixes this week. This issue has been sent upstream.
            Hide
            markn Mark Nelson added a comment -

            I created MDL-41995 and found out the reason for the missing menus is because of this change. The layout did not need to be changed, it just had to be called before the function calendar_preferences_button.

            Show
            markn Mark Nelson added a comment - I created MDL-41995 and found out the reason for the missing menus is because of this change. The layout did not need to be changed, it just had to be called before the function calendar_preferences_button.

              People

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

                Dates

                • Created:
                  Updated:
                  Resolved:
                  Fix Release Date:
                  9/Sep/13