Uploaded image for project: 'Moodle'
  1. Moodle
  2. MDL-30769

URL resources to unknown types do not handle errors well

    Details

    • Testing Instructions:
      Hide
      1. within a course, add url resource
      2. add external url to non-existance page (eg: http://yourmoodle.com/notfound.html)
      3. set display to embed
      4. select 'save and display'
      5. VERIFY: You can see the 'not found' page embedded within the page.
      6. within a course, add url resource
      7. add external url to a page that exists
      8. set display to embed
      9. select 'save and display'
      10. VERIFY: that you can see the resource
      Show
      within a course, add url resource add external url to non-existance page (eg: http://yourmoodle.com/notfound.html ) set display to embed select 'save and display' VERIFY: You can see the 'not found' page embedded within the page. within a course, add url resource add external url to a page that exists set display to embed select 'save and display' VERIFY: that you can see the resource
    • Affected Branches:
      MOODLE_22_STABLE
    • Fixed Branches:
      MOODLE_24_STABLE
    • Pull from Repository:
    • Pull Master Branch:
      MDL-30769_iframe

      Description

      I would like to request change in mod/url. During addition of new url resource module tries to determine mime type served by URL. If determined type is not one from list of specially handled contents it uses general display method which is to embed the URL in this HTML tag:

      <div class="resourcecontent resourcegeneral">
      <object id="resourceobject" data="<url>" type="<mimetype>" width="800" height="600">
      $param
      $clicktoopen
      </object>
      </div>

      When you add a url resource to a course with location that does not exist (for example http://www3.open.ac.uk/courses/bin/p12.dll?C01S103) the error page will not be displayed to the user since most modern browsers refuse to load embedded page that returns HTTP code 404.

      We find that behavior not to our liking. We would prefer that error page to be displayed to the user. Would it be possible to add the setting where iframe usage could be forced for a specific URL and thus always displayed?

        Gliffy Diagrams

          Issue Links

            Activity

            Hide
            salvetore Michael de Raadt added a comment -

            Hi, Darko.

            I'm not sure what you mean there. It is possible to create links to different types of files. How each link is presented depends on its extension.

            Could you please explain what you think is lacking? Perhaps you could give us an example.

            Show
            salvetore Michael de Raadt added a comment - Hi, Darko. I'm not sure what you mean there. It is possible to create links to different types of files. How each link is presented depends on its extension. Could you please explain what you think is lacking? Perhaps you could give us an example.
            Hide
            darko.miletic Darko Miletic added a comment -

            Updated issue description. Let me know if this clears the doubts.

            Show
            darko.miletic Darko Miletic added a comment - Updated issue description. Let me know if this clears the doubts.
            Hide
            salvetore Michael de Raadt added a comment -

            Hi, Darko.

            Thanks for adding more to that. I have re-classified this as a bug, otherwise it won't get appropriate attention.

            Yes, the two solutions would be to either present the linked file in another way (like an iframe) or to detect there is a problem and present that to the user.

            Show
            salvetore Michael de Raadt added a comment - Hi, Darko. Thanks for adding more to that. I have re-classified this as a bug, otherwise it won't get appropriate attention. Yes, the two solutions would be to either present the linked file in another way (like an iframe) or to detect there is a problem and present that to the user.
            Hide
            salvetore Michael de Raadt added a comment -

            Carrying over to the next sprint.

            Show
            salvetore Michael de Raadt added a comment - Carrying over to the next sprint.
            Hide
            ankit_frenz Ankit Agarwal added a comment -

            Hi Rosie,
            There is bunch of issues with current approach:-

            • fopen is disabled on most servers for remote fetching
            • We can use curl alternatively, but it will require a lot of processing, parsing https flags, get parameters etc
            • It is one additional request each time someone visits the url resource, which can be a huge performance hit, as it is making an external request.
            • fopen and curl are not always trust-able, since we are using server ip now to make the request instead of client ip, there can be specific ip based restriction on the external resource, which can lead to misleading results.

            We can implement a new setting in the drop down called "iframe", but that might lead to following issues:-

            • Accessibility concerns, Frames in general has bunch of accessibility issues associated with them and should be avoided.
            • Embed and Iframe options can be pretty confusing for common users.

            To conclude, I don't see any reasonable solution to this issue.
            Cheers
            Ankit

            Show
            ankit_frenz Ankit Agarwal added a comment - Hi Rosie, There is bunch of issues with current approach:- fopen is disabled on most servers for remote fetching We can use curl alternatively, but it will require a lot of processing, parsing https flags, get parameters etc It is one additional request each time someone visits the url resource, which can be a huge performance hit, as it is making an external request. fopen and curl are not always trust-able, since we are using server ip now to make the request instead of client ip, there can be specific ip based restriction on the external resource, which can lead to misleading results. We can implement a new setting in the drop down called "iframe", but that might lead to following issues:- Accessibility concerns, Frames in general has bunch of accessibility issues associated with them and should be avoided. Embed and Iframe options can be pretty confusing for common users. To conclude, I don't see any reasonable solution to this issue. Cheers Ankit
            Hide
            ankit_frenz Ankit Agarwal added a comment -

            Forgot to click the button
            Thanks

            Show
            ankit_frenz Ankit Agarwal added a comment - Forgot to click the button Thanks
            Hide
            rwijaya Rossiani Wijaya added a comment -

            Thank you Ankit for reviewing.

            Adding Sam for his input regarding this.

            Show
            rwijaya Rossiani Wijaya added a comment - Thank you Ankit for reviewing. Adding Sam for his input regarding this.
            Hide
            samhemelryk Sam Hemelryk added a comment -

            Hi guys,

            I've just had a quick at this.
            My initial reaction is that fopen is not great, as Ankit notes fopen utilising protocols can be disabled.
            Also I don't like that this fopen call would be made for each call to resourcelib_embed_general, thats going to lead to it for every view of a resouce + url, and is going to be socketed death when rebuilding course modinfo (the call path leads back into modinfolib via url_get_coursemodule_info). That would lead to a fopen call being made for each resource in a course when creating the modinfo data.

            Looking back at what Darko is asking for, he is after a setting to force the use of an iframe.
            This sounds like a pretty smart idea to me, although I was not involved in the usability and accessibility work done in this area.
            But you could easilyi implement a new URL resource module setting to force the use of an iframe when embedding a page, and then within url_get_final_display_type check for such a setting and return a new const RESOURCELIB_DISPLAY_IFRAME. Then look for and deal with that explicitly in view.php. You could even move the iframe code from resourcelib_embed_general into a new function resourcelib_embed_iframe and make view.php and resourcelib_embed_general call that as required.
            On an interesting note the iframe element is included in the HTML working draft, notes as a nested browsing context, no doubt when we eventually get to HTML5 we will be moving away from the object tag back to the iframe for text/html mime types.

            Alternatively you could easily specify an alternative to display within the object when the objects content cannot be loaded.
            I had a quick test and you can even embed a secondary object tag in there.
            However a simple "URL not available" so something similar may suffice?
            What do you guys think of that? Would it meet your needs Darko?
            I'm know very little about objects, I've tested to make sure it si valid and sure enough it is fine, so perhaps that simple.
            Or the setting above.

            Either way there definitely would be a solution to this, and it is most certainly a bug as the output when the requested page doesn't exist is useless.

            Many thanks
            Sam

            Show
            samhemelryk Sam Hemelryk added a comment - Hi guys, I've just had a quick at this. My initial reaction is that fopen is not great, as Ankit notes fopen utilising protocols can be disabled. Also I don't like that this fopen call would be made for each call to resourcelib_embed_general, thats going to lead to it for every view of a resouce + url, and is going to be socketed death when rebuilding course modinfo (the call path leads back into modinfolib via url_get_coursemodule_info). That would lead to a fopen call being made for each resource in a course when creating the modinfo data. Looking back at what Darko is asking for, he is after a setting to force the use of an iframe. This sounds like a pretty smart idea to me, although I was not involved in the usability and accessibility work done in this area. But you could easilyi implement a new URL resource module setting to force the use of an iframe when embedding a page, and then within url_get_final_display_type check for such a setting and return a new const RESOURCELIB_DISPLAY_IFRAME. Then look for and deal with that explicitly in view.php. You could even move the iframe code from resourcelib_embed_general into a new function resourcelib_embed_iframe and make view.php and resourcelib_embed_general call that as required. On an interesting note the iframe element is included in the HTML working draft, notes as a nested browsing context, no doubt when we eventually get to HTML5 we will be moving away from the object tag back to the iframe for text/html mime types. Alternatively you could easily specify an alternative to display within the object when the objects content cannot be loaded. I had a quick test and you can even embed a secondary object tag in there. However a simple "URL not available" so something similar may suffice? What do you guys think of that? Would it meet your needs Darko? I'm know very little about objects, I've tested to make sure it si valid and sure enough it is fine, so perhaps that simple. Or the setting above. Either way there definitely would be a solution to this, and it is most certainly a bug as the output when the requested page doesn't exist is useless. Many thanks Sam
            Hide
            rwijaya Rossiani Wijaya added a comment - - edited

            Thanks Sam for your feedback.

            My +1 for adding new setting for iFrame, if it is really necessary to have.

            Show
            rwijaya Rossiani Wijaya added a comment - - edited Thanks Sam for your feedback. My +1 for adding new setting for iFrame, if it is really necessary to have.
            Hide
            ankit_frenz Ankit Agarwal added a comment -

            Hi Sam,
            I guess Iframes are not recommended considering accessibility. Still if everybody thinks it is a good idea, than we can go ahead with that.

            About the alternative text thing, are you suggesting to edit the "Click xxx link to open resource." to something more like " Cannot embed the url, Click xxxx link to open resource." ?

            Cheers

            Show
            ankit_frenz Ankit Agarwal added a comment - Hi Sam, I guess Iframes are not recommended considering accessibility. Still if everybody thinks it is a good idea, than we can go ahead with that. About the alternative text thing, are you suggesting to edit the "Click xxx link to open resource." to something more like " Cannot embed the url, Click xxxx link to open resource." ? Cheers
            Hide
            samhemelryk Sam Hemelryk added a comment -

            No probs Rosie,
            Ankit if it becomes a setting the accessibility is less of a concern. We can add descriptive help that suggests the usability of an iframe isn't ideal but at the end of the day if people decide to enable it it would be a conscious decision by them to use the iframe, hopefully they will have considered their need to turn it on anyway.

            In regards to the alternative text, using a simple string like "Unable to load the requested URL" would be OK I think.
            Although ideally, and perhaps worth considering I'd suggest creating a url missing page within the URL resource that contained the text, and then using a nested object to display that as the alt (90% sure that works OK in all browsers we support).
            The advantage to that is that then we can make the missing page script log an event for the missing request (assuming here that the alt object doesn't get loaded unless needed, we'd need to test of course).

            Alternatively to both of those you could let the user decide as a setting of the URL module, although I would look to the above options first. This would work, but would be beating round the bush... why would you add a resource that could not be reached.

            Cheers
            Sam

            Show
            samhemelryk Sam Hemelryk added a comment - No probs Rosie, Ankit if it becomes a setting the accessibility is less of a concern. We can add descriptive help that suggests the usability of an iframe isn't ideal but at the end of the day if people decide to enable it it would be a conscious decision by them to use the iframe, hopefully they will have considered their need to turn it on anyway. In regards to the alternative text, using a simple string like "Unable to load the requested URL" would be OK I think. Although ideally, and perhaps worth considering I'd suggest creating a url missing page within the URL resource that contained the text, and then using a nested object to display that as the alt (90% sure that works OK in all browsers we support). The advantage to that is that then we can make the missing page script log an event for the missing request (assuming here that the alt object doesn't get loaded unless needed, we'd need to test of course). Alternatively to both of those you could let the user decide as a setting of the URL module, although I would look to the above options first. This would work, but would be beating round the bush... why would you add a resource that could not be reached. Cheers Sam
            Hide
            salvetore Michael de Raadt added a comment -

            OK. So updating the text seems to be a positive step forward now.

            I think that if iframe is redeemed in future and becomes acceptable as part of HTML5 then we should use it, but until then the current solution is probably best.

            I'm not in favour of providing an option for the user to decide between an object or an iframe. It makes sense to us, but it would be meaningless to regular users.

            Show
            salvetore Michael de Raadt added a comment - OK. So updating the text seems to be a positive step forward now. I think that if iframe is redeemed in future and becomes acceptable as part of HTML5 then we should use it, but until then the current solution is probably best. I'm not in favour of providing an option for the user to decide between an object or an iframe. It makes sense to us, but it would be meaningless to regular users.
            Hide
            rwijaya Rossiani Wijaya added a comment -

            Note: there is a glitch on displaying a loadable url in Master. The content page flicks when it gets display (eg: page is viewable then it disappears). I will create new issue to address the issue.

            Show
            rwijaya Rossiani Wijaya added a comment - Note: there is a glitch on displaying a loadable url in Master. The content page flicks when it gets display (eg: page is viewable then it disappears). I will create new issue to address the issue.
            Show
            rwijaya Rossiani Wijaya added a comment - Optional solution: https://github.com/rwijaya/moodle/compare/master...MDL-30769_withpage
            Hide
            ankit_frenz Ankit Agarwal added a comment -

            First solution looks good.
            About second solution:-

            1. $id should be a required param in the page because you are using $cm->course later which wont be set if $id is not present
            2. there is no need of empty($id) check incase we make it a required param
            3. array('id'=>$cm->course) needs spaces
            4. array('id'=>$cm->instance) needs spaces
            5. you cannot use get_string('missingpage', 'url') as $action for add_to_log(), I am not sure what appropriate action it is. Normally we use view,edit,delete,update etc, but view doesnt help us notice that it is 404 page. It will be good to get some ideas on this.
            6. you will also need to define a new rule in log.php
            7. and increase version of url module to recognize that change.
            8. I am not sure if we need any header , footer and other things in this page. It will be good to get Sam's opinion on that.

            Thanks

            Show
            ankit_frenz Ankit Agarwal added a comment - First solution looks good. About second solution:- $id should be a required param in the page because you are using $cm->course later which wont be set if $id is not present there is no need of empty($id) check incase we make it a required param array('id'=>$cm->course) needs spaces array('id'=>$cm->instance) needs spaces you cannot use get_string('missingpage', 'url') as $action for add_to_log(), I am not sure what appropriate action it is. Normally we use view,edit,delete,update etc, but view doesnt help us notice that it is 404 page. It will be good to get some ideas on this. you will also need to define a new rule in log.php and increase version of url module to recognize that change. I am not sure if we need any header , footer and other things in this page. It will be good to get Sam's opinion on that. Thanks
            Hide
            ankit_frenz Ankit Agarwal added a comment -

            Hi Rosie,
            Both patchs look good to me now.

            Thanks for the changes.
            Feel free to submit for integration.
            Thanks

            Show
            ankit_frenz Ankit Agarwal added a comment - Hi Rosie, Both patchs look good to me now. Thanks for the changes. Feel free to submit for integration. Thanks
            Hide
            rwijaya Rossiani Wijaya added a comment -

            Thanks Ankit for reviewing.

            I'm submitting optional solution (embadded page) for integration.

            Also, assigning Sam as integrator

            Submitting for integration review.

            Show
            rwijaya Rossiani Wijaya added a comment - Thanks Ankit for reviewing. I'm submitting optional solution (embadded page) for integration. Also, assigning Sam as integrator Submitting for integration review.
            Hide
            samhemelryk Sam Hemelryk added a comment -

            Hi Rossi,

            Sorry I am reopening this, its only over a very minor point but as it is Wednesday and it is security related I have no other option.

            I like the overall implementation, what I noted was the use of the msg param within the missing page script.
            It is being picked up as PARAM_RAW and then used directly in the page.
            That is a HUGE security risk as you could write anything you wanted into it have have it echo'd into the page.
            PARAM_RAW cleans data for use, it doesn't strip HTML and could leave people open to many different attacks there. You could construct quite the nasty link with that page presently.
            In thinking about it I don't see that need for that at the moment, perhaps sometime in the future someone will request it as a feature and we can add it to master, but for the time being I think it is better to leave it out entirely, it will make it easier to call this a bug fix as we're not introducing any functionality not required in order to fix the bug.

            Seeing as its back anyway could you please just confirm that you have tested this in all supported browsers.

            Many thanks
            Sam

            Show
            samhemelryk Sam Hemelryk added a comment - Hi Rossi, Sorry I am reopening this, its only over a very minor point but as it is Wednesday and it is security related I have no other option. I like the overall implementation, what I noted was the use of the msg param within the missing page script. It is being picked up as PARAM_RAW and then used directly in the page. That is a HUGE security risk as you could write anything you wanted into it have have it echo'd into the page. PARAM_RAW cleans data for use, it doesn't strip HTML and could leave people open to many different attacks there. You could construct quite the nasty link with that page presently. In thinking about it I don't see that need for that at the moment, perhaps sometime in the future someone will request it as a feature and we can add it to master, but for the time being I think it is better to leave it out entirely, it will make it easier to call this a bug fix as we're not introducing any functionality not required in order to fix the bug. Seeing as its back anyway could you please just confirm that you have tested this in all supported browsers. Many thanks Sam
            Hide
            cibot CiBoT added a comment -

            Moving this reopened issue out from current integration. Please, re-submit it for integration once ready.

            Show
            cibot CiBoT added a comment - Moving this reopened issue out from current integration. Please, re-submit it for integration once ready.
            Hide
            rwijaya Rossiani Wijaya added a comment -

            Hi Sam,

            I made the changes according to your suggestion. However, removing $msg optional param, will limit the access of accessing the page manually (the url link will not display on the page).

            Therefore I created an alternative solution for this. Instead of using optional_param ($msg, '', PARAM_RAW), it would probably better to use optional_param($link, '', PARAM_URL) then print it out the link (just like the value for $clicktoopen). I know it seems like a redundancy for $clicktoopen, but at least the URL is available to be access manually.

            Let me know what do you think about the new patch.

            Thanks
            Rosie

            Updated patch for 2.2, 2.3 and 2.4.

            Show
            rwijaya Rossiani Wijaya added a comment - Hi Sam, I made the changes according to your suggestion. However, removing $msg optional param, will limit the access of accessing the page manually (the url link will not display on the page). Therefore I created an alternative solution for this. Instead of using optional_param ($msg, '', PARAM_RAW), it would probably better to use optional_param($link, '', PARAM_URL) then print it out the link (just like the value for $clicktoopen). I know it seems like a redundancy for $clicktoopen, but at least the URL is available to be access manually. Let me know what do you think about the new patch. Thanks Rosie Updated patch for 2.2, 2.3 and 2.4.
            Hide
            rwijaya Rossiani Wijaya added a comment -

            Side note:

            I tested the patch in IE (7, 8, and 9), instead of displaying the 'missing' page, it displays the browser 404 page. *see attachment

            In Firefox, Chrome and Opera for both Windows and Ubuntu, the page embedded the 'missing' page correctly. *see attachment

            Show
            rwijaya Rossiani Wijaya added a comment - Side note: I tested the patch in IE (7, 8, and 9), instead of displaying the 'missing' page, it displays the browser 404 page. *see attachment In Firefox, Chrome and Opera for both Windows and Ubuntu, the page embedded the 'missing' page correctly. *see attachment
            Hide
            rwijaya Rossiani Wijaya added a comment -

            Re-submit for integration.

            Show
            rwijaya Rossiani Wijaya added a comment - Re-submit for integration.
            Hide
            poltawski Dan Poltawski added a comment -

            The integration of this issue has been delayed to next week because the integration period is over (Monday, Tuesday) and testing must happen on Wednesday.
            This change to a more rigid timeframe on each integration/testing cycle aims to produce a better and clear separation and organization of tasks for everybody.
            This is a bulk-automated message, so if you want to blame somebody/thing/where, don't do it here (use git instead) :-D :-P
            Apologizes for the inconvenient, this will be integrated next week. Thanks for your collaboration & ciao

            Show
            poltawski Dan Poltawski added a comment - The integration of this issue has been delayed to next week because the integration period is over (Monday, Tuesday) and testing must happen on Wednesday. This change to a more rigid timeframe on each integration/testing cycle aims to produce a better and clear separation and organization of tasks for everybody. This is a bulk-automated message, so if you want to blame somebody/thing/where, don't do it here (use git instead) :-D :-P Apologizes for the inconvenient, this will be integrated next week. Thanks for your collaboration & ciao
            Hide
            nebgor Aparup Banerjee 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
            nebgor Aparup Banerjee 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
            poltawski Dan Poltawski 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
            poltawski Dan Poltawski 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
            poltawski Dan Poltawski added a comment -

            Minor comment: global $DB; is not necessary in mod/url/missingpage.php

            I have some concerns about adding this log entry, because its only applicable when we embed a page, and well it might give the false impression that the resource module is checking that links are alive.

            Show
            poltawski Dan Poltawski added a comment - Minor comment: global $DB; is not necessary in mod/url/missingpage.php I have some concerns about adding this log entry, because its only applicable when we embed a page, and well it might give the false impression that the resource module is checking that links are alive.
            Hide
            poltawski Dan Poltawski added a comment -

            I'm refocusing on 2.4dev issues integrating so unassinging myself as integrator

            Show
            poltawski Dan Poltawski added a comment - I'm refocusing on 2.4dev issues integrating so unassinging myself as integrator
            Hide
            samhemelryk Sam Hemelryk added a comment -

            Hi Rosie,

            I've been playing around with this for a wee while now.
            In general I like the effect but I've noticed one gotcha to this.
            The missing page is ALWAYS loaded, regardless of whether the URL was loaded or not.
            It may be browser specific I don't know but I was using Firefox myself.
            The disadvantage to this is that all URL requests will load two pages, the missing page and the actual resource.
            Because the missing page is always loaded the log event is always added. Not a great outcome.
            Perhaps we could live with always loading it, but certainly the double log entries isn't a good outcome.
            I think we need to think about this one a bit more sorry.

            Many thanks
            Sam

            Show
            samhemelryk Sam Hemelryk added a comment - Hi Rosie, I've been playing around with this for a wee while now. In general I like the effect but I've noticed one gotcha to this. The missing page is ALWAYS loaded, regardless of whether the URL was loaded or not. It may be browser specific I don't know but I was using Firefox myself. The disadvantage to this is that all URL requests will load two pages, the missing page and the actual resource. Because the missing page is always loaded the log event is always added. Not a great outcome. Perhaps we could live with always loading it, but certainly the double log entries isn't a good outcome. I think we need to think about this one a bit more sorry. Many thanks Sam
            Hide
            cibot CiBoT added a comment -

            Moving this reopened issue out from current integration. Please, re-submit it for integration once ready.

            Show
            cibot CiBoT added a comment - Moving this reopened issue out from current integration. Please, re-submit it for integration once ready.
            Hide
            rwijaya Rossiani Wijaya added a comment -

            Adding Petr as watcher.

            Hi Petr,

            could you provide some feedback on how should we fix this issue?

            Thanks
            Rosie

            Show
            rwijaya Rossiani Wijaya added a comment - Adding Petr as watcher. Hi Petr, could you provide some feedback on how should we fix this issue? Thanks Rosie
            Hide
            skodak Petr Skoda added a comment -

            Hello, I do not understand this much, but why should you put object into object? Why not just put there a message indicating that either browser can not display the object or the object does not exist? Btw is there a way to find out if object was loaded or not? Maybe we could diagnose the loading from Javascript code...

            Show
            skodak Petr Skoda added a comment - Hello, I do not understand this much, but why should you put object into object? Why not just put there a message indicating that either browser can not display the object or the object does not exist? Btw is there a way to find out if object was loaded or not? Maybe we could diagnose the loading from Javascript code...
            Hide
            skodak Petr Skoda added a comment -

            I am going to do a bit testing later today and I will try to remember more of the mod_resource history...

            Show
            skodak Petr Skoda added a comment - I am going to do a bit testing later today and I will try to remember more of the mod_resource history...
            Hide
            skodak Petr Skoda added a comment -

            I think this is a nice example why we should use iframe instead of object tag, it is the good old way that is now again recommended in HTML5 (default doctype since Moodle 2.4). The following patch seemed to work fine for me:

            https://github.com/skodak/moodle/compare/009f190...wip_MDL-30769_m25_iframe

            Show
            skodak Petr Skoda added a comment - I think this is a nice example why we should use iframe instead of object tag, it is the good old way that is now again recommended in HTML5 (default doctype since Moodle 2.4). The following patch seemed to work fine for me: https://github.com/skodak/moodle/compare/009f190...wip_MDL-30769_m25_iframe
            Hide
            rwijaya Rossiani Wijaya added a comment -

            Hi Petr,

            Thank you for the feedback and patch.

            Patch looks great and I backported to 2.4 and 2.3.

            Submitting for integration review.

            Show
            rwijaya Rossiani Wijaya added a comment - Hi Petr, Thank you for the feedback and patch. Patch looks great and I backported to 2.4 and 2.3. Submitting for integration review.
            Hide
            poltawski Dan Poltawski 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
            poltawski Dan Poltawski 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
            skodak Petr Skoda added a comment -

            Hello, I am not sure if it is ok to backport to 2.3 because it is still supposed to be XHTML Strict 1.0. Thanks anyway.

            Show
            skodak Petr Skoda added a comment - Hello, I am not sure if it is ok to backport to 2.3 because it is still supposed to be XHTML Strict 1.0. Thanks anyway.
            Hide
            poltawski Dan Poltawski added a comment -

            Ok, integrated to master and 24 only.

            While it would be nice to have a solution for 2.3, I really do not think it is worth spending any more time on this issue. There are bigger problems to solve.

            Show
            poltawski Dan Poltawski added a comment - Ok, integrated to master and 24 only. While it would be nice to have a solution for 2.3, I really do not think it is worth spending any more time on this issue. There are bigger problems to solve.
            Hide
            dmonllao David Monllaó added a comment -

            It passes, tested in 24 and master

            Show
            dmonllao David Monllaó added a comment - It passes, tested in 24 and master
            Hide
            stronk7 Eloy Lafuente (stronk7) added a comment -

            Surely you will be happy to know that your code is now part of Moodle upstream. Thanks, thanks!

            Closing as fixed, ciao

            Show
            stronk7 Eloy Lafuente (stronk7) added a comment - Surely you will be happy to know that your code is now part of Moodle upstream. Thanks, thanks! Closing as fixed, ciao

              People

              • Votes:
                1 Vote for this issue
                Watchers:
                5 Start watching this issue

                Dates

                • Created:
                  Updated:
                  Resolved:
                  Fix Release Date:
                  11/Mar/13