Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Blocker Blocker
    • Resolution: Fixed
    • Affects Version/s: 2.0, 2.1, 2.2
    • Fix Version/s: 2.0.5, 2.1.2
    • Component/s: Files API, Themes
    • Labels:
      None
    • Testing Instructions:
      Hide

      There should be no visible changes with this patch (better performance and more accuracy)

      1. Browse around the site and an admin and as a normal user and just check you can still see user profile pics as you did before.
      Show
      There should be no visible changes with this patch (better performance and more accuracy) Browse around the site and an admin and as a normal user and just check you can still see user profile pics as you did before.
    • Affected Branches:
      MOODLE_20_STABLE, MOODLE_21_STABLE, MOODLE_22_STABLE
    • Fixed Branches:
      MOODLE_20_STABLE, MOODLE_21_STABLE
    • Pull Master Branch:
      wip-MDL-28043-master
    • Rank:
      17670

      Description

      Introduced by MDL-27866.
      From the upper level, moodle should check if user profile image exists. and pix_url should trigger page initialization.

        Issue Links

          Activity

          Hide
          Sam Hemelryk added a comment -

          This needs to be looked at immediately after the release of 2.1 - DO NOT LOWER ITS PRIORITY it is the tip of the ice berg to a bigger problem (the initialisation of page, theme, and output).
          It also needs to be fixed in Moodle 2.0+ and 2.1+.

          Cheers
          Sam

          Show
          Sam Hemelryk added a comment - This needs to be looked at immediately after the release of 2.1 - DO NOT LOWER ITS PRIORITY it is the tip of the ice berg to a bigger problem (the initialisation of page, theme, and output). It also needs to be fixed in Moodle 2.0+ and 2.1+. Cheers Sam
          Hide
          Sam Hemelryk added a comment -

          Ok - I've got a patch up for this.

          The changes remove the need to initialise the page, theme and output when serving a user profile pic if a users profile picture is not available at the time of the request.
          It also cleans up the context hack at the same time, and shifts the calculation of the URL for a profile pic from the output renderer and pluginfile to the user_picture component.

          Cheers
          Sam

          Show
          Sam Hemelryk added a comment - Ok - I've got a patch up for this. The changes remove the need to initialise the page, theme and output when serving a user profile pic if a users profile picture is not available at the time of the request. It also cleans up the context hack at the same time, and shifts the calculation of the URL for a profile pic from the output renderer and pluginfile to the user_picture component. Cheers Sam
          Hide
          Rajesh Taneja added a comment -

          Looks Great to me Sam...
          Thanks for improving Moodle

          Show
          Rajesh Taneja added a comment - Looks Great to me Sam... Thanks for improving Moodle
          Hide
          Sam Hemelryk added a comment -

          Rebased now

          Show
          Sam Hemelryk added a comment - Rebased now
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Hi, 3 questions before integrating this:

          • This line seems copy/paste unused one:
            $usercontext = get_context_instance(CONTEXT_USER, $this->user->id);
            
          • Minor: Not important at all but wouldn't be better to make the url ..../theme/fX instead of ..../fX/theme ?
          • This must be backported to 20_STABLE and 21_STABLE, correct? If so, please state it in the testing instructions and comment about it.

          I'll keep this open...ciao

          Show
          Eloy Lafuente (stronk7) added a comment - Hi, 3 questions before integrating this: This line seems copy/paste unused one: $usercontext = get_context_instance(CONTEXT_USER, $ this ->user->id); Minor: Not important at all but wouldn't be better to make the url ..../theme/fX instead of ..../fX/theme ? This must be backported to 20_STABLE and 21_STABLE, correct? If so, please state it in the testing instructions and comment about it. I'll keep this open...ciao
          Hide
          Jonathan Harker added a comment -

          I've rebased the Gravatar stuff on Sam's wip-MDL-28043-master branch - see MDL-21676

          Show
          Jonathan Harker added a comment - I've rebased the Gravatar stuff on Sam's wip- MDL-28043 -master branch - see MDL-21676
          Hide
          Eloy Lafuente (stronk7) added a comment -

          The main moodle.git repository has just been updated with latest weekly modifications. You may wish to rebase your PULL branches to simplify history and avoid any possible merge conflicts. This would also make integrator's life easier next week.

          TIA and ciao

          Show
          Eloy Lafuente (stronk7) added a comment - The main moodle.git repository has just been updated with latest weekly modifications. You may wish to rebase your PULL branches to simplify history and avoid any possible merge conflicts. This would also make integrator's life easier next week. TIA and ciao
          Hide
          Sam Hemelryk added a comment -

          Hi Eloy,

          Thanks for looking at it, sorry I didn't get a chance to review your comments while I was away.
          In regards to the three points you have raised:

          1: Definitely an unused typo carried across while I was rearranging that method.

          2: I'm open on discussion to this, the theme was added to the end to make it backwards compatible (albeit I'm sure it could be rearranged and still made bc).
          Perhaps the challenge will be coming up with a system that allows for the optional theme in the URL while still supporting all forms of the user pic, both default and what ever they have chosen. This is really only a problem if there is a path component to the URL, unfortunately I havn't had a look into, or think about this in all circumstances yet.
          If this is going to be backported to 20 and 21 then we had better decide now, if its not going to be backported or there is urgency to get this in (Id don't think there is is there?) then lets create a new issue and properly discuss and look at it. For the time being my +1 to delay this and look at it properly.

          3: I think this is suitable for backport, it should still be backwards compatible without any problems so my +1 to backport.

          Let me know what you think.

          Cheers
          Sam

          p.s. I am the dev so probably shouldn't integrate this, it could either wait till you're back online Eloy or I can assist Apu in reviewing it

          Show
          Sam Hemelryk added a comment - Hi Eloy, Thanks for looking at it, sorry I didn't get a chance to review your comments while I was away. In regards to the three points you have raised: 1: Definitely an unused typo carried across while I was rearranging that method. 2: I'm open on discussion to this, the theme was added to the end to make it backwards compatible (albeit I'm sure it could be rearranged and still made bc). Perhaps the challenge will be coming up with a system that allows for the optional theme in the URL while still supporting all forms of the user pic, both default and what ever they have chosen. This is really only a problem if there is a path component to the URL, unfortunately I havn't had a look into, or think about this in all circumstances yet. If this is going to be backported to 20 and 21 then we had better decide now, if its not going to be backported or there is urgency to get this in (Id don't think there is is there?) then lets create a new issue and properly discuss and look at it. For the time being my +1 to delay this and look at it properly. 3: I think this is suitable for backport, it should still be backwards compatible without any problems so my +1 to backport. Let me know what you think. Cheers Sam p.s. I am the dev so probably shouldn't integrate this, it could either wait till you're back online Eloy or I can assist Apu in reviewing it
          Hide
          Sam Hemelryk added a comment -

          Forgot to mention I am still setting things up - I will make the coding changes once everything is up and running at my end (or if you'd like to integrate feel free to remove the context during integration).

          Show
          Sam Hemelryk added a comment - Forgot to mention I am still setting things up - I will make the coding changes once everything is up and running at my end (or if you'd like to integrate feel free to remove the context during integration).
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Reopening heading to final solution this week. Apologizes for all these iterations before landing, but better once and correct.

          Show
          Eloy Lafuente (stronk7) added a comment - Reopening heading to final solution this week. Apologizes for all these iterations before landing, but better once and correct.
          Hide
          Michael de Raadt added a comment -

          Shifting this reopened issue to the current sprint.

          Show
          Michael de Raadt added a comment - Shifting this reopened issue to the current sprint.
          Hide
          Sam Hemelryk added a comment -

          Hi guys

          Putting this up for peer-review.
          I'm removed the unused context variable, switched the order of the themename in URL, and backported this to 20 and 21.

          Cheers
          Sam

          Show
          Sam Hemelryk added a comment - Hi guys Putting this up for peer-review. I'm removed the unused context variable, switched the order of the themename in URL, and backported this to 20 and 21. Cheers Sam
          Hide
          Aparup Banerjee added a comment -

          Hi Sam,
          Thanks for fixing up the @return so quick!

          • minor: extra semicolon on pluginfile.php:284
          • swapping between '&&' and 'and' in pluginfile.php:295 has the potential to be read/rewritten incorrectly. maybe a note to remind of the precedence needed there (if there is any).
            or perhaps multiple 'if'? note sure on the guidelines here.

          Otherwise the code looks alright to me , cheers!

          Show
          Aparup Banerjee added a comment - Hi Sam, Thanks for fixing up the @return so quick! minor: extra semicolon on pluginfile.php:284 swapping between '&&' and 'and' in pluginfile.php:295 has the potential to be read/rewritten incorrectly. maybe a note to remind of the precedence needed there (if there is any). or perhaps multiple 'if'? note sure on the guidelines here. Otherwise the code looks alright to me , cheers!
          Hide
          Sam Hemelryk added a comment -

          Thanks for checking that out, this is up for integration now

          Show
          Sam Hemelryk added a comment - Thanks for checking that out, this is up for integration now
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Integrated (finally), yay! Thanks!

          Show
          Eloy Lafuente (stronk7) added a comment - Integrated (finally), yay! Thanks!
          Hide
          Michael de Raadt added a comment -

          Test result: User picture was shown successfully at various sizes, before and after purging caches. Well done.

          Show
          Michael de Raadt added a comment - Test result: User picture was shown successfully at various sizes, before and after purging caches. Well done.
          Hide
          Eloy Lafuente (stronk7) added a comment -

          git & cvs repositories updated with your gorgeous code. Many thanks!

          Closing and ciao

          Show
          Eloy Lafuente (stronk7) added a comment - git & cvs repositories updated with your gorgeous code. Many thanks! Closing and ciao

            People

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

              Dates

              • Created:
                Updated:
                Resolved: