Moodle
  1. Moodle
  2. MDL-41398

Clean theme needs the upgrade page to be dumb

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Blocker Blocker
    • Resolution: Fixed
    • Affects Version/s: 2.6
    • Fix Version/s: 2.5.3, 2.6
    • Component/s: Course, Themes
    • Labels:
    • Testing Instructions:
      Hide

      Add the following to your config file:
      <code>
      $CFG->theme = 'clean';
      </code>

      Using your browser:

      1. Perform a fresh installation.
      2. Upgrade a 24 site to 25 and then master.
      3. While the upgrade is running using another browser attempt to browse to the site. Check the page is displayed correctly.
      4. Put the site into maintenance mode and check again using another browser.

      Using the command line interface:

      1. Install a fresh site.
      2. Upgrade a 24 site to 25 and then to master.
      3. During the upgrade using a browser visit the site.
      Show
      Add the following to your config file: <code> $CFG->theme = 'clean'; </code> Using your browser: Perform a fresh installation. Upgrade a 24 site to 25 and then master. While the upgrade is running using another browser attempt to browse to the site. Check the page is displayed correctly. Put the site into maintenance mode and check again using another browser. Using the command line interface: Install a fresh site. Upgrade a 24 site to 25 and then to master. During the upgrade using a browser visit the site.
    • Workaround:
      Hide

      Put

      $CFG->theme = 'standard';

      In your config.php file, until the upgrade has completed.

      Show
      Put $CFG->theme = 'standard'; In your config.php file, until the upgrade has completed.
    • Affected Branches:
      MOODLE_26_STABLE
    • Fixed Branches:
      MOODLE_25_STABLE, MOODLE_26_STABLE
    • Pull 2.5 Branch:
      41398-25-2
    • Pull Master Branch:
      41398-26-2
    • Story Points:
      8
    • Rank:
      52960
    • Sprint:
      FRONTEND Sprint 5

      Description

      1. Install Moodle 2.2
      2. Set in config.php: $CFG->theme = 'clean';
      3. Attempt to upgrade to Moodle 2.6:

      On /admin/, it all goes pete tong and you can't move forward:

      [Fri Aug 23 11:47:27 2013] [error] [client 127.0.0.1] Default exception handler: Table "course_format_options" does not exist 
      
      Debug: 
      Error code: ddltablenotexist
      * line 567 of /lib/dml/moodle_database.php: dml_exception thrown
      * line 1189 of /lib/dml/moodle_database.php: call to moodle_database->where_clause()
      * line 582 of /course/format/lib.php: call to moodle_database->get_records()
      * line 239 of /course/format/lib.php: call to format_base->get_format_options()
      * line 419 of /course/format/lib.php: call to format_base->get_course()
      * line 1788 of /lib/navigationlib.php: call to format_base->extend_course_navigation()
      * line 1093 of /lib/navigationlib.php: call to global_navigation->load_course_sections()
      * line 2977 of /lib/navigationlib.php: call to global_navigation->initialise()
      * line 3020 of /lib/navigationlib.php: call to navbar->has_items()
      * line 55 of /theme/bootstrapbase/renderers/core_renderer.php: call to navbar->get_items()
      * line 57 of /theme/clean/layout/columns1.php: call to theme_bootstrapbase_core_renderer->navbar()
      * line 847 of /lib/outputrenderers.php: call to include()
      * line 777 of /lib/outputrenderers.php: call to core_renderer->render_page_layout()
      * line ? of unknownfile: call to core_renderer->header()
      * line 234 of /lib/outputrenderers.php: call to call_user_func_array()
      * line 146 of /admin/renderer.php: call to plugin_renderer_base->__call()
      * line 146 of /admin/renderer.php: call to core_admin_renderer->header()
      * line 264 of /admin/index.php: call to core_admin_renderer->upgrade_confirm_page()
      , referer: http://dan.moodle.local/m/pm/
      

        Issue Links

          Activity

          Hide
          Marina Glancy added a comment -

          It looks like Clean theme uses clean/layout/columns1.php as layout for admin/index.php and tries to display navigation bar before the core is upgraded

          Show
          Marina Glancy added a comment - It looks like Clean theme uses clean/layout/columns1.php as layout for admin/index.php and tries to display navigation bar before the core is upgraded
          Hide
          Dan Poltawski added a comment -

          Here is another instance of it:
          https://gist.github.com/danpoltawski/6504987

          Show
          Dan Poltawski added a comment - Here is another instance of it: https://gist.github.com/danpoltawski/6504987
          Hide
          Marina Glancy added a comment - - edited

          'maintenance' layout has option 'nonavbar'=>true
          but it is not respected in clean/layout/columns1.php

          The following files need to be corrected:
          clean/layout/columns1.php
          clean/layout/columns2.php
          clean/layout/columns3.php
          bootstrapbase/layout/columns1.php
          bootstrapbase/layout/columns2.php
          bootstrapbase/layout/columns3.php

          Other layouts do not display navigation bar at all so there is no problems there.

          I attached the branch where I fix it quite primitive way. I would really like some Frontend people and especially Bootstrap guru to look at it and decide how to display html if navbar is not to be displayed

          Show
          Marina Glancy added a comment - - edited 'maintenance' layout has option 'nonavbar'=>true but it is not respected in clean/layout/columns1.php The following files need to be corrected: clean/layout/columns1.php clean/layout/columns2.php clean/layout/columns3.php bootstrapbase/layout/columns1.php bootstrapbase/layout/columns2.php bootstrapbase/layout/columns3.php Other layouts do not display navigation bar at all so there is no problems there. I attached the branch where I fix it quite primitive way. I would really like some Frontend people and especially Bootstrap guru to look at it and decide how to display html if navbar is not to be displayed
          Hide
          Marina Glancy added a comment -

          PS this also happens if you have 2.5 site, set theme to Clean and upgrade to 2.6

          Show
          Marina Glancy added a comment - PS this also happens if you have 2.5 site, set theme to Clean and upgrade to 2.6
          Hide
          Dan Poltawski added a comment -

          Note: I think a fix for this should be backported because its possible the 2.5 upgrade could lead to situations like this too.

          Show
          Dan Poltawski added a comment - Note: I think a fix for this should be backported because its possible the 2.5 upgrade could lead to situations like this too.
          Hide
          Marina Glancy added a comment -

          I'm afraid it also displays navbar in quizzes where it is not supposed to be

          Show
          Marina Glancy added a comment - I'm afraid it also displays navbar in quizzes where it is not supposed to be
          Hide
          Mary Evans added a comment -

          Bootstrap GURU is there sure a person? LOL

          Can someone check Moodle standard theme layouts as I am finding more and more that the layout options do not seem to be working as they should.

          The navbar issus with Bootstrap themes is unfotunate as the there are two. The one at the top of the screen and the navbar/breadcrumb, which is the one that should not be there but is.

          David knows more about the whys and wherefores of the layout better than I do. I tend to sit in the background and just fix the bugs as they present themselves. But I think the navbar naming is something else and needs fixing better than it is at present.

          Show
          Mary Evans added a comment - Bootstrap GURU is there sure a person? LOL Can someone check Moodle standard theme layouts as I am finding more and more that the layout options do not seem to be working as they should. The navbar issus with Bootstrap themes is unfotunate as the there are two. The one at the top of the screen and the navbar/breadcrumb, which is the one that should not be there but is. David knows more about the whys and wherefores of the layout better than I do. I tend to sit in the background and just fix the bugs as they present themselves. But I think the navbar naming is something else and needs fixing better than it is at present.
          Hide
          Mary Evans added a comment -

          I see that David Scotson is also watching this. David can you give us your view on this please?

          Show
          Mary Evans added a comment - I see that David Scotson is also watching this. David can you give us your view on this please?
          Hide
          Marina Glancy added a comment - - edited

          You know what, I'm marking it as a "could be security issue", we will discuss tomorrow whether to keep it so or not.

          yes Mary, you are right. Clean/bootstrapbase also ignores nofooter, nocustommenu, nocourseheader/footer/headerfooter,
          also it does not even define in config.php such options as noblocks, nologininfo and nologinlinks

          This piece in core.less just killed me - all data is being hidden with CSS:

          .layout-option-noheader #page-header,
          .layout-option-nonavbar #page-navbar,
          .layout-option-nofooter #page-footer,
          .layout-option-nocourseheader .course-content-header,
          .layout-option-nocoursefooter .course-content-footer {
              display:none;
          }
          

          which means that everything is still present on the page! Except upgrade problems that occur when navigation bar is being rendered it also means that in quiz popup all this information is also present and little acquaintance with firebug makes it accessible to students. Also in maintenance mode students can access the page footer and if admin enabled some debuging/perfinfo display - it is there.

          PS Dan, you were wondering why you did not see error messages on /admin/index.php and it was displayed blank for you. Here is the answer - it was inside the navbar block and therefore hidden with CSS

          Show
          Marina Glancy added a comment - - edited You know what, I'm marking it as a "could be security issue", we will discuss tomorrow whether to keep it so or not. yes Mary, you are right. Clean/bootstrapbase also ignores nofooter, nocustommenu, nocourseheader/footer/headerfooter, also it does not even define in config.php such options as noblocks, nologininfo and nologinlinks This piece in core.less just killed me - all data is being hidden with CSS: .layout-option-noheader #page-header, .layout-option-nonavbar #page-navbar, .layout-option-nofooter #page-footer, .layout-option-nocourseheader .course-content-header, .layout-option-nocoursefooter .course-content-footer { display:none; } which means that everything is still present on the page! Except upgrade problems that occur when navigation bar is being rendered it also means that in quiz popup all this information is also present and little acquaintance with firebug makes it accessible to students. Also in maintenance mode students can access the page footer and if admin enabled some debuging/perfinfo display - it is there. PS Dan, you were wondering why you did not see error messages on /admin/index.php and it was displayed blank for you. Here is the answer - it was inside the navbar block and therefore hidden with CSS
          Hide
          Mary Evans added a comment -

          I cannot answer for David, but I suspect there is reasoning behind adding that CSS is core.less

          I changed bootstrapbase/config.php about a couple of months ago, to match up with Base theme's config.php so that they are identical other than the file names. MDL-39697

          Show
          Mary Evans added a comment - I cannot answer for David, but I suspect there is reasoning behind adding that CSS is core.less I changed bootstrapbase/config.php about a couple of months ago, to match up with Base theme's config.php so that they are identical other than the file names. MDL-39697
          Hide
          Sam Hemelryk added a comment -

          Hehe fun issue.

          Just noting that quiz uses the secure pagelayout, and through bootstrapbase the secure layout file which does not contain any of this stuff.
          I think it is safe to remove the security level here.

          I introduced that CSS by the way, and as part of that commit removing all logic from those themes.
          I think this case safely be considered a regression from those changes.
          There are two solutions as I see it:

          1. Bring back the logic for the header/footer options.
          2. Switch the layout definition to use a more appropriate layout file and remove the options completely.
          Show
          Sam Hemelryk added a comment - Hehe fun issue. Just noting that quiz uses the secure pagelayout, and through bootstrapbase the secure layout file which does not contain any of this stuff. I think it is safe to remove the security level here. I introduced that CSS by the way, and as part of that commit removing all logic from those themes. I think this case safely be considered a regression from those changes. There are two solutions as I see it: Bring back the logic for the header/footer options. Switch the layout definition to use a more appropriate layout file and remove the options completely.
          Hide
          Mary Evans added a comment -

          +1 for No.2

          Show
          Mary Evans added a comment - +1 for No.2
          Hide
          Marina Glancy added a comment -

          I know that no2 is easier for theme developers because there is less PHP code there. But it means more layout pages and more chances that we fix (later on) something in one of them and forget another.

          Btw since bootstrapbase overwrites core_renderer anyway, do you think we can include verifying of the options in functions displaying the navbar and footer? Just to double check

          Show
          Marina Glancy added a comment - I know that no2 is easier for theme developers because there is less PHP code there. But it means more layout pages and more chances that we fix (later on) something in one of them and forget another. Btw since bootstrapbase overwrites core_renderer anyway, do you think we can include verifying of the options in functions displaying the navbar and footer? Just to double check
          Hide
          Martin Dougiamas added a comment -

          I've bumped this up into next sprint for FRONTEND ... Sam can you look at this?

          Show
          Martin Dougiamas added a comment - I've bumped this up into next sprint for FRONTEND ... Sam can you look at this?
          Hide
          Martin Dougiamas added a comment -

          Obviously we need to reduce further regressions to other themes to a minimum. If this can not be zero, please make this super clear.

          Show
          Martin Dougiamas added a comment - Obviously we need to reduce further regressions to other themes to a minimum. If this can not be zero, please make this super clear.
          Hide
          Dan Poltawski added a comment -

          I imagine third party themes already have this problem (especially that popular one with the menu on the top). It seems to me the safest way is to make this upgrade process theme indepedent.

          Show
          Dan Poltawski added a comment - I imagine third party themes already have this problem (especially that popular one with the menu on the top). It seems to me the safest way is to make this upgrade process theme indepedent.
          Hide
          Sam Hemelryk added a comment -

          I've created a branch that properly implements the maintenance layout. This would be option #2 that I noted above and the bare minimum fix.

          repo: git://github.com/samhemelryk/moodle.git
          branch: 41398-26
          diff: https://github.com/samhemelryk/moodle/commit/41398-26

          This fixes the immediate issue at hand but doesn't do anything to prevent this same issue from arising for another theme should a developer carelessly introduce an API call not supported during upgrade.
          As Dan mentioned the best way to prevent this would be to make upgrade theme independent. However I feel while that is the most thorough solution it is perhaps a little anti-progressive and over bearing as really we should be able to support themes during upgrade through better management.
          One thought that occurs to me is to provide a "maintenance" version of the core renderer as $OUTPUT that invisibly disregards requests to OUTPUT methods that require API's not available during upgrade. After playing with this for half an hour there are more intricacies that I envisaged and I think the hopes of such a solution succeeding are fading. That said its late here and perhaps something will come to me overnight so I will present the above solution for the time being and continue to ponder renderers and preventing access to unavailable API's during upgrade.

          Many thanks
          Sam

          Show
          Sam Hemelryk added a comment - I've created a branch that properly implements the maintenance layout. This would be option #2 that I noted above and the bare minimum fix. repo: git://github.com/samhemelryk/moodle.git branch: 41398-26 diff: https://github.com/samhemelryk/moodle/commit/41398-26 This fixes the immediate issue at hand but doesn't do anything to prevent this same issue from arising for another theme should a developer carelessly introduce an API call not supported during upgrade. As Dan mentioned the best way to prevent this would be to make upgrade theme independent. However I feel while that is the most thorough solution it is perhaps a little anti-progressive and over bearing as really we should be able to support themes during upgrade through better management. One thought that occurs to me is to provide a "maintenance" version of the core renderer as $OUTPUT that invisibly disregards requests to OUTPUT methods that require API's not available during upgrade. After playing with this for half an hour there are more intricacies that I envisaged and I think the hopes of such a solution succeeding are fading. That said its late here and perhaps something will come to me overnight so I will present the above solution for the time being and continue to ponder renderers and preventing access to unavailable API's during upgrade. Many thanks Sam
          Hide
          Marina Glancy added a comment -

          that's a nice solution - it fixes all clones of Clean theme as well. Should not we suppress the footer as well in maintenance?

          Show
          Marina Glancy added a comment - that's a nice solution - it fixes all clones of Clean theme as well. Should not we suppress the footer as well in maintenance?
          Hide
          Marina Glancy added a comment -

          I remove my branch from the issue, it is not a good solution anyway. For reference it is here
          https://github.com/marinaglancy/moodle/compare/moodle:master...wip-MDL-41398-master

          Show
          Marina Glancy added a comment - I remove my branch from the issue, it is not a good solution anyway. For reference it is here https://github.com/marinaglancy/moodle/compare/moodle:master...wip-MDL-41398-master
          Hide
          Mary Evans added a comment -

          There should be no homelink in footer but I think it would be wise to have the empty footer as you can see the debugging info there if need be.

          Show
          Mary Evans added a comment - There should be no homelink in footer but I think it would be wise to have the empty footer as you can see the debugging info there if need be.
          Hide
          Marina Glancy added a comment -

          I'd say this is the exact reason why footer should NOT be there - students/guests are not supposed to see debugging or other page information when site is in mainenance mode

          Show
          Marina Glancy added a comment - I'd say this is the exact reason why footer should NOT be there - students/guests are not supposed to see debugging or other page information when site is in mainenance mode
          Hide
          Petr Škoda added a comment -

          +1 for separate maintenance page, there cannot be any links or system API calls that result in DB reads there. Ideally there should not be even any login or user profile link.

          Show
          Petr Škoda added a comment - +1 for separate maintenance page, there cannot be any links or system API calls that result in DB reads there. Ideally there should not be even any login or user profile link.
          Hide
          David Scotson added a comment -

          Since Mary mentioned my name earlier, I should point out that I didn't have anything to do with the layout rewrite, so I've not got much to contribute here. I like Sam's #2 fix idea though, seems cleanest.

          Someone mentioned hard-coded themes (or being "theme independant") for upgrades, and I've previously suggested that some of the admin parts of Moodle could be simplified if they used a single theme (I'd suggest a variation on Clean), and this has various benefits e.g. you don't need to send that specialised admin CSS to every single student user of the site. The main thing stopping that is that, unlike say Wordpress, Moodle has no hard line between the functional "backstage" area and the branded user facing areas. But upgrade and install and probably a few other areas clearly qualify so you could probably do it in those clearly defined areas. (Install currently hard-codes some subset of CSS to use from Base and Standard I believe).

          Show
          David Scotson added a comment - Since Mary mentioned my name earlier, I should point out that I didn't have anything to do with the layout rewrite, so I've not got much to contribute here. I like Sam's #2 fix idea though, seems cleanest. Someone mentioned hard-coded themes (or being "theme independant") for upgrades, and I've previously suggested that some of the admin parts of Moodle could be simplified if they used a single theme (I'd suggest a variation on Clean), and this has various benefits e.g. you don't need to send that specialised admin CSS to every single student user of the site. The main thing stopping that is that, unlike say Wordpress, Moodle has no hard line between the functional "backstage" area and the branded user facing areas. But upgrade and install and probably a few other areas clearly qualify so you could probably do it in those clearly defined areas. (Install currently hard-codes some subset of CSS to use from Base and Standard I believe).
          Hide
          Mary Evans added a comment -

          @David, I was meaning the CSS not the layouts.

          @Marina, sorry I forgot other people (Guests and Users) can see the maintenance page too, I thought only I saw it.

          Show
          Mary Evans added a comment - @David, I was meaning the CSS not the layouts. @Marina, sorry I forgot other people (Guests and Users) can see the maintenance page too, I thought only I saw it.
          Hide
          Sam Hemelryk added a comment -

          Ok I've simplified the footer, removing the home and docs links.
          The standard footer HTML is still set to be displayed there however I think that is acceptable as it has always been designed to work for these pages.

          I've also managed to inject a maintenance renderer in place of the core_renderer which is used within theme layout files as $OUTPUT.
          The maintenance renderer extends the core_renderer but calls to API methods that would cause issues when in "maintenance" mode will simply return empty strings without triggering the API's that would lead to the issues.
          Accompanying this I've got a couple of unit tests although automatic testing is rather week here still as we don't currently test the theme output processes. Testing will need to be very thorough.

          Presently I am still testing this myself, I've shared the code here now however so that others can check it out but please be aware it is still liable to change if I encounter any edge cases.

          Also worth noting that discussion here is leading towards the idea of better separating this code.
          I completely 100% agree, separation is long overdue, especially for admin/index.php which serves several purposes presently.
          However such an task would definitely be better as a separate issue, and as it would be master only and I require a 25 fix for this undoable here.
          I'll have a search and see if there is already an issue for this and if not, create one.

          Many thanks
          Sam

          Show
          Sam Hemelryk added a comment - Ok I've simplified the footer, removing the home and docs links. The standard footer HTML is still set to be displayed there however I think that is acceptable as it has always been designed to work for these pages. I've also managed to inject a maintenance renderer in place of the core_renderer which is used within theme layout files as $OUTPUT. The maintenance renderer extends the core_renderer but calls to API methods that would cause issues when in "maintenance" mode will simply return empty strings without triggering the API's that would lead to the issues. Accompanying this I've got a couple of unit tests although automatic testing is rather week here still as we don't currently test the theme output processes. Testing will need to be very thorough. Presently I am still testing this myself, I've shared the code here now however so that others can check it out but please be aware it is still liable to change if I encounter any edge cases. Also worth noting that discussion here is leading towards the idea of better separating this code. I completely 100% agree, separation is long overdue, especially for admin/index.php which serves several purposes presently. However such an task would definitely be better as a separate issue, and as it would be master only and I require a 25 fix for this undoable here. I'll have a search and see if there is already an issue for this and if not, create one. Many thanks Sam
          Hide
          Mary Evans added a comment -

          Considering that this is now critical as Clean theme breaks a new installation and also screws up an upgrade, I just cannot see why Clean theme needs extra treatment just to make it do what all the other themes do?

          All I can think, at this moment in time, after spending a very long time last night trying to set up a new installation of Moodle 2.6 Dev on a laptop, and finding that Clean theme is now the center of a MDL-41780 by not only breaking an upgrade but totally screwing up a new installation.

          I just think Clean is getting more and more complex where there is really no need. It uses a simple layout, it uses simple logic, why create more renderers and then still block all the elements with CSS, this just sounds too crazy to me.

          Show
          Mary Evans added a comment - Considering that this is now critical as Clean theme breaks a new installation and also screws up an upgrade, I just cannot see why Clean theme needs extra treatment just to make it do what all the other themes do? All I can think, at this moment in time, after spending a very long time last night trying to set up a new installation of Moodle 2.6 Dev on a laptop, and finding that Clean theme is now the center of a MDL-41780 by not only breaking an upgrade but totally screwing up a new installation. I just think Clean is getting more and more complex where there is really no need. It uses a simple layout, it uses simple logic, why create more renderers and then still block all the elements with CSS, this just sounds too crazy to me.
          Hide
          Jason Fowler added a comment -

          will peer review this first thing in the morning Sam.

          Show
          Jason Fowler added a comment - will peer review this first thing in the morning Sam.
          Hide
          Jason Fowler added a comment -

          Code looks fine, just not 100% sure about all the "Computer says no..." comments though. Funny, but not really meaningful.

          Show
          Jason Fowler added a comment - Code looks fine, just not 100% sure about all the "Computer says no..." comments though. Funny, but not really meaningful.
          Hide
          Sam Hemelryk added a comment -

          Thanks Jason - putting this up for integration review now.

          Show
          Sam Hemelryk added a comment - Thanks Jason - putting this up for integration review now.
          Hide
          Sam Hemelryk added a comment -

          After talking with Marina I'm going to split the new maintenance renderer off into a separate issue MDL-42057 so that we can explore it fully.

          Show
          Sam Hemelryk added a comment - After talking with Marina I'm going to split the new maintenance renderer off into a separate issue MDL-42057 so that we can explore it fully.
          Hide
          Marina Glancy added a comment -

          Sam, I've noticed that Clean theme overwrites all layouts from bootstrapbase and replaces the heading with the custom logo if it is specified in settings. This means that after your change the custom logo will not be displayed in Clean theme on maintenance page.
          Please tell me if this is ok or you want to add a layout file for clean as well.

          We could mention new layout file in theme/bootstrapbase/upgrade.txt (introduced in 2.6 afaik)

          Also just to let you know that I will need to squash your commits anyway because they modify the same code

          Show
          Marina Glancy added a comment - Sam, I've noticed that Clean theme overwrites all layouts from bootstrapbase and replaces the heading with the custom logo if it is specified in settings. This means that after your change the custom logo will not be displayed in Clean theme on maintenance page. Please tell me if this is ok or you want to add a layout file for clean as well. We could mention new layout file in theme/bootstrapbase/upgrade.txt (introduced in 2.6 afaik) Also just to let you know that I will need to squash your commits anyway because they modify the same code
          Hide
          Sam Hemelryk added a comment -

          Hi Marina,

          I've pushed up a couple of new branches that add the maintenance layout to clean.
          There is only two lines difference so nice and easy introduction. Not too sure whether its a good idea but I've tested and it performs perfectly.
          I've also noted the new layout file in the bootstrap upgrade.txt file. I didn't worry about that for the clean theme as I don't feel its needed there.
          All squashed to a single commit as well.

          Cheers
          Sam

          Show
          Sam Hemelryk added a comment - Hi Marina, I've pushed up a couple of new branches that add the maintenance layout to clean. There is only two lines difference so nice and easy introduction. Not too sure whether its a good idea but I've tested and it performs perfectly. I've also noted the new layout file in the bootstrap upgrade.txt file. I didn't worry about that for the clean theme as I don't feel its needed there. All squashed to a single commit as well. Cheers Sam
          Hide
          Marina Glancy added a comment -

          Thanks Sam, integrated in 2.5 and master. I changed the upgrade.txt text a little during integration

          Show
          Marina Glancy added a comment - Thanks Sam, integrated in 2.5 and master. I changed the upgrade.txt text a little during integration
          Hide
          Andrew Nicols added a comment -

          At the final step, after changing the theme back to standard and setting:

              'maintenance' => array(
                  'file' => 'columns2.php',
                  'regions' => array('sire-pre'),
                  'defaultregion' => 'side-pre',
              ),
          

          I tried dropping my DB and creating a fresh database, then trying to complete a browser installation. I'm getting an error:

          Coding error detected, it must be fixed by a programmer: Trying to reference an unknown block region side-pre
          

          Attaching screenshot

          Show
          Andrew Nicols added a comment - At the final step, after changing the theme back to standard and setting: 'maintenance' => array( 'file' => 'columns2.php', 'regions' => array('sire-pre'), 'defaultregion' => 'side-pre', ), I tried dropping my DB and creating a fresh database, then trying to complete a browser installation. I'm getting an error: Coding error detected, it must be fixed by a programmer: Trying to reference an unknown block region side-pre Attaching screenshot
          Hide
          Andrew Nicols added a comment -

          All other tests passed without issue.

          Show
          Andrew Nicols added a comment - All other tests passed without issue.
          Hide
          Marina Glancy added a comment -

          oh sorry, I meant to comment about it and forgot. Testing instructions need to be corrected. During integration we decided to go with only half of original fix.

          Show
          Marina Glancy added a comment - oh sorry, I meant to comment about it and forgot. Testing instructions need to be corrected. During integration we decided to go with only half of original fix.
          Hide
          Marina Glancy added a comment -

          Andrew, I removed the last part of testing instructions. Checking in standard theme also makes no sense because only bootstrapbase/clean were modified.
          Sending back to testing

          Show
          Marina Glancy added a comment - Andrew, I removed the last part of testing instructions. Checking in standard theme also makes no sense because only bootstrapbase/clean were modified. Sending back to testing
          Hide
          Andrew Nicols added a comment -

          In which case this state can be reset and the testing passed then

          Show
          Andrew Nicols added a comment - In which case this state can be reset and the testing passed then
          Hide
          Andrew Nicols added a comment -

          Passing as discussed

          Show
          Andrew Nicols added a comment - Passing as discussed
          Hide
          David Scotson added a comment -

          Hi Andrew Nicols,

          Don't know if it's still relevant but your code you pasted a few comments up had a typo: sire-pre rather than side-pre.

          Show
          David Scotson added a comment - Hi Andrew Nicols , Don't know if it's still relevant but your code you pasted a few comments up had a typo: sire-pre rather than side-pre.
          Hide
          Marina Glancy added a comment -

          no, not relevant any more David, this part of testing instructions was removed because the commit adding a new maintenance renderer was not integrated.

          Show
          Marina Glancy added a comment - no, not relevant any more David, this part of testing instructions was removed because the commit adding a new maintenance renderer was not integrated.
          Hide
          Dan Poltawski added a comment -

          You did it!

          Thanks for your contribution, this change has been merged upstream and is now available on your local git mirror and on download sites shortly.

          Show
          Dan Poltawski added a comment - You did it! Thanks for your contribution, this change has been merged upstream and is now available on your local git mirror and on download sites shortly.
          Hide
          Marina Glancy added a comment -

          Sam, we were just having a discussion in devchat about another theme failing for the same reason and it fails in function navbar() that unfortunately was not overwritten in maintenance pagelayout: https://moodle.org/local/chatlogs/index.php?conversationid=14287

          Show
          Marina Glancy added a comment - Sam, we were just having a discussion in devchat about another theme failing for the same reason and it fails in function navbar() that unfortunately was not overwritten in maintenance pagelayout: https://moodle.org/local/chatlogs/index.php?conversationid=14287

            People

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

              Dates

              • Created:
                Updated:
                Resolved:

                Agile