Moodle
  1. Moodle
  2. MDL-29161

Square brackets in url parameter are removed by moodle_url

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: 2.0.4, 2.1.4, 2.2.1, 2.3
    • Fix Version/s: 2.1.5, 2.2.2
    • Component/s: Libraries
    • Labels:
    • Rank:
      18706

      Description

      With PHP, it is possible to pass an array as URL parameter like this:
      http://www.yourphpsite.com/index.php?foo[]=bar&foo[]=bar2

      I'm not sure if that kind of URL is RFC compliant, but at least it works and is sometimes used by delevopers.
      I realized now that the square brackets in such a URL are removed when they are fed into a moodle_url class.

      To reproduce this:

      • Add a rss reader block to your course
      • Add the rss feed http://www.uni-ulm.de/index.php?id=10794 to the rss reader. This feed is based on the ttnews extension for typo3.
      • View the feed entries in the block

      Links to a feed entry in this feed look like this:
      http://www.uni-ulm.de/index.php?id=10719&tx_ttnews[tt_news]=7245&cHash=ac6e21dc232f5607fd3265ebd4339758
      but they are displayed like this:
      http://www.uni-ulm.de/index.php?id=10719&tx_ttnews=&cHash=ac6e21dc232f5607fd3265ebd4339758

      Additionally, there is a debug message in apache log which says:
      Warning: rawurlencode() expects parameter 1 to be string, array given in /pathtomoodle/lib/weblib.php on line 497 Warning: rawurlencode() expects parameter 1 to be string, array given in /pathtomoodle/lib/weblib.php on line 497

      I would be grateful if you could check if URLs like these should be allowed in moodle_url class or not.

        Activity

        Hide
        Michael de Raadt added a comment -

        Thanks for reporting this.

        I've put it on our backlog and we'll try to get to it as soon as we can.

        In the meantime adding more information, such as screenshots, a workaround or even a code solution, will help us and other users. If you do add a solution, please add a 'patch' label.

        Show
        Michael de Raadt added a comment - Thanks for reporting this. I've put it on our backlog and we'll try to get to it as soon as we can. In the meantime adding more information, such as screenshots, a workaround or even a code solution, will help us and other users. If you do add a solution, please add a 'patch' label.
        Hide
        Alexander Bias added a comment -

        I had a look at this again.

        The moodle_url class seems to handle array parameters in most of its functions. Array parameters aren't supported, but at least they are handled and throw a coding exception.

        Now, in get_query_string() array parameters aren't handled completely which produces the error message mentioned above. I think there has to be a check compared to the check in merge_overrideparams().

        moodle_url class supporting array parameters would be far better, of course, but I think that would be much work in moodle_url class.

        Show
        Alexander Bias added a comment - I had a look at this again. The moodle_url class seems to handle array parameters in most of its functions. Array parameters aren't supported, but at least they are handled and throw a coding exception. Now, in get_query_string() array parameters aren't handled completely which produces the error message mentioned above. I think there has to be a check compared to the check in merge_overrideparams(). moodle_url class supporting array parameters would be far better, of course, but I think that would be much work in moodle_url class.
        Hide
        Tim Hunt added a comment -

        I just encountered this problem, and came up with a fix. Submitting for integration.

        Show
        Tim Hunt added a comment - I just encountered this problem, and came up with a fix. Submitting for integration.
        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
        Petr Škoda added a comment -

        hmm, should not the ['.$index.'] be encoded too? what if there is some weird character there?

        Show
        Petr Škoda added a comment - hmm, should not the ['.$index.'] be encoded too? what if there is some weird character there?
        Hide
        Petr Škoda added a comment -

        oops, it is encoded together with the key, sorry....

        Show
        Petr Škoda added a comment - oops, it is encoded together with the key, sorry....
        Hide
        Tim Hunt added a comment -

        No worries. Thanks for your peer-review.

        Show
        Tim Hunt added a comment - No worries. Thanks for your peer-review.
        Hide
        Eloy Lafuente (stronk7) added a comment -

        The only case that will be failing, if tested is, exactly the one you typed above, isn't it?

        http://example.com/index.php?foo[]=bar&foo[]=bar2
        

        ... because numerical indexes will be added always. just guessing if we could differentiate between "string" keys and "empty/integer" ones, or PHP prevents us to make any difference. Just in order to make the example above to ->out() exactly the same than the original url.

        Holding this for some hours... ciao

        Show
        Eloy Lafuente (stronk7) added a comment - The only case that will be failing, if tested is, exactly the one you typed above, isn't it? http: //example.com/index.php?foo[]=bar&foo[]=bar2 ... because numerical indexes will be added always. just guessing if we could differentiate between "string" keys and "empty/integer" ones, or PHP prevents us to make any difference. Just in order to make the example above to ->out() exactly the same than the original url. Holding this for some hours... ciao
        Hide
        Tim Hunt added a comment -

        Well, I did not really need that case. The case that was blocking my work had array keys.

        I suggest you integrate my fix, and then if you think more work is needed then open another issue.

        With my code. http://example.com/index.php?foo[]=bar&foo[]=bar2 will get changed to
        http://example.com/index.php?foo[0]=bar&foo[1]=bar2 which will have exactly the same effect when you call optional_param_array, so the change does not worry me.

        Show
        Tim Hunt added a comment - Well, I did not really need that case. The case that was blocking my work had array keys. I suggest you integrate my fix, and then if you think more work is needed then open another issue. With my code. http://example.com/index.php?foo[]=bar&foo[]=bar2 will get changed to http://example.com/index.php?foo[0]=bar&foo[1]=bar2 which will have exactly the same effect when you call optional_param_array, so the change does not worry me.
        Hide
        Eloy Lafuente (stronk7) added a comment -

        I think I'm going to add one more test with the un-keyed url, just to have a reference to it. No problem if that is the behavior we define as expected.

        Thanks and ciao

        Show
        Eloy Lafuente (stronk7) added a comment - I think I'm going to add one more test with the un-keyed url, just to have a reference to it. No problem if that is the behavior we define as expected. Thanks and ciao
        Hide
        Eloy Lafuente (stronk7) added a comment -

        Integrated, thanks! (21, 22 & master)

        Show
        Eloy Lafuente (stronk7) added a comment - Integrated, thanks! (21, 22 & master)
        Hide
        Eloy Lafuente (stronk7) added a comment -

        Passing, tested under 21 and master, all weblib tests (64) ok.

        Show
        Eloy Lafuente (stronk7) added a comment - Passing, tested under 21 and master, all weblib tests (64) ok.
        Hide
        Eloy Lafuente (stronk7) added a comment -

        Well,

        I wish I said it every time
        you do the things you do.
        You always lend a helping hand,
        and I'm filled with gratitude.

        You are strong and generous
        for each and everyone one of us.
        I am eternally grateful,
        I cannot say thanks enough.

        Sorry for the (un)cool bit above, lol. Closing this as fixed. Ciao

        Show
        Eloy Lafuente (stronk7) added a comment - Well, I wish I said it every time you do the things you do. You always lend a helping hand, and I'm filled with gratitude. You are strong and generous for each and everyone one of us. I am eternally grateful, I cannot say thanks enough. Sorry for the (un)cool bit above, lol. Closing this as fixed. Ciao
        Hide
        Nicholas Koeppen added a comment -

        I'm still confused on why there isn't support for adding array parameters to a moodle_url object. There is a optional_param_array() function, but I don't see the reverse supported. Why should there be a function to retrieve an array parameter, but none to add an array parameter to a url.

        e.g.

        $params = array('arrayparam' => array('param1' => 'val1', 'param2' => 'val2'), 'param3' => 'val3');
        $url = new moodle_url(SOMETHING, $params);

        Thanks for any help you can provide and I apologize if I missed the right method.

        Show
        Nicholas Koeppen added a comment - I'm still confused on why there isn't support for adding array parameters to a moodle_url object. There is a optional_param_array() function, but I don't see the reverse supported. Why should there be a function to retrieve an array parameter, but none to add an array parameter to a url. e.g. $params = array('arrayparam' => array('param1' => 'val1', 'param2' => 'val2'), 'param3' => 'val3'); $url = new moodle_url(SOMETHING, $params); Thanks for any help you can provide and I apologize if I missed the right method.
        Hide
        Petr Škoda added a comment -

        To me it seems that anybody who needs [] moodle_url parameters is not using recommended moodle coding style. I still did not see a valid use case for it except external URLs. Could you please show us REAL code that needs it?

        Show
        Petr Škoda added a comment - To me it seems that anybody who needs [] moodle_url parameters is not using recommended moodle coding style. I still did not see a valid use case for it except external URLs. Could you please show us REAL code that needs it?

          People

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

            Dates

            • Created:
              Updated:
              Resolved: