Moodle
  1. Moodle
  2. MDL-41516

jQuery libraries cannot be included in blocks when themes have a call to 'body_attributes()'

    Details

    • Testing Instructions:
      Hide

      Open blocks/html/block_html.php, find the block_html class and add:

          public function get_required_javascript() {
              parent::get_required_javascript();
              $this->page->requires->jquery();
              $this->page->requires->jquery_plugin('ui');
              $this->page->requires->jquery_plugin('ui-css');
          }
      

      Checking against all themes:

      • Turn editing on;
      • Add an HTML block;
      • Confirm that no errors were displayed - those relating to jQuery being called after output has started
      Show
      Open blocks/html/block_html.php, find the block_html class and add: public function get_required_javascript() { parent::get_required_javascript(); $ this ->page->requires->jquery(); $ this ->page->requires->jquery_plugin('ui'); $ this ->page->requires->jquery_plugin('ui-css'); } Checking against all themes: Turn editing on; Add an HTML block; Confirm that no errors were displayed - those relating to jQuery being called after output has started
    • Workaround:
      Hide

      None at this moment in time.

      Show
      None at this moment in time.
    • Affected Branches:
      MOODLE_25_STABLE, MOODLE_26_STABLE
    • Fixed Branches:
      MOODLE_25_STABLE
    • Pull 2.5 Branch:
      MDL-41516-m25
    • Pull Master Branch:
    • Rank:
      52578

      Description

      On the themes forum an issue has been highlighted in post https://moodle.org/mod/forum/discuss.php?d=236254 where the stack trace given in the custom block is:

      Can not add jQuery plugins after starting page output!
      
      line 397 of /lib/outputrequirementslib.php: call to debugging()
      line 11 of /blocks/myblock/block_myblock.php: call to page_requirements_manager->jquery_plugin()
      line 293 of /blocks/moodleblock.class.php: call to block_myblock->get_required_javascript()
      line 238 of /blocks/moodleblock.class.php: call to block_base->formatted_contents()
      line 951 of /lib/blocklib.php: call to block_base->get_content_for_output()
      line 1003 of /lib/blocklib.php: call to block_manager->create_block_contents()
      line 353 of /lib/blocklib.php: call to block_manager->ensure_content_created()
      line 3071 of /lib/outputrenderers.php: call to block_manager->region_has_content()
      line 3107 of /lib/outputrenderers.php: call to core_renderer->body_css_classes()
      line 49 of /theme/clean/layout/columns3.php: call to core_renderer->body_attributes()
      line 850 of /lib/outputrenderers.php: call to include()
      line 780 of /lib/outputrenderers.php: call to core_renderer->render_page_layout()
      line 101 of /index.php: call to core_renderer->header()
      

      It is stated that the jQuery libraries have been included correctly:

          public function get_required_javascript() {
              parent::get_required_javascript();
       
              $this->page->requires->jquery();
              $this->page->requires->jquery_plugin('ui');
              $this->page->requires->jquery_plugin('ui-css');
              $this->page->requires->js('/blocks/myblock/myscript.js');
          }
      

      Which matches the information given on http://docs.moodle.org/dev/jQuery#jQuery_UI_in_add-on_block.

      It looks like with the 'Clean' theme that test '4' on MDL-15727 will now fail because the method 'body_attributes()' was added in MDL-39838 and applied to the 'Clean' theme in MDL-40089 after the tests on MDL-15727 were run. Therefore I suspect a regression has taken place.

        Issue Links

          Activity

          Hide
          Mary Evans added a comment -

          I have just added Libraries to the list of labels for this as this too is relevant to this bug, so hopefully Petr Škoda will see this.

          Show
          Mary Evans added a comment - I have just added Libraries to the list of labels for this as this too is relevant to this bug, so hopefully Petr Škoda will see this.
          Hide
          Mary Evans added a comment -

          Gareth, in base it is written thus...with a join

          <body id="<?php p($PAGE->bodyid) ?>" class="<?php p($PAGE->bodyclasses.' '.join(' ', $bodyclasses)) ?>">
          Show
          Mary Evans added a comment - Gareth, in base it is written thus...with a join <body id= "<?php p($PAGE->bodyid) ?>" class= "<?php p($PAGE->bodyclasses.' '.join(' ', $bodyclasses)) ?>" >
          Hide
          Gareth J Barnard added a comment - - edited

          Dear Mary Evans,

          True, but if you look how '$bodyclasses' is defined within the layout file you will find that it's used for the layout column thing that 'Clean' used to have when it had 'general.php' before it moved to 'columnX.php' and thus serves no purpose in 'Clean' which is why I took it out! .

          But there could be a need to be intelligent in the replacement to have things like 'two-column' added as an example:

          <body id="<?php p($PAGE->bodyid) ?>" class="<?php p($PAGE->bodyclasses)?> two-column">
          

          Cheers,

          Gareth

          Show
          Gareth J Barnard added a comment - - edited Dear Mary Evans , True, but if you look how '$bodyclasses' is defined within the layout file you will find that it's used for the layout column thing that 'Clean' used to have when it had 'general.php' before it moved to 'columnX.php' and thus serves no purpose in 'Clean' which is why I took it out! . But there could be a need to be intelligent in the replacement to have things like 'two-column' added as an example: <body id= "<?php p($PAGE->bodyid) ?>" class= "<?php p($PAGE->bodyclasses)?> two-column" > Cheers, Gareth
          Hide
          Mary Evans added a comment - - edited

          But was it it not to do with this lib/outputrenderers.php function?

              /**
               * Returns HTML attributes to use within the body tag. This includes an ID and classes.
               *
               * @since 2.5.1 2.6
               * @param string|array $additionalclasses Any additional classes to give the body tag,
               * @return string
               */
              public function body_attributes($additionalclasses = array()) {
                  if (!is_array($additionalclasses)) {
                      $additionalclasses = explode(' ', $additionalclasses);
                  }
                  return ' id="'. $this->body_id().'" class="'.$this->body_css_classes($additionalclasses).'"';
              }
          

          So perhaps you should have left the body attribute code in.

          Show
          Mary Evans added a comment - - edited But was it it not to do with this lib/outputrenderers.php function? /** * Returns HTML attributes to use within the body tag. This includes an ID and classes. * * @since 2.5.1 2.6 * @param string|array $additionalclasses Any additional classes to give the body tag, * @ return string */ public function body_attributes($additionalclasses = array()) { if (!is_array($additionalclasses)) { $additionalclasses = explode(' ', $additionalclasses); } return ' id= "'. $ this ->body_id().'" class= "'.$ this ->body_css_classes($additionalclasses).'" '; } So perhaps you should have left the body attribute code in.
          Hide
          Mary Evans added a comment - - edited

          And if you want to add extra body classes they should be define in the layout as we do in standard themes. So I would say you removing that use of attributes was incorrect at the time.

          Show
          Mary Evans added a comment - - edited And if you want to add extra body classes they should be define in the layout as we do in standard themes. So I would say you removing that use of attributes was incorrect at the time.
          Hide
          Gareth J Barnard added a comment - - edited

          No, I'm not advocating that at all. I'm saying that the workaround is a temporary workaround until the issue of the method 'body_css_classes()' being called by 'body_attributes()' eventually causing the block JavaScript issue is resolved as evident in the stack trace. The workaround is not a solution to the problem, all it does is bypass the 'body_attributes()' method as other themes do, which is why Paul stated only having an issue with the 'Clean' theme.

          And if I'd left the 'body_attributes()' code in then the problem would happen again in the workaround! Catch-22.

          Show
          Gareth J Barnard added a comment - - edited No, I'm not advocating that at all. I'm saying that the workaround is a temporary workaround until the issue of the method 'body_css_classes()' being called by 'body_attributes()' eventually causing the block JavaScript issue is resolved as evident in the stack trace. The workaround is not a solution to the problem, all it does is bypass the 'body_attributes()' method as other themes do, which is why Paul stated only having an issue with the 'Clean' theme. And if I'd left the 'body_attributes()' code in then the problem would happen again in the workaround! Catch-22.
          Hide
          Gareth J Barnard added a comment - - edited

          So, I'm saying the workaround is for the 'Clean' theme to act like the other themes until a solution is found.

          Show
          Gareth J Barnard added a comment - - edited So, I'm saying the workaround is for the 'Clean' theme to act like the other themes until a solution is found.
          Hide
          Gareth J Barnard added a comment -

          Looking at the thread on the theme's forum, it looks like the workaround does not work:....

          Change 'Clean' theme to use '$PAGE->bodyclasses' instead of .'body_attributes()' in it's layout files etc. Such as:

          <body id="<?php p($PAGE->bodyid) ?>" class="<?php p($PAGE->bodyclasses) ?>">
          

          But this needs to be tested.

          Show
          Gareth J Barnard added a comment - Looking at the thread on the theme's forum, it looks like the workaround does not work:.... Change 'Clean' theme to use '$PAGE->bodyclasses' instead of .'body_attributes()' in it's layout files etc. Such as: <body id= "<?php p($PAGE->bodyid) ?>" class= "<?php p($PAGE->bodyclasses) ?>" > But this needs to be tested.
          Hide
          Paul P added a comment - - edited

          To duplicate the error, edit moodle/blocks/html/block_html.php and add the following function immediately after class block_html extends block_base{

          public function get_required_javascript() {
          parent::get_required_javascript();

          $this->page->requires->jquery();
          $this->page->requires->jquery_plugin('ui');
          $this->page->requires->jquery_plugin('ui-css');

          }

          Then add an HTML block to a page. If you open that page in any theme (other than Clean) no error is produced. However, if you change the theme to Clean, you get the errors.

          Show
          Paul P added a comment - - edited To duplicate the error, edit moodle/blocks/html/block_html.php and add the following function immediately after class block_html extends block_base{ public function get_required_javascript() { parent::get_required_javascript(); $this->page->requires->jquery(); $this->page->requires->jquery_plugin('ui'); $this->page->requires->jquery_plugin('ui-css'); } Then add an HTML block to a page. If you open that page in any theme (other than Clean) no error is produced. However, if you change the theme to Clean, you get the errors.
          Hide
          Andrew Nicols added a comment -

          Okay,

          After starting to chase a wild-goose around Moodle, I've found the cause of this.

          I've added the code that Paul P suggests to confirm the issue.
          I've then thrown an exception into the newly created get_required_javascript() function, shown below:

          Stack trace:
          line 30 of /blocks/html/block_html.php: moodle_exception thrown
          line 297 of /blocks/moodleblock.class.php: call to block_html->get_required_javascript()
          line 238 of /blocks/moodleblock.class.php: call to block_base->formatted_contents()
          line 956 of /lib/blocklib.php: call to block_base->get_content_for_output()
          line 1008 of /lib/blocklib.php: call to block_manager->create_block_contents()
          line 353 of /lib/blocklib.php: call to block_manager->ensure_content_created()
          line 3202 of /lib/outputrenderers.php: call to block_manager->region_has_content()
          line 3238 of /lib/outputrenderers.php: call to core_renderer->body_css_classes()
          line 33 of /theme/clean/layout/columns3.php: call to core_renderer->body_attributes()
          line 850 of /lib/outputrenderers.php: call to include()
          line 780 of /lib/outputrenderers.php: call to core_renderer->render_page_layout()
          line 101 of /index.php: call to core_renderer->header()
          

          As you can see, this is triggered by body_attributes() and eventually calls jquery() and jquery_plugin().

          Since those functions must be called before the page head is started, they have to be set before standard_head_html() is called.

          However, looking at the columns3.php layout (as an example) in the clean theme:

          • on line 45; you call standard_head_html();
          • on line 49; you call body_attributes();

          Therefore you just need to move the body_attributes() call above standard_head_html(). In other themes, I notice you set various variables early at the top of the layout.

          Show
          Andrew Nicols added a comment - Okay, After starting to chase a wild-goose around Moodle, I've found the cause of this. I've added the code that Paul P suggests to confirm the issue. I've then thrown an exception into the newly created get_required_javascript() function, shown below: Stack trace: line 30 of /blocks/html/block_html.php: moodle_exception thrown line 297 of /blocks/moodleblock.class.php: call to block_html->get_required_javascript() line 238 of /blocks/moodleblock.class.php: call to block_base->formatted_contents() line 956 of /lib/blocklib.php: call to block_base->get_content_for_output() line 1008 of /lib/blocklib.php: call to block_manager->create_block_contents() line 353 of /lib/blocklib.php: call to block_manager->ensure_content_created() line 3202 of /lib/outputrenderers.php: call to block_manager->region_has_content() line 3238 of /lib/outputrenderers.php: call to core_renderer->body_css_classes() line 33 of /theme/clean/layout/columns3.php: call to core_renderer->body_attributes() line 850 of /lib/outputrenderers.php: call to include() line 780 of /lib/outputrenderers.php: call to core_renderer->render_page_layout() line 101 of /index.php: call to core_renderer->header() As you can see, this is triggered by body_attributes() and eventually calls jquery() and jquery_plugin(). Since those functions must be called before the page head is started, they have to be set before standard_head_html() is called. However, looking at the columns3.php layout (as an example) in the clean theme: on line 45; you call standard_head_html(); on line 49; you call body_attributes(); Therefore you just need to move the body_attributes() call above standard_head_html(). In other themes, I notice you set various variables early at the top of the layout.
          Hide
          Andrew Nicols added a comment -

          Patch attached, but since themes is not really my knowledge area, I'll get some feedback before submitting 2.5 branch. I may be barking up the wrong tree for a fix, though I don't think I am.

          Petr Škoda, do you think that this is correct, or should body_attributes() not really rely on page output having not started already? E.g. is this fix just a bodge/workaround and not really fixing the issue.

          Show
          Andrew Nicols added a comment - Patch attached, but since themes is not really my knowledge area, I'll get some feedback before submitting 2.5 branch. I may be barking up the wrong tree for a fix, though I don't think I am. Petr Škoda , do you think that this is correct, or should body_attributes() not really rely on page output having not started already? E.g. is this fix just a bodge/workaround and not really fixing the issue.
          Hide
          Andrew Nicols added a comment -

          If the above fix is correct, I'll provide a backport branch for 2.5 and will submit an additional patch for all other themes on master.

          Show
          Andrew Nicols added a comment - If the above fix is correct, I'll provide a backport branch for 2.5 and will submit an additional patch for all other themes on master.
          Hide
          Gareth J Barnard added a comment - - edited

          Hi Andrew Nicols, thanks for looking at this. 'body_attributes()' was added in MDL-40089 (which has been integrated but the tracker seems somewhat sparse and umm, open) - the call is in exactly the same place as the old '$PAGE->bodyclasses' reference so would assume it was a like for like replacement.

          Paul says on the theme thread referenced by this issue that his code is actually called three times. This does not make sense to me and feels like an optimisation waiting to happen. Somehow I feel that the wrong methods are being called for the overall function and purpose of the method. That 'region_has_content()' should be optimised such that it stores it's result on the first call and then returns it upon repeated calls to save processor time - if in the overall 'Object Interaction Diagram' of things that this meant an earlier step generated the result and had the JavaScript included on the page manager before the header was outputted, then that would be another solution that should also speed things up a bit.

          From a theme's perspective it seems logical to me that 'body_attributes()' should be called on the same line as the 'body' tag and therefore after the header has been outputted. It would make no sense to a theme designer to think that this method caused header information to be outputted. You would only think that it fetched the attributes it needs.

          Cheers,

          Gareth

          Show
          Gareth J Barnard added a comment - - edited Hi Andrew Nicols , thanks for looking at this. 'body_attributes()' was added in MDL-40089 (which has been integrated but the tracker seems somewhat sparse and umm, open) - the call is in exactly the same place as the old '$PAGE->bodyclasses' reference so would assume it was a like for like replacement. Paul says on the theme thread referenced by this issue that his code is actually called three times. This does not make sense to me and feels like an optimisation waiting to happen. Somehow I feel that the wrong methods are being called for the overall function and purpose of the method. That 'region_has_content()' should be optimised such that it stores it's result on the first call and then returns it upon repeated calls to save processor time - if in the overall 'Object Interaction Diagram' of things that this meant an earlier step generated the result and had the JavaScript included on the page manager before the header was outputted, then that would be another solution that should also speed things up a bit. From a theme's perspective it seems logical to me that 'body_attributes()' should be called on the same line as the 'body' tag and therefore after the header has been outputted. It would make no sense to a theme designer to think that this method caused header information to be outputted. You would only think that it fetched the attributes it needs. Cheers, Gareth
          Hide
          Andrew Nicols added a comment -

          Hi Gareth,

          The old way of doing it just accessed $PAGE->bodyclasses, but body_attributes() does more than that. It also gets the block_regions for that page and adds them to the list of classes.

          If you look at the stacktrace for anywhere calling jquery(), you'll see that it's parented by body_attributes() at some point.

          Show
          Andrew Nicols added a comment - Hi Gareth, The old way of doing it just accessed $PAGE->bodyclasses, but body_attributes() does more than that. It also gets the block_regions for that page and adds them to the list of classes. If you look at the stacktrace for anywhere calling jquery(), you'll see that it's parented by body_attributes() at some point.
          Hide
          Andrew Nicols added a comment -

          Sam Hemelryk, you wrote body_attributes() and friends, could really do with your views here.
          Should we be caching the output of blocks->get_regions() if it's called multiple times?

          It's easy to verify it is called multiple times for a single pageload.

          Show
          Andrew Nicols added a comment - Sam Hemelryk , you wrote body_attributes() and friends, could really do with your views here. Should we be caching the output of blocks->get_regions() if it's called multiple times? It's easy to verify it is called multiple times for a single pageload.
          Hide
          Gareth J Barnard added a comment -

          But why should 'body_attrbutes()' end up calling 'get_required_javascript()'? Makes no sense as the latter would not produce any CSS attributes?

          Show
          Gareth J Barnard added a comment - But why should 'body_attrbutes()' end up calling 'get_required_javascript()'? Makes no sense as the latter would not produce any CSS attributes?
          Hide
          Andrew Nicols added a comment -

          Here are the stacktraces for each of the three places it's called:

          line 932 of /lib/blocklib.php: call to block_manager->get_regions()
          line 1417 of /lib/pagelib.php: call to block_manager->create_all_block_instances()
          line 848 of /lib/pagelib.php: call to moodle_page->starting_output()
          line 775 of /lib/outputrenderers.php: call to moodle_page->set_state()
          line 101 of /index.php: call to core_renderer->header()
          
          line 297 of /lib/outputrequirementslib.php: call to block_manager->get_regions()
          line 1312 of /lib/outputrequirementslib.php: call to page_requirements_manager->init_requirements_data()
          line 411 of /lib/outputrenderers.php: call to page_requirements_manager->get_head_code()
          line 45 of /theme/clean/layout/columns3.php: call to core_renderer->standard_head_html()
          line 850 of /lib/outputrenderers.php: call to include()
          line 780 of /lib/outputrenderers.php: call to core_renderer->render_page_layout()
          line 101 of /index.php: call to core_renderer->header()
          
          line 3200 of /lib/outputrenderers.php: call to block_manager->get_regions()
          line 3238 of /lib/outputrenderers.php: call to core_renderer->body_css_classes()
          line 49 of /theme/clean/layout/columns3.php: call to core_renderer->body_attributes()
          line 850 of /lib/outputrenderers.php: call to include()
          line 780 of /lib/outputrenderers.php: call to core_renderer->render_page_layout()
          line 101 of /index.php: call to core_renderer->header()
          

          So that's:

          • line 49 of /theme/clean/layout/columns3.php: call to core_renderer->body_attributes()
          • line 45 of /theme/clean/layout/columns3.php: call to core_renderer->standard_head_html()
          • line 775 of /lib/outputrenderers.php: call to moodle_page->set_state()
          Show
          Andrew Nicols added a comment - Here are the stacktraces for each of the three places it's called: line 932 of /lib/blocklib.php: call to block_manager->get_regions() line 1417 of /lib/pagelib.php: call to block_manager->create_all_block_instances() line 848 of /lib/pagelib.php: call to moodle_page->starting_output() line 775 of /lib/outputrenderers.php: call to moodle_page->set_state() line 101 of /index.php: call to core_renderer->header() line 297 of /lib/outputrequirementslib.php: call to block_manager->get_regions() line 1312 of /lib/outputrequirementslib.php: call to page_requirements_manager->init_requirements_data() line 411 of /lib/outputrenderers.php: call to page_requirements_manager->get_head_code() line 45 of /theme/clean/layout/columns3.php: call to core_renderer->standard_head_html() line 850 of /lib/outputrenderers.php: call to include() line 780 of /lib/outputrenderers.php: call to core_renderer->render_page_layout() line 101 of /index.php: call to core_renderer->header() line 3200 of /lib/outputrenderers.php: call to block_manager->get_regions() line 3238 of /lib/outputrenderers.php: call to core_renderer->body_css_classes() line 49 of /theme/clean/layout/columns3.php: call to core_renderer->body_attributes() line 850 of /lib/outputrenderers.php: call to include() line 780 of /lib/outputrenderers.php: call to core_renderer->render_page_layout() line 101 of /index.php: call to core_renderer->header() So that's: line 49 of /theme/clean/layout/columns3.php: call to core_renderer->body_attributes() line 45 of /theme/clean/layout/columns3.php: call to core_renderer->standard_head_html() line 775 of /lib/outputrenderers.php: call to moodle_page->set_state()
          Hide
          Andrew Nicols added a comment -

          Gareth J Barnard it's not caring about the JS output, it's find out the list of blocks, which in turns ensures that they're instantiated, which includes setting up the JS.

          Show
          Andrew Nicols added a comment - Gareth J Barnard it's not caring about the JS output, it's find out the list of blocks, which in turns ensures that they're instantiated, which includes setting up the JS.
          Hide
          Gareth J Barnard added a comment - - edited

          So could 'get_content_for_output($output)' in 'moodleblock.class.php' cache the result of it's 'formatted_contents($output)'? Or once instantiated they are like the man from DelMonte who says 'yes' each time he is asked without reinstantiating the fruit (content)? And as this would first happen with 'line 45 of /theme/clean/layout/columns3.php: call to core_renderer->standard_head_html()' then no problem.

          Just thinking with my OO hat on along the lines of 'if a method that produces a result is called many times and the result once determined is always the same then that result should be stored'.

          Show
          Gareth J Barnard added a comment - - edited So could 'get_content_for_output($output)' in 'moodleblock.class.php' cache the result of it's 'formatted_contents($output)'? Or once instantiated they are like the man from DelMonte who says 'yes' each time he is asked without reinstantiating the fruit (content)? And as this would first happen with 'line 45 of /theme/clean/layout/columns3.php: call to core_renderer->standard_head_html()' then no problem. Just thinking with my OO hat on along the lines of 'if a method that produces a result is called many times and the result once determined is always the same then that result should be stored'.
          Hide
          Andrew Nicols added a comment -

          Possibly Gareth J Barnard, but not always. Remember, storing a value takes up memory. It is often cheaper to re-fetch than it is to store. I don't know in this case.
          But for this case, I agree it feels wrong that we run through the whole stack just to retrieve a list of block names for things which are already created. I do suspect that there's an improvement to be had here but I am going to pass the buck to Sam Hemelryk as he is certainly more familiar with the lay of the land.

          Show
          Andrew Nicols added a comment - Possibly Gareth J Barnard , but not always. Remember, storing a value takes up memory. It is often cheaper to re-fetch than it is to store. I don't know in this case. But for this case, I agree it feels wrong that we run through the whole stack just to retrieve a list of block names for things which are already created. I do suspect that there's an improvement to be had here but I am going to pass the buck to Sam Hemelryk as he is certainly more familiar with the lay of the land.
          Hide
          Paul P added a comment - - edited

          Confirming that the patch works - thanks!

          Unfortunately, I cannot offer any insight on the memory/performance question.

          On a side note, it may be a good idea to bring this to the attention of other third-party theme designers who developed themes based on Clean and Bootstrapbase.

          Show
          Paul P added a comment - - edited Confirming that the patch works - thanks! Unfortunately, I cannot offer any insight on the memory/performance question. On a side note, it may be a good idea to bring this to the attention of other third-party theme designers who developed themes based on Clean and Bootstrapbase.
          Hide
          Petr Škoda added a comment -

          +1 for the patch, submitting for integration, thanks everybody

          Show
          Petr Škoda added a comment - +1 for the patch, submitting for integration, thanks everybody
          Hide
          Andrew Nicols added a comment -

          Just to note, we do cache get_regions, and ensure_content_exists().
          This isn't actually a caching issue.

          When body_attributes() is called, that's the first time that we instantiate the blocks.
          Other theme's have a call to $PAGE->blocks->region_has_content() at the top of the layout which instantiates the blocks. The output is cached as can been by commenting out the body_attributes() call in a clean layout - this just moves the issue down to the next time something which uses the blocks is loaded (in this case a call to $OUTPUT->blocks().

          Perhaps what we need is a function to setup the page so that we can keep the body_attributes() in a sane place?

          $OUTPUT->setup_layout_content() for example:

              public function setup_layout_content() {
                  foreach ($this->page->blocks->get_regions() as $region) {
                      $this->page->blocks->region_has_content($region, $this);
                  }
              }
          

          Would this be a more appropriate fix to this issue?

          Show
          Andrew Nicols added a comment - Just to note, we do cache get_regions, and ensure_content_exists(). This isn't actually a caching issue. When body_attributes() is called, that's the first time that we instantiate the blocks. Other theme's have a call to $PAGE->blocks->region_has_content() at the top of the layout which instantiates the blocks. The output is cached as can been by commenting out the body_attributes() call in a clean layout - this just moves the issue down to the next time something which uses the blocks is loaded (in this case a call to $OUTPUT->blocks(). Perhaps what we need is a function to setup the page so that we can keep the body_attributes() in a sane place? $OUTPUT->setup_layout_content() for example: public function setup_layout_content() { foreach ($ this ->page->blocks->get_regions() as $region) { $ this ->page->blocks->region_has_content($region, $ this ); } } Would this be a more appropriate fix to this issue?
          Hide
          Mary Evans added a comment - - edited

          @Andrew,
          Can I ask what the relevance of adding body class 'two-column' in the columns2.php is as this class selector is not used in any bootstrapbase CSS?

          Show
          Mary Evans added a comment - - edited @Andrew, Can I ask what the relevance of adding body class 'two-column' in the columns2.php is as this class selector is not used in any bootstrapbase CSS?
          Hide
          Mary Evans added a comment -

          Andrew, ignore that last question as I thought class 'two-column' was something added here, however, checkout Moodle GIT logs I see it was added by Sam when the 'body attributes' where added and changes made to introduce the new style layouts.

          Even so, I don't really understand why it's there at all, as the bodyclass selector 'two-column' is not use anywhere in bootstrapbase LESS/CSS, unless I missed it.

          However, if it was Sam's intention to use this bodyclass selector, then why only 'two-column', why not add 'one-column' and 'three-column' in columns1.php and columns3.php respectively?

          Show
          Mary Evans added a comment - Andrew, ignore that last question as I thought class 'two-column' was something added here, however, checkout Moodle GIT logs I see it was added by Sam when the 'body attributes' where added and changes made to introduce the new style layouts. Even so, I don't really understand why it's there at all, as the bodyclass selector 'two-column' is not use anywhere in bootstrapbase LESS/CSS, unless I missed it. However, if it was Sam's intention to use this bodyclass selector, then why only 'two-column', why not add 'one-column' and 'three-column' in columns1.php and columns3.php respectively?
          Hide
          Gareth J Barnard added a comment -

          Or even at this rate a 'nelsons-column'

          Show
          Gareth J Barnard added a comment - Or even at this rate a 'nelsons-column'
          Hide
          Andrew Nicols added a comment -

          I must admit Mary Evans that I'm not sure. I just carried the setting forward.

          Show
          Andrew Nicols added a comment - I must admit Mary Evans that I'm not sure. I just carried the setting forward.
          Hide
          Mary Evans added a comment -

          Thanks for your reply Andrew. At least now it is in the correct place, and the jQuery works in Clean/Bootstrap(Base) themes for blocks and other stuff, hopefully. So I guess the two-column class is just a reference point for designers to add further classes as and when they wish, which was my initial thought with regards adding the other columns (ignoring Gareth's tongue in cheek comment of course!).

          Show
          Mary Evans added a comment - Thanks for your reply Andrew. At least now it is in the correct place, and the jQuery works in Clean/Bootstrap(Base) themes for blocks and other stuff, hopefully. So I guess the two-column class is just a reference point for designers to add further classes as and when they wish, which was my initial thought with regards adding the other columns (ignoring Gareth's tongue in cheek comment of course!).
          Hide
          Andrew Nicols added a comment -

          Oops - forgot to add the links to the branches I wrote this morning for 2.5.
          They're the same as the master branch and I've checked that they work as expected.

          Show
          Andrew Nicols added a comment - Oops - forgot to add the links to the branches I wrote this morning for 2.5. They're the same as the master branch and I've checked that they work as expected.
          Hide
          Sam Hemelryk added a comment -

          Hmmmmm, I'm on the fence for the solution here.

          Its gets a +1 from in that is solves the immediate issue (good research here Andrew, that's a real prick of an error), however it gets a -0.5 as it solves it for this one specific scenario only (body_attributes being called before standard_head_html).

          Looking at what is actually going wrong here it appears the issue centres around blocks not having being initialised before standard_head_html locks the JS/CSS that can be included in the head of a page.
          No doubt this has arisen from the requirement to include jQuery within the head of the page, and has gone safety unnoticed until now because of the dumb luck (only because of the order in which a theme layout operates).
          I didn't follow the process behind deciding how jQuery would be included but its safe to assume it is included in the head for good reason.
          Interestingly enough the same error could be triggered by a block attempting to load any JS into the head, or through a block attempting to include CSS within the page via the requirements manager (an issue I have encountered before but never addressed).
          Perhaps a better solution to this issue would be ensure that blocks get initialised before the opportunity to include JS/CSS in the head has been locked out.
          To me that sounds like a solution that would solve not only the bug at hand but also bugs closely related bugs to do with the order of calls within a theme layout, no doubt that would avoid this situation which would be extremely frustrating for anyone who wasn't familiar with our block page initialisation routines.

          So... having written my thoughts as they come to me I think this gets a -1 from me in favour of the approach I've discussed, on the basis that it is a more thorough solution.

          Show
          Sam Hemelryk added a comment - Hmmmmm, I'm on the fence for the solution here. Its gets a +1 from in that is solves the immediate issue (good research here Andrew, that's a real prick of an error), however it gets a -0.5 as it solves it for this one specific scenario only (body_attributes being called before standard_head_html). Looking at what is actually going wrong here it appears the issue centres around blocks not having being initialised before standard_head_html locks the JS/CSS that can be included in the head of a page. No doubt this has arisen from the requirement to include jQuery within the head of the page, and has gone safety unnoticed until now because of the dumb luck (only because of the order in which a theme layout operates). I didn't follow the process behind deciding how jQuery would be included but its safe to assume it is included in the head for good reason. Interestingly enough the same error could be triggered by a block attempting to load any JS into the head, or through a block attempting to include CSS within the page via the requirements manager (an issue I have encountered before but never addressed). Perhaps a better solution to this issue would be ensure that blocks get initialised before the opportunity to include JS/CSS in the head has been locked out. To me that sounds like a solution that would solve not only the bug at hand but also bugs closely related bugs to do with the order of calls within a theme layout, no doubt that would avoid this situation which would be extremely frustrating for anyone who wasn't familiar with our block page initialisation routines. So... having written my thoughts as they come to me I think this gets a -1 from me in favour of the approach I've discussed, on the basis that it is a more thorough solution.
          Show
          Andrew Nicols added a comment - So is that the suggestion I made in https://tracker.moodle.org/browse/MDL-41516?focusedCommentId=241867&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-241867 or something slightly different?
          Hide
          Sam Hemelryk added a comment -

          Mary - The two-column class was added originally as it was required for positioning of the block region in situations where it was expected to be on the other side of the page (rtl). Since I made these changes someone has made further changes improving that area and removing the need for that class.
          No doubt it was left there in case someone has already used it in a custom theme.
          I'm sure it could be removed if required, or equivalent classes added to the 1/3 column layouts for consistency reasons.

          Show
          Sam Hemelryk added a comment - Mary - The two-column class was added originally as it was required for positioning of the block region in situations where it was expected to be on the other side of the page (rtl). Since I made these changes someone has made further changes improving that area and removing the need for that class. No doubt it was left there in case someone has already used it in a custom theme. I'm sure it could be removed if required, or equivalent classes added to the 1/3 column layouts for consistency reasons.
          Hide
          Andrew Nicols added a comment -

          Putting this up for peer review again following Sam's suggestions and a quick chat.

          Rather than writing a new function which must be called from the top of each layout to do this, I've moved it to standard_head_html(). I've also changed the visibility of block_manager::ensure_content_created() as this is the function we really want to call anyway and this makes much more sense.

          This will make writing layouts easier than my previous suggestion as you will just be able to use the body_attributes() function in the most obvious place.

          I've force-pushed the branches.

          Testing instructions remain the same.

          Putting up for peer review again since this patch has changed quite drastically - Sam Hemelryk has already offered to review.

          Show
          Andrew Nicols added a comment - Putting this up for peer review again following Sam's suggestions and a quick chat. Rather than writing a new function which must be called from the top of each layout to do this, I've moved it to standard_head_html(). I've also changed the visibility of block_manager::ensure_content_created() as this is the function we really want to call anyway and this makes much more sense. This will make writing layouts easier than my previous suggestion as you will just be able to use the body_attributes() function in the most obvious place. I've force-pushed the branches. Testing instructions remain the same. Putting up for peer review again since this patch has changed quite drastically - Sam Hemelryk has already offered to review.
          Hide
          Mary Evans added a comment -

          I thought it would be something like that. I think it should be removed in that case as the likelihood of someone using that class it pretty slim. And as with all custom themes it can be fixed at source.

          Show
          Mary Evans added a comment - I thought it would be something like that. I think it should be removed in that case as the likelihood of someone using that class it pretty slim. And as with all custom themes it can be fixed at source.
          Hide
          Sam Hemelryk added a comment -

          Looks spot on thanks Andrew - I've put this up for integration review now.

          Show
          Sam Hemelryk added a comment - Looks spot on thanks Andrew - I've put this up for integration review now.
          Hide
          Damyon Wiese added a comment -

          Thanks for the fix Andrew,

          Looks good to me. Integrated to master and 25.

          Show
          Damyon Wiese added a comment - Thanks for the fix Andrew, Looks good to me. Integrated to master and 25.
          Hide
          Paul P added a comment -

          FWIW I tested it and it behaves correctly. Thanks again!

          Show
          Paul P added a comment - FWIW I tested it and it behaves correctly. Thanks again!
          Hide
          Gareth J Barnard added a comment -

          Awesome looking patch Andrew Nicols, thank you

          Show
          Gareth J Barnard added a comment - Awesome looking patch Andrew Nicols , thank you
          Hide
          Mary Evans added a comment -

          Nice and neat patch Andrew.
          Thanks for all the hard work.

          Show
          Mary Evans added a comment - Nice and neat patch Andrew. Thanks for all the hard work.
          Hide
          Jason Fowler added a comment -

          All good Andrew, thanks for the patch

          Show
          Jason Fowler added a comment - All good Andrew, thanks for the patch
          Hide
          Dan Poltawski added a comment -

          Congratulations! This change has been integrated upstream and is now available from our git and download mirrors. To celebrate, here is a joke:

          A SQL query goes into a bar, walks up to two tables and asks, "Can I join you?"

          Show
          Dan Poltawski added a comment - Congratulations! This change has been integrated upstream and is now available from our git and download mirrors. To celebrate, here is a joke: A SQL query goes into a bar, walks up to two tables and asks, "Can I join you?"

            People

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

              Dates

              • Created:
                Updated:
                Resolved: