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

          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

                  Agile