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

Use get_file_url instead of manual slasharguments check

    Details

    • Type: Bug
    • Status: Closed
    • Priority: Major
    • Resolution: Fixed
    • Affects Version/s: 1.8, 1.9
    • Fix Version/s: 1.9.2
    • Component/s: General, RSS
    • Labels:
      None
    • Affected Branches:
      MOODLE_18_STABLE, MOODLE_19_STABLE
    • Fixed Branches:
      MOODLE_19_STABLE

      Description

      To fix MDL-13792, the function get_file_url was added. This bug aims to make use of it in every possible place. I will send in a big patch later (starting with 1.8) for review.

      There is a problem with links to rss files, as most of them are to /rss/file.php, not /file.php. See /blog/rsslib.php, line 41, for one case of this. Possibilities :
      1) don't do anything, xml files shouldn't have non-ascii file names anyway
      2) modify get_file_url to make a link to /rss/file.php in these cases
      3) make a different function for rss links

      I vote for #2. Petr? Eloy? What do you think?

        Gliffy Diagrams

        1. patch-get_file_url-18.txt
          30 kB
          Mathieu Petit-Clair
        2. patch-get_file_url-19_20080707.txt
          43 kB
          Mathieu Petit-Clair
        3. patch-get_file_url-19.txt
          31 kB
          Mathieu Petit-Clair

          Issue Links

            Activity

            Hide
            scyrma Mathieu Petit-Clair added a comment -

            About rss stuff, I answered my own question. I'll modify the function as it should, it's actually kinda obvious.. (sorry about the noise)

            Show
            scyrma Mathieu Petit-Clair added a comment - About rss stuff, I answered my own question. I'll modify the function as it should, it's actually kinda obvious.. (sorry about the noise)
            Hide
            scyrma Mathieu Petit-Clair added a comment -

            Please have a look at the file...

            Show
            scyrma Mathieu Petit-Clair added a comment - Please have a look at the file...
            Hide
            skodak Petr Skoda added a comment -

            stuff like this should not go into 1.8 imo

            Show
            skodak Petr Skoda added a comment - stuff like this should not go into 1.8 imo
            Hide
            scyrma Mathieu Petit-Clair added a comment -

            That's where the original bug was reported and that's what I was trying to fix (MDL-13792). Why shouldn't that go into 1.8?

            Show
            scyrma Mathieu Petit-Clair added a comment - That's where the original bug was reported and that's what I was trying to fix ( MDL-13792 ). Why shouldn't that go into 1.8?
            Hide
            skodak Petr Skoda added a comment -
            • utf-8 is not officially supported in file names and will not be until 2.0 (or later)
            • we did not backport many more serious bugs into 1.8.x

            To be 100% correct we should also bump up the requires version in modules.
            Could you please make a patch against 19?

            Show
            skodak Petr Skoda added a comment - utf-8 is not officially supported in file names and will not be until 2.0 (or later) we did not backport many more serious bugs into 1.8.x To be 100% correct we should also bump up the requires version in modules. Could you please make a patch against 19?
            Hide
            stronk7 Eloy Lafuente (stronk7) added a comment -

            I like the idea of those all $CFG->slahsarguments conditions centralised in the new function. And 100% agree wit Petr about 1.9 being the proper target. Not sure if we need requires bumps (AFAIK we bumped before release, isn't it?). Easy to check, anyway.

            Ciao

            Show
            stronk7 Eloy Lafuente (stronk7) added a comment - I like the idea of those all $CFG->slahsarguments conditions centralised in the new function. And 100% agree wit Petr about 1.9 being the proper target. Not sure if we need requires bumps (AFAIK we bumped before release, isn't it?). Easy to check, anyway. Ciao
            Hide
            scyrma Mathieu Petit-Clair added a comment -

            Here's a version of the patch against MOODLE_19_STABLE. I did this carefully, but I can't possibly test all cases, so .. this needs some more eyes.

            Show
            scyrma Mathieu Petit-Clair added a comment - Here's a version of the patch against MOODLE_19_STABLE. I did this carefully, but I can't possibly test all cases, so .. this needs some more eyes.
            Hide
            bear Bill Burgos added a comment -

            Mat,

            We are still having problems with Link to File Resource. Log snippet below:

            192.168.0.1 - - [22/May/2008:16:44:19 +0900] "GET /mod/resource/view.php?id=3 HTTP/1.1" 303 232 "http://example.com/course/view.php?id=2" "Mozilla/4.0 (compatible; MSIE 6.0; Windows NT 5.0)"
            192.168.0.1 - - [22/May/2008:16:44:19 +0900] "GET /file.php/2/QA\xe7\xae\xa1\xe7\x90\x81E.xls HTTP/1.1" 404 6333 "http://example.com/course/view.php?id=2" "Mozilla/4.0 (compatible; MSIE 6.0; Windows NT 5.0)"
            192.168.0.1 - - [22/May/2008:16:44:20 +0900] "GET /theme/standardwhite/styles.php HTTP/1.1" 200 663 "http://example.com/file.php/2/QA\xe7\xae\xa1\xe7\x90\x81E.xls" "Mozilla/4.0 (compatible; MSIE 6.0; Windows NT 5.0)"
            192.168.0.1 - - [22/May/2008:16:44:20 +0900] "GET /theme/standard/styles.php HTTP/1.1" 200 121244 "http://example.com/file.php/2/QA\xe7\xae\xa1\xe7\x90\x81E.xls" "Mozilla/4.0 (compatible; MSIE 6.0; Windows NT 5.0)"

            Show
            bear Bill Burgos added a comment - Mat, We are still having problems with Link to File Resource. Log snippet below: 192.168.0.1 - - [22/May/2008:16:44:19 +0900] "GET /mod/resource/view.php?id=3 HTTP/1.1" 303 232 "http://example.com/course/view.php?id=2" "Mozilla/4.0 (compatible; MSIE 6.0; Windows NT 5.0)" 192.168.0.1 - - [22/May/2008:16:44:19 +0900] "GET /file.php/2/QA\xe7\xae\xa1\xe7\x90\x81E.xls HTTP/1.1" 404 6333 "http://example.com/course/view.php?id=2" "Mozilla/4.0 (compatible; MSIE 6.0; Windows NT 5.0)" 192.168.0.1 - - [22/May/2008:16:44:20 +0900] "GET /theme/standardwhite/styles.php HTTP/1.1" 200 663 "http://example.com/file.php/2/QA\xe7\xae\xa1\xe7\x90\x81E.xls" "Mozilla/4.0 (compatible; MSIE 6.0; Windows NT 5.0)" 192.168.0.1 - - [22/May/2008:16:44:20 +0900] "GET /theme/standard/styles.php HTTP/1.1" 200 121244 "http://example.com/file.php/2/QA\xe7\xae\xa1\xe7\x90\x81E.xls" "Mozilla/4.0 (compatible; MSIE 6.0; Windows NT 5.0)"
            Hide
            scyrma Mathieu Petit-Clair added a comment -

            Here's a new version of the patch, for 1.9. This one corrects (afaics) all calls using $CFG->slasharguments, adding new "types" to get_file_url where necessary.

            I did go over it a few times, but it needs more reviews. Petr, Eloy?

            Show
            scyrma Mathieu Petit-Clair added a comment - Here's a new version of the patch, for 1.9. This one corrects (afaics) all calls using $CFG->slasharguments, adding new "types" to get_file_url where necessary. I did go over it a few times, but it needs more reviews. Petr, Eloy?
            Hide
            dougiamas Martin Dougiamas added a comment -

            I had a read through the patch without spotting anything out of place ... it looks OK to me. I think we should put it in and test it wider today before the release tomorrow.

            Show
            dougiamas Martin Dougiamas added a comment - I had a read through the patch without spotting anything out of place ... it looks OK to me. I think we should put it in and test it wider today before the release tomorrow.
            Hide
            dougiamas Martin Dougiamas added a comment -

            Reminder: update $versions in modules where necessary

            Show
            dougiamas Martin Dougiamas added a comment - Reminder: update $versions in modules where necessary
            Hide
            smithrn Ryan Smith added a comment -

            I'm not sure if this bug caused this, but I updated this morning and got the new htmlarea.php and coursefiles.php and I now see no directory structure in the insert image section of the editor.

            [Thu Jul 10 09:24:27 2008] [error] [client 137.x.x.x] PHP Parse error: syntax error, unexpected T_VARIABLE in C:\\Apache2\\htdocs\\moodle\\lib\\editor\\htmlarea
            coursefiles.php on line 758, referer: moodle/lib/editor/htmlarea/popups/insert_image.php?id=1
            [Thu Jul 10 09:24:46 2008] [error] [client 137.x.x.x] PHP Parse error: syntax error, unexpected T_VARIABLE in C:\\Apache2\\htdocs\\moodle\\lib\\editor\\htmlarea
            coursefiles.php on line 758, referer: moodle/lib/editor/htmlarea/popups/insert_image.php?id=1
            [Thu Jul 10 09:24:50 2008] [error] [client 137.x.x.x] PHP Parse error: syntax error, unexpected T_VARIABLE in C:\\Apache2\\htdocs\\moodle\\lib\\editor\\htmlarea
            coursefiles.php on line 758, referer: moodle/lib/editor/htmlarea/popups/insert_image.php?id=1

            I apologize if this isn't the correct bug to report this. Thanks.

            Show
            smithrn Ryan Smith added a comment - I'm not sure if this bug caused this, but I updated this morning and got the new htmlarea.php and coursefiles.php and I now see no directory structure in the insert image section of the editor. [Thu Jul 10 09:24:27 2008] [error] [client 137.x.x.x] PHP Parse error: syntax error, unexpected T_VARIABLE in C:\\Apache2\\htdocs\\moodle\\lib\\editor\\htmlarea coursefiles.php on line 758, referer: moodle/lib/editor/htmlarea/popups/insert_image.php?id=1 [Thu Jul 10 09:24:46 2008] [error] [client 137.x.x.x] PHP Parse error: syntax error, unexpected T_VARIABLE in C:\\Apache2\\htdocs\\moodle\\lib\\editor\\htmlarea coursefiles.php on line 758, referer: moodle/lib/editor/htmlarea/popups/insert_image.php?id=1 [Thu Jul 10 09:24:50 2008] [error] [client 137.x.x.x] PHP Parse error: syntax error, unexpected T_VARIABLE in C:\\Apache2\\htdocs\\moodle\\lib\\editor\\htmlarea coursefiles.php on line 758, referer: moodle/lib/editor/htmlarea/popups/insert_image.php?id=1 I apologize if this isn't the correct bug to report this. Thanks.
            Hide
            smithrn Ryan Smith added a comment -

            Something is wrong in moodle/files/index.php now as well, when I try to download a file from the course files area I now get:

            You don't have permission to access /moodlehttp://moodle/file.php/286/backupdata/backup-ejhs-cb1-20080519-2322.zip on this server.

            Show
            smithrn Ryan Smith added a comment - Something is wrong in moodle/files/index.php now as well, when I try to download a file from the course files area I now get: You don't have permission to access /moodlehttp://moodle/file.php/286/backupdata/backup-ejhs-cb1-20080519-2322.zip on this server.
            Hide
            scyrma Mathieu Petit-Clair added a comment -

            Thanks for the report, Ryan. I've just sent in fixes for these.

            Show
            scyrma Mathieu Petit-Clair added a comment - Thanks for the report, Ryan. I've just sent in fixes for these.
            Hide
            smithrn Ryan Smith added a comment -

            Mathieu,

            I just tested the updated files and they seem to work perfectly. Thanks for fixing it!

            Show
            smithrn Ryan Smith added a comment - Mathieu, I just tested the updated files and they seem to work perfectly. Thanks for fixing it!

              People

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

                Dates

                • Created:
                  Updated:
                  Resolved:
                  Fix Release Date:
                  11/Jul/08