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

Clean up all the themes shipping in 2.7 by removing some from core

    Details

    • Type: New Feature
    • Status: Closed
    • Priority: Blocker
    • Resolution: Fixed
    • Affects Version/s: 2.7
    • Fix Version/s: 2.7
    • Component/s: Themes
    • Labels:
    • Testing Instructions:
      Hide

      In the latest weekly

      Use a DIFFERENT theme from that list for each the following steps: afterburner, anomaly, arialist, binarius, boxxie, brick, formal_white, formfactor, fusion, leatherbound, magazine, nimble, nonzero, overlay, serenity, sky_high, splash, standard and standardold

      1. Using the theme selector, select a theme for
        • Default
        • Legacy
        • Tablet
        • Mobile
      2. Set a preferred theme for any user
      3. Force a theme in a category
      4. Force a theme in a course
      5. Force a theme for a MNet peer (Home ► Site administration ► Networking ► Peers)
      6. Configure some settings in a few themes (still in the list above) - Remember one of them (I'll call it theme_x)

      Checkout the latest integration, DO NOT UPGRADE yet

      1. Now that theme_x has disappeared from the themes directory, copy it back in.

      Upgrade now

      1. Run the upgrade
        • Make sure theme_x is not mentioned in the list of themes to be upgraded or deleted
        • Make sure the theme used during upgrade is Clean and not the one you set above
      2. Make sure the value has been set to clean (except if it was theme_x) in
        • The user's preferred theme
        • The forced theme for the course
        • The forced theme for the category
        • The forced theme for the MNet peer
      3. Make sure the theme selectors is not using any of the themes you had set previously (except for theme_x)
      4. Check the table mdl_config_plugins and make sure you do not find any entries for any of the themes listed above, except for theme_x.

      Install

      • Run a fresh install on the latest integration, make sure it works fine.

      Unit Tests

      • Run all the tests
      Show
      In the latest weekly Use a DIFFERENT theme from that list for each the following steps: afterburner, anomaly, arialist, binarius, boxxie, brick, formal_white, formfactor, fusion, leatherbound, magazine, nimble, nonzero, overlay, serenity, sky_high, splash, standard and standardold Using the theme selector, select a theme for Default Legacy Tablet Mobile Set a preferred theme for any user Force a theme in a category Force a theme in a course Force a theme for a MNet peer (Home ► Site administration ► Networking ► Peers) Configure some settings in a few themes (still in the list above) - Remember one of them (I'll call it theme_x) Checkout the latest integration, DO NOT UPGRADE yet Now that theme_x has disappeared from the themes directory, copy it back in. Upgrade now Run the upgrade Make sure theme_x is not mentioned in the list of themes to be upgraded or deleted Make sure the theme used during upgrade is Clean and not the one you set above Make sure the value has been set to clean (except if it was theme_x) in The user's preferred theme The forced theme for the course The forced theme for the category The forced theme for the MNet peer Make sure the theme selectors is not using any of the themes you had set previously (except for theme_x) Check the table mdl_config_plugins and make sure you do not find any entries for any of the themes listed above, except for theme_x. Install Run a fresh install on the latest integration, make sure it works fine. Unit Tests Run all the tests
    • Affected Branches:
      MOODLE_27_STABLE
    • Fixed Branches:
      MOODLE_27_STABLE
    • Pull from Repository:
    • Pull Master Branch:
      MDL-43784-master
    • Sprint:
      FRONTEND Sprint 11
    • Story Points (Obsolete):
      20
    • Sprint:
      FRONTEND Sprint 11

      Description

      Moodle 2.7 contains two "base" themes that developers are maintaining:

      • base: the old deprecated base theme with minimal CSS to be functional.
      • bootstrapbase: the new responsive base theme with minimal CSS to be functional.

      As well as this theme that a lot of community themes use as a parent theme:

      • canvas: created by New School Learning but no longer maintained much

      None of the above themes are selectable for use as themes themselves.

      Moodle 2.7 will have two selectable themes based on bootstrapbase:

      • clean: as default, implementing almost pure bootstrapbase and to provide an clonable example of a basic theme
      • more: to allow basic admins to make simple tweaks to colours, images and CSS from UI without needing to touch files.

      Everything else should be removed and put into github.com as public projects (if anyone wants to maintain them) and moodle.org/plugins for download:

      • afterburner
      • anomaly
      • arialist
      • binarius
      • boxxie
      • brick
      • formal_white
      • formfactor
      • fusion
      • leatherbound
      • magazine
      • nimble
      • nonzero
      • overlay
      • serenity
      • sky_high
      • splash
      • standard
      • standardold

        Gliffy Diagrams

          Attachments

            Issue Links

              Activity

              Hide
              tmuras Tomasz Muras added a comment -

              +1 move some theme into the Moodle.org plugins repository
              +1 add a link to the Moodle.org plugins repository

              Show
              tmuras Tomasz Muras added a comment - +1 move some theme into the Moodle.org plugins repository +1 add a link to the Moodle.org plugins repository
              Hide
              quen Sam Marshall added a comment -

              +1 to remove all themes from the repository except base/standard and bootstrapbase/clean, might need an easy 'install theme' mechanism like there is for languages? (Not sure if this already exists.) The existing load of themes is a bit cluttered especially in the OU repository where we have several custom themes as well as the standard ones.

              Show
              quen Sam Marshall added a comment - +1 to remove all themes from the repository except base/standard and bootstrapbase/clean, might need an easy 'install theme' mechanism like there is for languages? (Not sure if this already exists.) The existing load of themes is a bit cluttered especially in the OU repository where we have several custom themes as well as the standard ones.
              Hide
              samhemelryk Sam Hemelryk added a comment -

              My vote would be to make clean default in 2.7, and in 2.8 remove standard all together (moving it to contrib of course).
              Hiding seems unnatural to me, just another thing users have to do. Really if we hide it lets just remove it and move to contrib.
              For theme's that have parented standard they will of course need to install it from contrib.

              Show
              samhemelryk Sam Hemelryk added a comment - My vote would be to make clean default in 2.7, and in 2.8 remove standard all together (moving it to contrib of course). Hiding seems unnatural to me, just another thing users have to do. Really if we hide it lets just remove it and move to contrib. For theme's that have parented standard they will of course need to install it from contrib.
              Hide
              lazydaisy Mary Evans added a comment - - edited

              +1 to dump all standard based themes to plugin database.

              The sooner the better, that way we can all concentrate on getting Moodle Bootstrap friendly, and ready to upgrade to Bootstrap 3 which is fun to play with.

              Show
              lazydaisy Mary Evans added a comment - - edited +1 to dump all standard based themes to plugin database. The sooner the better, that way we can all concentrate on getting Moodle Bootstrap friendly, and ready to upgrade to Bootstrap 3 which is fun to play with.
              Hide
              jerome Jérôme Mouneyrac added a comment -

              As 2.7 is a LTS I would vote to remove all none bootstrap themes from Moodle in 2.7. Of course before upgrade a message must clearly state what themes have been removed and how to reinstall them easily if the administrator don't want to fallback to Clean.

              Show
              jerome Jérôme Mouneyrac added a comment - As 2.7 is a LTS I would vote to remove all none bootstrap themes from Moodle in 2.7. Of course before upgrade a message must clearly state what themes have been removed and how to reinstall them easily if the administrator don't want to fallback to Clean.
              Hide
              samhemelryk Sam Hemelryk added a comment -

              The idea of shipping with just two themes have infinite appeal to me, perhaps the LTS release is reason enough to break with our standard deprecation protocol?

              Show
              samhemelryk Sam Hemelryk added a comment - The idea of shipping with just two themes have infinite appeal to me, perhaps the LTS release is reason enough to break with our standard deprecation protocol?
              Hide
              moodle.com moodle.com added a comment -

              This is dependent on a few current issues, so it won't be able to be worked on for a while.

              Show
              moodle.com moodle.com added a comment - This is dependent on a few current issues, so it won't be able to be worked on for a while.
              Hide
              andyjdavis Andrew Davis added a comment -

              I don't think this blocks MDL-43839. We can change the default theme regardless of whether or not we move any of the other themes.

              Show
              andyjdavis Andrew Davis added a comment - I don't think this blocks MDL-43839 . We can change the default theme regardless of whether or not we move any of the other themes.
              Hide
              dougiamas Martin Dougiamas added a comment -

              This is actually blocked by MDL-43786 which will give Clean some basic customisation options in the UI.

              This will make it less necessary to have a range of themes in the box so that people with simple server setups have options.

              Show
              dougiamas Martin Dougiamas added a comment - This is actually blocked by MDL-43786 which will give Clean some basic customisation options in the UI. This will make it less necessary to have a range of themes in the box so that people with simple server setups have options.
              Hide
              dougiamas Martin Dougiamas added a comment -

              OK, so just documenting overall that we are doing this in Moodle 2.7:

              a) Leaving Clean as it is, and making it the new default. MDL-42964 MDL-41459
              b) Making a new theme called "More" with all the easy customisation stuff, including built-in LESS. MDL-43786 MDL-44357
              c) Deprecate many of the old "base-based" themes currently only there to make up for previous lack of b). This means removing them from 2.7 and moving them into moodle.org/plugins for those who still want them. MDL-43784

              Show
              dougiamas Martin Dougiamas added a comment - OK, so just documenting overall that we are doing this in Moodle 2.7: a) Leaving Clean as it is, and making it the new default. MDL-42964 MDL-41459 b) Making a new theme called "More" with all the easy customisation stuff, including built-in LESS. MDL-43786 MDL-44357 c) Deprecate many of the old "base-based" themes currently only there to make up for previous lack of b). This means removing them from 2.7 and moving them into moodle.org/plugins for those who still want them. MDL-43784
              Hide
              lazydaisy Mary Evans added a comment - - edited

              Does this mean we are dropping Base and Canvas themes too?

              Apologies...ignore my last comment, I should have read the description first before asking a silly question!

              Show
              lazydaisy Mary Evans added a comment - - edited Does this mean we are dropping Base and Canvas themes too? Apologies...ignore my last comment, I should have read the description first before asking a silly question!
              Hide
              fred Frédéric Massart added a comment -

              As per my understanding we were keeping standard too. Can you confirm that Martin Dougiamas? Under which account should the plugins be put on github?

              Show
              fred Frédéric Massart added a comment - As per my understanding we were keeping standard too. Can you confirm that Martin Dougiamas ? Under which account should the plugins be put on github?
              Hide
              dougiamas Martin Dougiamas added a comment -

              https://github.com/moodlehq is fine.

              As for standard, I think the name is only confusing for new users and prefer to lose it. However, I'd be open to keeping it hidden from the UI like canvas so devs can still test 'base' with it sometimes.

              Show
              dougiamas Martin Dougiamas added a comment - https://github.com/moodlehq is fine. As for standard, I think the name is only confusing for new users and prefer to lose it. However, I'd be open to keeping it hidden from the UI like canvas so devs can still test 'base' with it sometimes.
              Hide
              dougiamas Martin Dougiamas added a comment -

              After a bit of grepping the only child themes of "standard" I could find are theme_flexpage, theme_simple, theme_krystle2 and theme_standard_fluid

              Show
              dougiamas Martin Dougiamas added a comment - After a bit of grepping the only child themes of "standard" I could find are theme_flexpage, theme_simple, theme_krystle2 and theme_standard_fluid
              Hide
              fred Frédéric Massart added a comment -

              Sending this for peer review.

              • As agreed by Martin, standard retires from core.
              • The upgrade process is identical to mymobile retirement (See upgrade.php or 7c2efc11 and e0caccb4).
              • I added a major upgrade required because it's much safer.
              • Please note the blocking issues.
              Show
              fred Frédéric Massart added a comment - Sending this for peer review. As agreed by Martin, standard retires from core. The upgrade process is identical to mymobile retirement (See upgrade.php or 7c2efc11 and e0caccb4). I added a major upgrade required because it's much safer. Please note the blocking issues.
              Hide
              cibot CiBoT added a comment -

              Results for MDL-43784

              • Remote repository: git://github.com/FMCorz/moodle.git
              Show
              cibot CiBoT added a comment - Results for MDL-43784 Remote repository: git://github.com/FMCorz/moodle.git Remote branch MDL-43784 -master to be integrated into upstream master Executed job http://integration.moodle.org/job/Precheck%20remote%20branch/2353 Details: http://integration.moodle.org/job/Precheck%20remote%20branch/2353/artifact/work/smurf.html
              Hide
              fred Frédéric Massart added a comment - - edited

              All the themes have been uploaded on github.com/moodlehq.

              I also sent an email to those who appeared to be the original authors:

              Show
              fred Frédéric Massart added a comment - - edited All the themes have been uploaded on github.com/moodlehq. I also sent an email to those who appeared to be the original authors: Mary Evans Patrick Malley John Stabinger Daniele Cordella Caroline Kennedy
              Hide
              damyon Damyon Wiese added a comment -

              Thanks Fred,

              This all looks correct to me. You have repeated the upgrade step that we did for mymobile and that still looks correct to me. I tested this with a custom theme based on canvas and there were no side effects.

              Note for integrator to pay attention to the blockers. This patch is not rebased on them - but it needs the clean as default one to avoid errors on the upgrade page.

              +1 from me.

              Not much to comment on the code because it's mostly deleting things (yay)!

              It is very important that we get the old themes on the plugins DB before release (you have a followup issue for it - I'm just repeating it).

              Show
              damyon Damyon Wiese added a comment - Thanks Fred, This all looks correct to me. You have repeated the upgrade step that we did for mymobile and that still looks correct to me. I tested this with a custom theme based on canvas and there were no side effects. Note for integrator to pay attention to the blockers. This patch is not rebased on them - but it needs the clean as default one to avoid errors on the upgrade page. +1 from me. Not much to comment on the code because it's mostly deleting things (yay)! It is very important that we get the old themes on the plugins DB before release (you have a followup issue for it - I'm just repeating it).
              Hide
              fred Frédéric Massart added a comment -

              Thanks Damyon. Yes, this is MDL-44705, which I have intentionally set as Blocker.

              Show
              fred Frédéric Massart added a comment - Thanks Damyon. Yes, this is MDL-44705 , which I have intentionally set as Blocker .
              Hide
              fred Frédéric Massart added a comment -

              Rebased. 2 changes in themes (serenity, brick) were introduced since last week, those have been pushed in their respective repos.

              Show
              fred Frédéric Massart added a comment - Rebased. 2 changes in themes (serenity, brick) were introduced since last week, those have been pushed in their respective repos.
              Hide
              fred Frédéric Massart added a comment -

              The theme_formal_white has transferred to Andrea Bicciolo (github.com/andreabix) because him and Daniele Cordella were the original authors.

              Show
              fred Frédéric Massart added a comment - The theme_formal_white has transferred to Andrea Bicciolo (github.com/andreabix) because him and Daniele Cordella were the original authors.
              Hide
              cibot CiBoT added a comment -

              Moving this issue to current integration cycle, will be reviewed soon. Thanks for the hard work!

              Show
              cibot CiBoT added a comment - Moving this issue to current integration cycle, will be reviewed soon. Thanks for the hard work!
              Hide
              samhemelryk Sam Hemelryk added a comment -

              S?o I don't forget to ask is this actually blocked by MDL-43786 Fred?

              Show
              samhemelryk Sam Hemelryk added a comment - S?o I don't forget to ask is this actually blocked by MDL-43786 Fred?
              Hide
              fred Frédéric Massart added a comment - - edited

              Technically yes, but as MDL-43786 is a meta, I'd say only MDL-43839 and MDL-44357 are blocking it. We need the new theme in MDL-44357 and Clean as default in MDL-43839.

              Actually, MDL-44364 is blocking this one too, because Martin's requirement for removing the themes is to have theme_more integrated.

              Show
              fred Frédéric Massart added a comment - - edited Technically yes, but as MDL-43786 is a meta, I'd say only MDL-43839 and MDL-44357 are blocking it. We need the new theme in MDL-44357 and Clean as default in MDL-43839 . Actually, MDL-44364 is blocking this one too, because Martin's requirement for removing the themes is to have theme_more integrated.
              Hide
              samhemelryk Sam Hemelryk added a comment -

              Ok this has been integrated.

              HOWEVER unit tests are failing because a couple of assertions in lib/tests/outputcomponents_test.php rely on themes that have now been removed.

              Fred you need to get a fix up for these that I can integrate ASAP.
              It is going to muck with all issues that specify unit tests be run.
              So please. ASAP.

              Unit tests are failing

              1) core_outputcomponents_testcase::test_get_url
              Failed asserting that file "/var/www/integration/theme/afterburner/config.php" exists.

              /var/www/integration/lib/tests/outputcomponents_test.php:234
              /var/www/integration/lib/phpunit/classes/advanced_testcase.php:80

              To re-run:
              vendor/bin/phpunit core_outputcomponents_testcase lib/tests/outputcomponents_test.php

              Show
              samhemelryk Sam Hemelryk added a comment - Ok this has been integrated. HOWEVER unit tests are failing because a couple of assertions in lib/tests/outputcomponents_test.php rely on themes that have now been removed. Fred you need to get a fix up for these that I can integrate ASAP. It is going to muck with all issues that specify unit tests be run. So please. ASAP. Unit tests are failing 1) core_outputcomponents_testcase::test_get_url Failed asserting that file "/var/www/integration/theme/afterburner/config.php" exists. /var/www/integration/lib/tests/outputcomponents_test.php:234 /var/www/integration/lib/phpunit/classes/advanced_testcase.php:80 To re-run: vendor/bin/phpunit core_outputcomponents_testcase lib/tests/outputcomponents_test.php
              Hide
              salvetore Michael de Raadt added a comment -

              I ran into this fail also.

              Show
              salvetore Michael de Raadt added a comment - I ran into this fail also.
              Hide
              abgreeve Adrian Greeve added a comment -

              Waiting on a fix for the unit tests before passing this.

              Show
              abgreeve Adrian Greeve added a comment - Waiting on a fix for the unit tests before passing this.
              Hide
              fred Frédéric Massart added a comment -

              Here is a fix, sorry about that.

              git://github.com/FMCorz/moodle.git
              MDL-43784-master-fixup
              https://github.com/FMCorz/moodle/compare/MDL-43784-master-fixup%5E...MDL-43784-master-fixup

              After discussing it with Sam, I decided to comment out those tests because they were relying on afterburner and formal_white. Really the solution would be to have fixtures of themes to be able to test those properly. I have raised MDL-44792 for this.

              Cheers,
              Fred

              Show
              fred Frédéric Massart added a comment - Here is a fix, sorry about that. git://github.com/FMCorz/moodle.git MDL-43784 -master-fixup https://github.com/FMCorz/moodle/compare/MDL-43784-master-fixup%5E...MDL-43784-master-fixup After discussing it with Sam, I decided to comment out those tests because they were relying on afterburner and formal_white . Really the solution would be to have fixtures of themes to be able to test those properly. I have raised MDL-44792 for this. Cheers, Fred
              Hide
              samhemelryk Sam Hemelryk added a comment -

              Thanks Fred - this fix has been integrated now - all yours once more Adrian.

              Show
              samhemelryk Sam Hemelryk added a comment - Thanks Fred - this fix has been integrated now - all yours once more Adrian.
              Hide
              abgreeve Adrian Greeve added a comment -

              Unit tests passed and everything else worked as expected.
              Test passed.

              Show
              abgreeve Adrian Greeve added a comment - Unit tests passed and everything else worked as expected. Test passed.
              Hide
              marina Marina Glancy added a comment -

              this fails behat test:

              (::) failed steps (::)
               
              01. Table_row matching locator "'Afterburner'" not found.
                  In step `And I click on "Use theme" "button" in the "Afterburner" "table_row"'. # behat_general::i_click_on_in_the()
                  From scenario background.                                                       # /home/marina/repositories/int_master/moodle/blocks/tests/behat/manage_blocks.feature:7
                  Of feature `Block appearances'.                                                 # /home/marina/repositories/int_master/moodle/blocks/tests/behat/manage_blocks.feature
              

              Show
              marina Marina Glancy added a comment - this fails behat test: (::) failed steps (::)   01. Table_row matching locator "'Afterburner'" not found. In step `And I click on "Use theme" "button" in the "Afterburner" "table_row"'. # behat_general::i_click_on_in_the() From scenario background. # /home/marina/repositories/int_master/moodle/blocks/tests/behat/manage_blocks.feature:7 Of feature `Block appearances'. # /home/marina/repositories/int_master/moodle/blocks/tests/behat/manage_blocks.feature
              Hide
              fred Frédéric Massart added a comment -

              Oops. Getting onto that when I finish my other tests.

              Show
              fred Frédéric Massart added a comment - Oops. Getting onto that when I finish my other tests.
              Hide
              fred Frédéric Massart added a comment -

              There is one more commit in the fixup branch.

              git://github.com/FMCorz/moodle.git
              MDL-43784-master-fixup
              https://github.com/FMCorz/moodle/compare/MDL-43784-master-fixup^...MDL-43784-master-fixup

              I removed the usage of the theme. The MDLQA that were converted in that feature stated they needed a 3 columns theme. As Clean is one we don't need to switch.

              Show
              fred Frédéric Massart added a comment - There is one more commit in the fixup branch. git://github.com/FMCorz/moodle.git MDL-43784 -master-fixup https://github.com/FMCorz/moodle/compare/MDL-43784-master-fixup ^... MDL-43784 -master-fixup I removed the usage of the theme. The MDLQA that were converted in that feature stated they needed a 3 columns theme. As Clean is one we don't need to switch.
              Hide
              poltawski Dan Poltawski added a comment -

              Thanks Fred. I pulled in that fix (and tested @core_block and found an atto issue)

              Show
              poltawski Dan Poltawski added a comment - Thanks Fred. I pulled in that fix (and tested @core_block and found an atto issue)
              Hide
              stronk7 Eloy Lafuente (stronk7) added a comment -

              Clothes and manners do
              not make the man; but,
              when he is made, they
              greatly improve his appearance.

              ---- Henry Ward Beecher

              What a week, your changes are now part of Moodle, well done!

              Closing, thanks!

              Show
              stronk7 Eloy Lafuente (stronk7) added a comment - Clothes and manners do not make the man; but, when he is made, they greatly improve his appearance. ---- Henry Ward Beecher What a week, your changes are now part of Moodle, well done! Closing, thanks!
              Hide
              marycooch Mary Cooch added a comment -

              Removing docs_required label as this is documented in the Release notes http://docs.moodle.org/dev/Moodle_2.7_release_notes and in http://docs.moodle.org/27/en/Standard_themes

              Show
              marycooch Mary Cooch added a comment - Removing docs_required label as this is documented in the Release notes http://docs.moodle.org/dev/Moodle_2.7_release_notes and in http://docs.moodle.org/27/en/Standard_themes

                People

                • Votes:
                  2 Vote for this issue
                  Watchers:
                  17 Start watching this issue

                  Dates

                  • Created:
                    Updated:
                    Resolved:
                    Fix Release Date:
                    12/May/14