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

          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