Moodle
  1. Moodle
  2. MDL-25468

"Coding Error Detected" message saving Page resources when Description box empty

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Blocker Blocker
    • Resolution: Fixed
    • Affects Version/s: 2.0
    • Fix Version/s: 2.0.2
    • Component/s: Resource
    • Labels:
    • Affected Branches:
      MOODLE_20_STABLE
    • Fixed Branches:
      MOODLE_20_STABLE
    • Rank:
      2828

      Description

      The error message is this and only happens on some resources. It seems to occur when editing an existing Page resource.... On saving the Page this is logged (and the page doesn't save)

      Default exception handler: Coding error detected, it must be fixed by a programmer: Url parameters values can not be arrays! Debug: \n

      • line 393 of /lib/weblib.php: coding_exception thrown
        \n* line 446 of /lib/weblib.php: call to moodle_url->params()\n
      • line 3402 of /lib/navigationlib.php: call to moodle_url->param()\n
      • line 3211 of /lib/navigationlib.php: call to settings_navigation->generate_user_settings()\n
      • line 2579 of /lib/navigationlib.php: call to settings_navigation->load_user_settings()\n
      • line 583 of /lib/pagelib.php: call to settings_navigation->initialise()\n
      • line 599 of /lib/pagelib.php: call to moodle_page->magic_get_settingsnav()\n
      • line 134 of /blocks/settings/block_settings.php: call to moodle_page->__get()\n
      • line 279 of /blocks/moodleblock.class.php: call to block_settings->get_content()\n
      • line 232 of /blocks/moodleblock.class.php: call to block_base->formatted_contents()\n
      • line 882 of /lib/blocklib.php: call to block_base->get_content_for_output()\n
      • line 934 of /lib/blocklib.php: call to block_manager->create_block_contents()\n
      • line 342 of /lib/blocklib.php: call to block_manager->ensure_content_created()\n
      • line 6 of /theme/formfactor/layout/general.php: call to block_manager->region_has_content()\n
      • line 627 of /lib/outputrenderers.php: call to include()\n
      • line 585 of /lib/outputrenderers.php: call to core_renderer->render_page_layout()\n
      • line 587 of /course/modedit.php: call to core_renderer->header()\n, referer: http://my.moodle.site/course/modedit.php?update=4&return=1&sesskey=eHkDwqXdYZ

      I can reproduce this and will continue to investigate but someone might have some ideas or things to check

        Issue Links

          Activity

          Hide
          Howard Miller added a comment -

          The problem is that there is nothing in the Description box. Rather than displaying an error for that it falls over with this.,

          Show
          Howard Miller added a comment - The problem is that there is nothing in the Description box. Rather than displaying an error for that it falls over with this.,
          Hide
          Howard Miller added a comment -

          noting some similar sounding reports (fixed) I updated to 2010112400 but it makes no difference.

          Show
          Howard Miller added a comment - noting some similar sounding reports (fixed) I updated to 2010112400 but it makes no difference.
          Hide
          Helen Foster added a comment -

          I just tried creating a new page resource on http://qa.moodle.net/ and obtained the same error message.

          Show
          Helen Foster added a comment - I just tried creating a new page resource on http://qa.moodle.net/ and obtained the same error message.
          Hide
          Helen Foster added a comment -

          The same error message occurs in MDL-25121 with a slightly different set-up.

          Show
          Helen Foster added a comment - The same error message occurs in MDL-25121 with a slightly different set-up.
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Well,

          problem here is that portfolio stuff is conflicting withe the module name ("page"). As far as we cannot change the module name, I think this is something to be fixed in the portfolio stuff both by:

          • let's use "exclusive" (unique) names for params used by portfolio, i.e. instead of "page", "config"... use "portfoliopage", "portfolioconfig"...
          • only looking for those parameters when the page is really one portfolio page and is suitable for being using them (/user/portfolio.php, /user/portfoliologs.php ...) and not always (current approach).

          Ciao

          Show
          Eloy Lafuente (stronk7) added a comment - Well, problem here is that portfolio stuff is conflicting withe the module name ("page"). As far as we cannot change the module name, I think this is something to be fixed in the portfolio stuff both by: let's use "exclusive" (unique) names for params used by portfolio, i.e. instead of "page", "config"... use "portfoliopage", "portfolioconfig"... only looking for those parameters when the page is really one portfolio page and is suitable for being using them (/user/portfolio.php, /user/portfoliologs.php ...) and not always (current approach). Ciao
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Adding Penny in case we are missing something... for sure her expertise with portfolios will help. TIA!

          Show
          Eloy Lafuente (stronk7) added a comment - Adding Penny in case we are missing something... for sure her expertise with portfolios will help. TIA!
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Adding also to Sam... to ask if we have any other example of conditionally execution of code based in the request url along the navigation code or how this should be implemented (to resolve the second point above).

          Ciao

          Show
          Eloy Lafuente (stronk7) added a comment - Adding also to Sam... to ask if we have any other example of conditionally execution of code based in the request url along the navigation code or how this should be implemented (to resolve the second point above). Ciao
          Hide
          Dongsheng Cai added a comment -

          https://github.com/dongsheng/moodle/commit/11ba34de997bb92076ef102e0920dce78f970e17

          The above commit renamed page perpage config hide to portfolioxxxx, this fixed the problem, needs Sam's review.

          Show
          Dongsheng Cai added a comment - https://github.com/dongsheng/moodle/commit/11ba34de997bb92076ef102e0920dce78f970e17 The above commit renamed page perpage config hide to portfolioxxxx, this fixed the problem, needs Sam's review.
          Hide
          Sam Hemelryk added a comment -

          Hi guys,
          I've just been chatting to Dongsheng about this now. To summarise I don't think that those optional param calls should be there or are needed at all.
          The links on the navigation shouldn't change based upon variations of a page you are viewing, i.e. portfolio > configure should always take you to the same page (the overview of your portfolios) whether you were on that page or configuring a particular portfolio instance. Keeping those links the same helps people understand the navigation and where they lead.

          So in regards to the params:

          1. hide : The hide param is used to toggle a user config option and I don't think there is any reason you would want to return to that page from the navigation.
          2. config, page, and perpage all change the state of the page you are viewing and as aboce I don't think these should influence the navigation, i.e. the transfer logs link in the navigation should always take you back to the initial page.

          So my +1 to do the following:

          1. Remove all portfolio optional params from navigationlib.php.
          2. user/portfolio.php
            1. Don't add hide to $PAGE->url, it doesn't need to be there as its not a page you will return to.
            2. If config is used as a param use navigation_node::override_active_url and give it a URL without config set so that the navigation still finds the correct node.
          3. user/portfoliologs.php use navigation_node::override_active_url and give it a URL without page or perpage set.

          The advantage of doing it this was is a) we don't need to rename those params so we won't break places we haven't thought of b) We remove the optional param calls from navigation lib that I really don't like being there and c) we keep the navigation links predictable and we won't go off surprising our users by changing where links lead.

          How does that sound to everyone?

          Cheers
          Sam

          Show
          Sam Hemelryk added a comment - Hi guys, I've just been chatting to Dongsheng about this now. To summarise I don't think that those optional param calls should be there or are needed at all. The links on the navigation shouldn't change based upon variations of a page you are viewing, i.e. portfolio > configure should always take you to the same page (the overview of your portfolios) whether you were on that page or configuring a particular portfolio instance. Keeping those links the same helps people understand the navigation and where they lead. So in regards to the params: hide : The hide param is used to toggle a user config option and I don't think there is any reason you would want to return to that page from the navigation. config, page, and perpage all change the state of the page you are viewing and as aboce I don't think these should influence the navigation, i.e. the transfer logs link in the navigation should always take you back to the initial page. So my +1 to do the following: Remove all portfolio optional params from navigationlib.php. user/portfolio.php Don't add hide to $PAGE->url, it doesn't need to be there as its not a page you will return to. If config is used as a param use navigation_node::override_active_url and give it a URL without config set so that the navigation still finds the correct node. user/portfoliologs.php use navigation_node::override_active_url and give it a URL without page or perpage set. The advantage of doing it this was is a) we don't need to rename those params so we won't break places we haven't thought of b) We remove the optional param calls from navigation lib that I really don't like being there and c) we keep the navigation links predictable and we won't go off surprising our users by changing where links lead. How does that sound to everyone? Cheers Sam
          Hide
          Penny Leach added a comment -

          sounds good to me go go sam!

          Show
          Penny Leach added a comment - sounds good to me go go sam!
          Hide
          Dongsheng Cai added a comment -

          Thanks Sam

          Can you please have a look at this commit.

          Regards,
          Dognsheng Cai

          Show
          Dongsheng Cai added a comment - Thanks Sam Can you please have a look at this commit. Regards, Dognsheng Cai
          Hide
          Sam Hemelryk added a comment -

          Hi Dongsheng,

          The changes overall look good however the changes to user/portfoliologs.php aren't quite right yet.
          You've removed the page and limit params from the page URL and have then called navigation_node::override_active_url with a URL that is now identical to $PAGE->url.
          I think the best thing to do is to still add those params to the URL that is going to be given to $PAGE->set_url, and to call override_active_url with a URL that doesn't contain those two params:
          e.g.

          // Base URL
          $url = new moodle_url('/user/portfoliologs.php', array('courseid'=>$courseid));
          // Set the active URL now before we add page and limit params so that we find
          // the correct node on the navigation
          navigation_node::override_active_url($url);
          // Add the page and limit params to complete the current URL
          if ($page !== 0) {
              $url->param('page', $page);
          }
          if ($perpage !== 0) {
              $url->param('perpage', $perpage);
          }
          // Make $PAGE->url the URL of the current page in case an incidental action
          // such as setting a user preference causes the page to refresh
          $PAGE->set_url($url);
          

          Cheers
          Sam

          Show
          Sam Hemelryk added a comment - Hi Dongsheng, The changes overall look good however the changes to user/portfoliologs.php aren't quite right yet. You've removed the page and limit params from the page URL and have then called navigation_node::override_active_url with a URL that is now identical to $PAGE->url. I think the best thing to do is to still add those params to the URL that is going to be given to $PAGE->set_url, and to call override_active_url with a URL that doesn't contain those two params: e.g. // Base URL $url = new moodle_url('/user/portfoliologs.php', array('courseid'=>$courseid)); // Set the active URL now before we add page and limit params so that we find // the correct node on the navigation navigation_node::override_active_url($url); // Add the page and limit params to complete the current URL if ($page !== 0) { $url->param('page', $page); } if ($perpage !== 0) { $url->param('perpage', $perpage); } // Make $PAGE->url the URL of the current page in case an incidental action // such as setting a user preference causes the page to refresh $PAGE->set_url($url); Cheers Sam
          Hide
          Dongsheng Cai added a comment -

          Thanks, Sam.

          I just added page and perpage to $url back, and submitted the pull request PULL-243.

          Regards,
          Dongsheng

          Show
          Dongsheng Cai added a comment - Thanks, Sam. I just added page and perpage to $url back, and submitted the pull request PULL-243. Regards, Dongsheng
          Hide
          Helen Foster added a comment -

          Fix included in the weekly 2.0.1+. Thanks everyone.

          Show
          Helen Foster added a comment - Fix included in the weekly 2.0.1+. Thanks everyone.

            People

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

              Dates

              • Created:
                Updated:
                Resolved: