Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Trivial Trivial
    • Resolution: Fixed
    • Affects Version/s: 1.5
    • Fix Version/s: None
    • Component/s: Blocks
    • Labels:
      None
    • Environment:
      All
    • Database:
      PostgreSQL
    • Affected Branches:
      MOODLE_15_STABLE
    • Rank:
      14505

      Description

      Error: invalid input syntax for integer: none

      Query: SELECT * FROM nmit_block_rss_client WHERE id = 'none' LIMIT 1

      this seems to be from block_rss_client_action.php and a combination of

      $rssid = optional_param('rssid', 'none' );

      and

      $rss_record = get_record('block_rss_client', 'id', $rssid);

      I would change default to 0 but not sure of the implication. There doesn't seem to be a validation of $rss_record being set anywhere, but it's used all throughout this file.

      (Does mysql really not complain about this sort of thing? Am I alone in the world? )

        Activity

        Hide
        Martin Dougiamas added a comment -

        From Jon Papaioannou (pj at moodle.org) Tuesday, 30 August 2005, 11:52 AM:

        Copying Daryl into this, because indeed the obvious thing to do is change the default to 0. Daryl will know if this is a problem.

        Penny looks like you are alone... I guess MySQL friendliness silently converts the string to 0.

        From (penny at catalyst.net.nz) Wednesday, 31 August 2005, 05:15 AM:

        (Pasting email content here)

        Daryl:

        Would this be OK?

        $act = optional_param('act', 'none' );

        if ($act != 'none')

        { $rssid = require_param('rssid', PARAM_INT); }

        else

        { $rssid = optional_param('rssid', 'none' ); }

        If the action requested requires an rssid to work we can require an int rssid, if not then default to an rssid of 'none' and add checking in later code to ensure that we don't try to load a feed with this detail.

        Jon:

        If you are going to be checking for validity of rssid anyway, then I suggest

        $rssid = optional_param('rssid', NULL, PARAM_INT);

        and checking for the presence of a non-null rssid as needed.

        This would be clearer IMO, but it's a rewrite of the parameter handling and thus would take some work.

        From (penny at catalyst.net.nz) Wednesday, 31 August 2005, 05:22 AM:

        Penny:

        Doesn't that not stop the problem in the first place? that is, that there'll still be a query that looks like a string in a should-be-integer field? with this line:

        $rssid = optional_param('rssid', 'none' );

        From Daryl Hawes (dhawes at mac.com) Sunday, 2 October 2005, 11:08 PM:

        I've made some changes to the block_action script that should solve this. I could use some eyes on this test code to verify before closing this bug. Thanks in advance for posting your test results here.

        From Daryl Hawes (dhawes at mac.com) Sunday, 2 October 2005, 11:09 PM:

        NOTE: The changes were made to moodle HEAD - 1.6dev - only.

        From Jon Papaioannou (pj at moodle.org) Sunday, 2 October 2005, 11:46 PM:

        I just made a pass through block_rss_client_action.php and made some changes here and there to make it more easily readable. In the process I caught a notice as well, which reminds me:

        It's better to use isset() or empty() instead of testing for NULL value. This way the meaning is more clear to people reading the code (especially with isset) and it doesn't throw notices. Can you tweak it a little more to change those checks for NULL?

        (remember to cvs update first to get my changes)

        From Daryl Hawes (dhawes at mac.com) Monday, 3 October 2005, 01:46 AM:

        OK - I have traded all checks for NULL in for isset() function calls. Revision 1.39 in HEAD.

        From (penny at catalyst.net.nz) Monday, 3 October 2005, 06:43 AM:

        it's fixed my problem..

        From (penny at catalyst.net.nz) Monday, 3 October 2005, 06:44 AM:

        can the fix be backported to 1.5 once you're happy with it?

        From Daryl Hawes (dhawes at mac.com) Monday, 3 October 2005, 09:17 PM:

        The original reporters have confirmed that the code in CVS fixes the problem. Marking this bug resolved/fixed. If I get time I will backport to the latest stable branch.

        From (penny at catalyst.net.nz) Monday, 12 December 2005, 11:20 AM:

        marking as closed.

        Show
        Martin Dougiamas added a comment - From Jon Papaioannou (pj at moodle.org) Tuesday, 30 August 2005, 11:52 AM: Copying Daryl into this, because indeed the obvious thing to do is change the default to 0. Daryl will know if this is a problem. Penny looks like you are alone... I guess MySQL friendliness silently converts the string to 0. From (penny at catalyst.net.nz) Wednesday, 31 August 2005, 05:15 AM: (Pasting email content here) Daryl: Would this be OK? $act = optional_param('act', 'none' ); if ($act != 'none') { $rssid = require_param('rssid', PARAM_INT); } else { $rssid = optional_param('rssid', 'none' ); } If the action requested requires an rssid to work we can require an int rssid, if not then default to an rssid of 'none' and add checking in later code to ensure that we don't try to load a feed with this detail. Jon: If you are going to be checking for validity of rssid anyway, then I suggest $rssid = optional_param('rssid', NULL, PARAM_INT); and checking for the presence of a non-null rssid as needed. This would be clearer IMO, but it's a rewrite of the parameter handling and thus would take some work. From (penny at catalyst.net.nz) Wednesday, 31 August 2005, 05:22 AM: Penny: Doesn't that not stop the problem in the first place? that is, that there'll still be a query that looks like a string in a should-be-integer field? with this line: $rssid = optional_param('rssid', 'none' ); From Daryl Hawes (dhawes at mac.com) Sunday, 2 October 2005, 11:08 PM: I've made some changes to the block_action script that should solve this. I could use some eyes on this test code to verify before closing this bug. Thanks in advance for posting your test results here. From Daryl Hawes (dhawes at mac.com) Sunday, 2 October 2005, 11:09 PM: NOTE: The changes were made to moodle HEAD - 1.6dev - only. From Jon Papaioannou (pj at moodle.org) Sunday, 2 October 2005, 11:46 PM: I just made a pass through block_rss_client_action.php and made some changes here and there to make it more easily readable. In the process I caught a notice as well, which reminds me: It's better to use isset() or empty() instead of testing for NULL value. This way the meaning is more clear to people reading the code (especially with isset) and it doesn't throw notices. Can you tweak it a little more to change those checks for NULL? (remember to cvs update first to get my changes) From Daryl Hawes (dhawes at mac.com) Monday, 3 October 2005, 01:46 AM: OK - I have traded all checks for NULL in for isset() function calls. Revision 1.39 in HEAD. From (penny at catalyst.net.nz) Monday, 3 October 2005, 06:43 AM: it's fixed my problem.. From (penny at catalyst.net.nz) Monday, 3 October 2005, 06:44 AM: can the fix be backported to 1.5 once you're happy with it? From Daryl Hawes (dhawes at mac.com) Monday, 3 October 2005, 09:17 PM: The original reporters have confirmed that the code in CVS fixes the problem. Marking this bug resolved/fixed. If I get time I will backport to the latest stable branch. From (penny at catalyst.net.nz) Monday, 12 December 2005, 11:20 AM: marking as closed.
        Hide
        Michael Blake added a comment -

        Temp reopen to assign to a valid user. Daryl Hawes.

        Show
        Michael Blake added a comment - Temp reopen to assign to a valid user. Daryl Hawes.
        Hide
        Michael Blake added a comment -

        Reassigning to a valid user.

        Show
        Michael Blake added a comment - Reassigning to a valid user.

          People

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

            Dates

            • Created:
              Updated:
              Resolved: