Details

    • Type: Sub-task
    • Status: Closed
    • Priority: Minor
    • Resolution: Fixed
    • Affects Version/s: 2.4.3, 2.5
    • Fix Version/s: 2.4.8, 2.5.4, 2.6
    • Component/s: Themes
    • Labels:
    • Testing Instructions:
      Hide
      1. Check each of the following themes:
        • afterburner
        • brick
        • magazine
      2. Make sure the theme doesn't have a logo defined in its settings (and not background for magazine)
      3. Make sure the logo is displayed as it should
        • brick and magazine do not display a logo at all when none is defined
      4. Make sure the background image of magazine is correct
      5. Set a logo for each theme, and a background color for magazine.
      6. Make sure they appear as expected

      Test each branch separately because 2.6 code is slightly different

      Show
      Check each of the following themes: afterburner brick magazine Make sure the theme doesn't have a logo defined in its settings (and not background for magazine) Make sure the logo is displayed as it should brick and magazine do not display a logo at all when none is defined Make sure the background image of magazine is correct Set a logo for each theme, and a background color for magazine. Make sure they appear as expected Test each branch separately because 2.6 code is slightly different
    • Affected Branches:
      MOODLE_24_STABLE, MOODLE_25_STABLE
    • Fixed Branches:
      MOODLE_24_STABLE, MOODLE_25_STABLE, MOODLE_26_STABLE
    • Pull from Repository:
    • Pull Master Branch:
      MDL-42043-master

      Description

      See discussion:

      https://tracker.moodle.org/browse/MDL-42009?focusedCommentId=246618&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-246618

      List of Themes currently affected:

      afterburner/lib.php
      brick/lib.php
      clean/lib.php
      magazine/lib.php
      mymobile/renderers.php

        Gliffy Diagrams

          Attachments

            Activity

            Hide
            fred Frédéric Massart added a comment -

            Requesting peer review.

            1. I could not replicate the error, but it makes sense to avoid the usage of $OUTPUT as it goes back to the theme_config class anyway... except that it fails, sometimes...
            2. This doesn't apply to Clean as the usage of $OUTPUT there is legit.
            3. The problem should have never occurred on other themes than Afterburner as the logo wasn't displayed when not set.

            Cheers,
            Fred

            Show
            fred Frédéric Massart added a comment - Requesting peer review. I could not replicate the error, but it makes sense to avoid the usage of $OUTPUT as it goes back to the theme_config class anyway... except that it fails, sometimes... This doesn't apply to Clean as the usage of $OUTPUT there is legit. The problem should have never occurred on other themes than Afterburner as the logo wasn't displayed when not set. Cheers, Fred
            Hide
            skodak Petr Skoda added a comment -

            +1, makes sense

            Show
            skodak Petr Skoda added a comment - +1, makes sense
            Hide
            lazydaisy Mary Evans added a comment -

            I was going to do this and forgot to assign it to myself! Thanks for picking this up Fred.

            Show
            lazydaisy Mary Evans added a comment - I was going to do this and forgot to assign it to myself! Thanks for picking this up Fred.
            Hide
            fred Frédéric Massart added a comment -

            No worries! Pushing for integration. Thanks guys.

            Show
            fred Frédéric Massart added a comment - No worries! Pushing for integration. Thanks guys.
            Hide
            marina Marina Glancy added a comment -

            Thanks Fred, looks good, just one suggestion:

            In a very unlikely but possible case when there is a 3rd party theme based on afterburner/magazine/brick, can we please have a default value (null) for the new argument in functions and assume it as $OUTPUT? In 2.6 we can show debugging message

            P.S. Why is it a must-fix for 2.6 if it is an existing bug on all stable branches?

            Show
            marina Marina Glancy added a comment - Thanks Fred, looks good, just one suggestion: In a very unlikely but possible case when there is a 3rd party theme based on afterburner/magazine/brick, can we please have a default value (null) for the new argument in functions and assume it as $OUTPUT? In 2.6 we can show debugging message P.S. Why is it a must-fix for 2.6 if it is an existing bug on all stable branches?
            Hide
            fred Frédéric Massart added a comment -

            Hi Marina,

            I have fixed 2.4 and 2.5 to accept handle the optional argument.

            I have no idea why this is a must-fix. I suppose it's more of a "the sooner fixed the better" bug.

            Cheers,
            Fred

            Show
            fred Frédéric Massart added a comment - Hi Marina, I have fixed 2.4 and 2.5 to accept handle the optional argument. I have no idea why this is a must-fix. I suppose it's more of a "the sooner fixed the better" bug. Cheers, Fred
            Hide
            marina Marina Glancy added a comment -

            Thanks Fred, integrated in 2.4, 2.5 and 2.6.

            I added a commit on top to fix tabs in theme/magazine/lib.php, otherwise the checker on integration server would have complained

            Show
            marina Marina Glancy added a comment - Thanks Fred, integrated in 2.4, 2.5 and 2.6. I added a commit on top to fix tabs in theme/magazine/lib.php, otherwise the checker on integration server would have complained
            Hide
            ankit_frenz Ankit Agarwal added a comment -

            This works as described, however I had to manually purge caches in 24 after every change for it to take effect. Is this know behaviour?

            Show
            ankit_frenz Ankit Agarwal added a comment - This works as described, however I had to manually purge caches in 24 after every change for it to take effect. Is this know behaviour?
            Hide
            marina Marina Glancy added a comment -

            yes Ankit, we don't bump version unnecessary in integration (unless there is an upgrade script). But when weeklies is released the version will be bumped and cache is being reset on upgrade

            Show
            marina Marina Glancy added a comment - yes Ankit, we don't bump version unnecessary in integration (unless there is an upgrade script). But when weeklies is released the version will be bumped and cache is being reset on upgrade
            Hide
            ankit_frenz Ankit Agarwal added a comment -

            Hi Marina,
            I am talking about changes in theme config, that is not really related to the version bumps. I had to purge cache every time I saved a logo, for the changes to take effect in 24 but not in 25 and master. However this could be something in my environment.

            Cheers

            Show
            ankit_frenz Ankit Agarwal added a comment - Hi Marina, I am talking about changes in theme config, that is not really related to the version bumps. I had to purge cache every time I saved a logo, for the changes to take effect in 24 but not in 25 and master. However this could be something in my environment. Cheers
            Hide
            marina Marina Glancy added a comment -

            I'd say this is a bug then

            Show
            marina Marina Glancy added a comment - I'd say this is a bug then
            Hide
            gb2048 Gareth J Barnard added a comment -

            Hi Ankit Agarwal,

            I suspect that the 'purge' for the logo in M2.4 is because the image serving mechanism in M2.5 themes has the following line for the setting in the 'settings.php' file:

            $setting->set_updatedcallback('theme_reset_all_caches');

            Which operates each time the logo is set in the settings. I have recently a similar issue with images and the Grid format, so had to invent an increasing counter that was prepended to the image file name to make the browser think it was a different image and therefore request it from the server.

            I hope this helps, cheers,

            Gareth

            Show
            gb2048 Gareth J Barnard added a comment - Hi Ankit Agarwal , I suspect that the 'purge' for the logo in M2.4 is because the image serving mechanism in M2.5 themes has the following line for the setting in the 'settings.php' file: $setting->set_updatedcallback('theme_reset_all_caches'); Which operates each time the logo is set in the settings. I have recently a similar issue with images and the Grid format, so had to invent an increasing counter that was prepended to the image file name to make the browser think it was a different image and therefore request it from the server. I hope this helps, cheers, Gareth
            Hide
            lazydaisy Mary Evans added a comment - - edited

            I don't think of it as a BUG since we were not aware of the setting, which Gareth mentions in his comment above, we just assumed one needed to have Theme Designer Mode enabled. Now we know differently we can add it. So if that makes it a BUG then it needs reporting and fixing. But can we use this setting safely in Moodle 2.4? If we can then this will mean we will have to add this to ALL the setting in every theme that uses a setting.php for 2.4/2.5 & 2.6 and not just for logos I am talking all theme custom settings like link colours and such.

            Show
            lazydaisy Mary Evans added a comment - - edited I don't think of it as a BUG since we were not aware of the setting, which Gareth mentions in his comment above, we just assumed one needed to have Theme Designer Mode enabled. Now we know differently we can add it. So if that makes it a BUG then it needs reporting and fixing. But can we use this setting safely in Moodle 2.4? If we can then this will mean we will have to add this to ALL the setting in every theme that uses a setting.php for 2.4/2.5 & 2.6 and not just for logos I am talking all theme custom settings like link colours and such.
            Hide
            marina Marina Glancy added a comment -

            hehe, 2.4 is supported just for another month. Should we bother?

            Show
            marina Marina Glancy added a comment - hehe, 2.4 is supported just for another month. Should we bother?
            Hide
            ankit_frenz Ankit Agarwal added a comment -

            MDL-39178 fixed this issue in 2.5. The issue is marked as improvement, may be that is why it was not backported. Anyway this should be a simple backport if we want it in 2.4.

            Show
            ankit_frenz Ankit Agarwal added a comment - MDL-39178 fixed this issue in 2.5. The issue is marked as improvement, may be that is why it was not backported. Anyway this should be a simple backport if we want it in 2.4.
            Hide
            marina Marina Glancy added a comment -

            no, encourage people to upgrade

            Show
            marina Marina Glancy added a comment - no, encourage people to upgrade
            Hide
            stronk7 Eloy Lafuente (stronk7) added a comment -

            It's Friday, I'm tired so I won't be very imaginative today.

            No matter of that, yes, you did it! Thanks for your collaboration!

            Closing this as fixed!

            Show
            stronk7 Eloy Lafuente (stronk7) added a comment - It's Friday, I'm tired so I won't be very imaginative today. No matter of that, yes, you did it! Thanks for your collaboration! Closing this as fixed!

              People

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

                Dates

                • Created:
                  Updated:
                  Resolved:
                  Fix Release Date:
                  18/Nov/13