Moodle
  1. Moodle
  2. MDL-27866

pix_url call in pluginfile.php produces error

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Blocker Blocker
    • Resolution: Fixed
    • Affects Version/s: 2.0.3
    • Fix Version/s: 2.1
    • Component/s: Files API
    • Labels:
    • Rank:
      17514

      Description

      in debug mode, when try to access user profile image, say: http://qa.moodle.net/pluginfile.php/153/user/icon/f1

      It's a problem if user didn't set profile, it will produce a broken image, it also affect moodle mobile app.

        Issue Links

          Activity

          Hide
          Sam Hemelryk added a comment -

          Looks good to me DS, once up for integration I may assign to Eloy however just to be doubly sure

          Show
          Sam Hemelryk added a comment - Looks good to me DS, once up for integration I may assign to Eloy however just to be doubly sure
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Ho,

          I understand why you are doing that... but I really don't like those sort of hacks in pluginfile.php at all (I think SamH had the same problem not much ago with another page and some other method requiring complete init of nosense context stuff).

          Really the point to fix here is the bloody output/theme/page initialization and not make it require any page nor context, just to return one (static) image from one theme. We cannot be adding fake contexts each time we get this problem IMO.

          So, if it's really needed +0.1, BUT ONLY IF the proper issue is created and addressed as blocker for 2.1.1. Only then!

          Horrible! Thanks! Ciao

          Show
          Eloy Lafuente (stronk7) added a comment - Ho, I understand why you are doing that... but I really don't like those sort of hacks in pluginfile.php at all (I think SamH had the same problem not much ago with another page and some other method requiring complete init of nosense context stuff). Really the point to fix here is the bloody output/theme/page initialization and not make it require any page nor context, just to return one (static) image from one theme. We cannot be adding fake contexts each time we get this problem IMO. So, if it's really needed +0.1, BUT ONLY IF the proper issue is created and addressed as blocker for 2.1.1. Only then! Horrible! Thanks! Ciao
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Adding to integration for SamH final decision, discuss it there.

          Another point, btw... is why you're trying to one image without previously checking if it exists or why pluginnfile is doing that redirect on fail. It should be detected earlier, by the caller, not offering any fallback in pluginfile IMO.

          Show
          Eloy Lafuente (stronk7) added a comment - Adding to integration for SamH final decision, discuss it there. Another point, btw... is why you're trying to one image without previously checking if it exists or why pluginnfile is doing that redirect on fail. It should be detected earlier, by the caller, not offering any fallback in pluginfile IMO.
          Hide
          Dongsheng Cai added a comment -

          Created MDL-28043 for further inspection.

          Show
          Dongsheng Cai added a comment - Created MDL-28043 for further inspection.
          Hide
          Sam Hemelryk added a comment -

          Hi Eloy,

          Sorry I think we probably should have included a bit more background on this issue for you.
          When it first came up Dongsheng and myself had a discussion about what the best way to solve this particular issue was.
          Over the past couple of weeks as web services have been popping up there have been other similar issues (not with files but with other areas) and its become apparent that we need to allow for these alternative output systems for areas like this.
          In the case of many of the webservice situations the best solution (that we could come up with anyway) is to implement a core_renderer_webservice class and a define switch WEBSERVICE_SCRIPT like we have done with AJAX scripts. (there should be a bug for this in the queue for 2.1.1 or 2.2)

          In the case of this change I completely agree this is a hack - and an unfortunate one at that.
          Because of the desire to get the moodle modile app up and running as soon as possible after the release of 2.1 there is some priority on getting a solution to this sorted and for that reason it was put up for integration.

          That all being said I've been looked over the code a couple of times now and I'm not sure this should go in. I think the webservice may be taking the wrong approach.

          Presently within Moodle when viewing an users icon it is offloading all of the work to pluginfile.php, arriving at pluginfile.php it is asecertained whether or not the user actually has a profile picture.
          If the user has a profile picture then great it is served. If not however pluginfile.php redirects to theme/image.php and in doing so needs to ascertain the theme the should be used to search for the profile picture within.
          To do this it makes use of $OUTPUT which has to initialise the theme and output in order to initialise the correct theme at which point we call theme->pix_url.
          Really to fix the use of $OUTPUT within pluginfile.php a better solution for theme and output initialisation needs to be arrived at - or we need to come up with a system of working out the current theme for the purposes of redirecting without initialising theme and output.
          Both of those routes sound like they would be complicated and not really sutiable for 2.1 at such a late stage making the addition of the set_context call a hack to get things working until we have time to actually look for a solution to the problem.

          The second question of course is whether or not the calling code should be responsible for checking if the user has a profile picture or not.
          It certainly should be able to check - if nesecary to do so. However in the case of webservices I think (as I don't have complete knowledge of webservices) that this would have a negative effect if not causing errors because it would lead to theem and output being initialised again in place we don't want them.

          There is one more point about this code however - now that I look at it should the context be the users context that is being checked 2 lines above this change? perhaps that is the tipped.

          This patch gets my +1 providing that context is checked out AND given that we have a blocker issue to find a better solution.

          DS I'll wait for your reply about the context.
          Eloy if you are want to integrate or reject based upon this extra information feel free too otherwise I will look at it once I have DS's feedback.

          Cheers
          Sam

          Show
          Sam Hemelryk added a comment - Hi Eloy, Sorry I think we probably should have included a bit more background on this issue for you. When it first came up Dongsheng and myself had a discussion about what the best way to solve this particular issue was. Over the past couple of weeks as web services have been popping up there have been other similar issues (not with files but with other areas) and its become apparent that we need to allow for these alternative output systems for areas like this. In the case of many of the webservice situations the best solution (that we could come up with anyway) is to implement a core_renderer_webservice class and a define switch WEBSERVICE_SCRIPT like we have done with AJAX scripts. (there should be a bug for this in the queue for 2.1.1 or 2.2) In the case of this change I completely agree this is a hack - and an unfortunate one at that. Because of the desire to get the moodle modile app up and running as soon as possible after the release of 2.1 there is some priority on getting a solution to this sorted and for that reason it was put up for integration. That all being said I've been looked over the code a couple of times now and I'm not sure this should go in. I think the webservice may be taking the wrong approach. Presently within Moodle when viewing an users icon it is offloading all of the work to pluginfile.php, arriving at pluginfile.php it is asecertained whether or not the user actually has a profile picture. If the user has a profile picture then great it is served. If not however pluginfile.php redirects to theme/image.php and in doing so needs to ascertain the theme the should be used to search for the profile picture within. To do this it makes use of $OUTPUT which has to initialise the theme and output in order to initialise the correct theme at which point we call theme->pix_url. Really to fix the use of $OUTPUT within pluginfile.php a better solution for theme and output initialisation needs to be arrived at - or we need to come up with a system of working out the current theme for the purposes of redirecting without initialising theme and output. Both of those routes sound like they would be complicated and not really sutiable for 2.1 at such a late stage making the addition of the set_context call a hack to get things working until we have time to actually look for a solution to the problem. The second question of course is whether or not the calling code should be responsible for checking if the user has a profile picture or not. It certainly should be able to check - if nesecary to do so. However in the case of webservices I think (as I don't have complete knowledge of webservices) that this would have a negative effect if not causing errors because it would lead to theem and output being initialised again in place we don't want them. There is one more point about this code however - now that I look at it should the context be the users context that is being checked 2 lines above this change? perhaps that is the tipped. This patch gets my +1 providing that context is checked out AND given that we have a blocker issue to find a better solution. DS I'll wait for your reply about the context. Eloy if you are want to integrate or reject based upon this extra information feel free too otherwise I will look at it once I have DS's feedback. Cheers Sam
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Yeah, my main point here is that, in order to serve one (fixed, in disk) theme image we are requiring all the output/page stuff to be initialised, and I think it's way too much for displaying one simple disk-based file, i.e. it's not really a PAGE request but an image one and all those (static) requests shouldn't require the init of the whole BEAST at all, only the minimal part about themes/revisions perhaps.

          In any case, if the blocker is already there (MDL-28043) and it's clearly stated that it's IMPORTANT in order to get this reverted ASAP, feel free to go with it. I would do that, by my soul forbids me to do so, sincerely, LOL.

          Thanks for the complete explanations, ciao

          Show
          Eloy Lafuente (stronk7) added a comment - Yeah, my main point here is that, in order to serve one (fixed, in disk) theme image we are requiring all the output/page stuff to be initialised, and I think it's way too much for displaying one simple disk-based file, i.e. it's not really a PAGE request but an image one and all those (static) requests shouldn't require the init of the whole BEAST at all, only the minimal part about themes/revisions perhaps. In any case, if the blocker is already there ( MDL-28043 ) and it's clearly stated that it's IMPORTANT in order to get this reverted ASAP, feel free to go with it. I would do that, by my soul forbids me to do so, sincerely, LOL. Thanks for the complete explanations, ciao
          Hide
          Sam Hemelryk added a comment -

          Hi guys,

          This has been integrated now.
          Thanks for the reply Eloy, made me laugh.

          Cheers
          Sam

          Show
          Sam Hemelryk added a comment - Hi guys, This has been integrated now. Thanks for the reply Eloy, made me laugh. Cheers Sam
          Hide
          Rajesh Taneja added a comment -

          Thanks for providing the fix Dongsheng

          Redirection works Great after the patch.
          I am not sure if it's useful to apply in 2.0?

          Show
          Rajesh Taneja added a comment - Thanks for providing the fix Dongsheng Redirection works Great after the patch. I am not sure if it's useful to apply in 2.0?
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Thanks Rajesh!

          -1 for 20_STABLE. If the cause for this was the mobile app and it's only 2.1 then let that piece of xxxx only in 21_STABLE. And only till MDL-28043 gets fixed.

          Ciao

          Show
          Eloy Lafuente (stronk7) added a comment - Thanks Rajesh! -1 for 20_STABLE. If the cause for this was the mobile app and it's only 2.1 then let that piece of xxxx only in 21_STABLE. And only till MDL-28043 gets fixed. Ciao
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Upstream-ized! Thanks!

          Show
          Eloy Lafuente (stronk7) added a comment - Upstream-ized! Thanks!

            People

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

              Dates

              • Created:
                Updated:
                Resolved: