Moodle
  1. Moodle
  2. MDL-36741

Incorrect number of query params passed for rss feed when timed posts or groups are enabled

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Critical Critical
    • Resolution: Fixed
    • Affects Version/s: 2.2.7, 2.3.4, 2.4, 2.5
    • Fix Version/s: 2.2.7, 2.3.4, 2.4.1
    • Component/s: Forum
    • Labels:
      None
    • Testing Instructions:
      Hide
      1. Visit <yoursite>/admin/settings.php?section=optionalsubsystems and check 'Enable RSS feeds'.
      2. Visit <yoursite>/admin/settings.php?section=modsettingforum and set 'Enable RSS feeds' to 'yes' and check 'Timed posts'.
      3. On course page select Users -> Groups.
      4. Create multiple groups and assign a student to one of these groups.
      5. Create a new forum activity and set 'RSS feed for this activity' to 'Discussions', set 'Number of RSS recent articles' to 10 and set 'Group mode' to 'Separate groups'.
      6. Click on the forum activity and create a forum discussion called 'Past' and set it to expire some time in the past and make it only available to a certain group.
      7. Create another forum discussion called 'Future' and set it to start some time in the future and make it only available to the group chosen above.
      8. Create another forum discussion called 'Present' with no time restrictions and make it only available to the group chosen above.
      9. Create another forum activity with the same settings as earlier, but with 'Group mode' set to 'No Groups'.
      10. For this new forum activity set the same forum discussions mentioned above except without restricting to a group.
      11. As student in the group you selected for the first forum activity, visit all the forum discussions created. Ensure you can not see the 'Future' or 'Past' topics, for the forum discussions you can see click on 'RSS feed of discussions' and ensure you do not get an error.
      12. Log in as a student NOT in the group you chose and click on the discussion. Ensure you can not see the 'Future' and 'Past' ones, as well as the one 'Present' forum discussion restricted to one group. For the forum discussions you can see click on 'RSS feed of discussions' and ensure you do not get an error.
      13. As an administrator change the setting 'RSS feed for this activity' for the forum to 'Posts'.
      14. Re-check the forums RSS feed as both students.
      Show
      Visit <yoursite>/admin/settings.php?section=optionalsubsystems and check 'Enable RSS feeds'. Visit <yoursite>/admin/settings.php?section=modsettingforum and set 'Enable RSS feeds' to 'yes' and check 'Timed posts'. On course page select Users -> Groups. Create multiple groups and assign a student to one of these groups. Create a new forum activity and set 'RSS feed for this activity' to 'Discussions', set 'Number of RSS recent articles' to 10 and set 'Group mode' to 'Separate groups'. Click on the forum activity and create a forum discussion called 'Past' and set it to expire some time in the past and make it only available to a certain group. Create another forum discussion called 'Future' and set it to start some time in the future and make it only available to the group chosen above. Create another forum discussion called 'Present' with no time restrictions and make it only available to the group chosen above. Create another forum activity with the same settings as earlier, but with 'Group mode' set to 'No Groups'. For this new forum activity set the same forum discussions mentioned above except without restricting to a group. As student in the group you selected for the first forum activity, visit all the forum discussions created. Ensure you can not see the 'Future' or 'Past' topics, for the forum discussions you can see click on 'RSS feed of discussions' and ensure you do not get an error. Log in as a student NOT in the group you chose and click on the discussion. Ensure you can not see the 'Future' and 'Past' ones, as well as the one 'Present' forum discussion restricted to one group. For the forum discussions you can see click on 'RSS feed of discussions' and ensure you do not get an error. As an administrator change the setting 'RSS feed for this activity' for the forum to 'Posts'. Re-check the forums RSS feed as both students.
    • Affected Branches:
      MOODLE_22_STABLE, MOODLE_23_STABLE, MOODLE_24_STABLE, MOODLE_25_STABLE
    • Fixed Branches:
      MOODLE_22_STABLE, MOODLE_23_STABLE, MOODLE_24_STABLE
    • Pull from Repository:
    • Pull Master Branch:
      MDL-36741_master
    • Rank:
      46247

      Description

      Noticed the following error while testing MDL-30377.

      ERROR: Incorrect number of query parameters. Expected 4, got 0.
      line 800 of /lib/dml/moodle_database.php: dml_exception thrown
      line 1018 of /lib/dml/mysqli_native_moodle_database.php: call to moodle_database->fix_sql_params()
      line 116 of /mod/forum/rsslib.php: call to mysqli_native_moodle_database->get_records_sql()
      line 78 of /mod/forum/rsslib.php: call to forum_rss_newstuff()
      line 168 of /rss/file.php: call to forum_rss_get_feed()
      

      Steps to replicate:

      1. Visit <yoursite>/admin/settings.php?section=optionalsubsystems and check 'enable RSS feeds'.
      2. Visit <yoursite>/admin/settings.php?section=modsettingforum and set 'Enable RSS feeds' to 'yes' and check 'Timed posts'.
      3. On course page select Users -> Groups.
      4. Create multiple groups and assign a student to one of these groups.
      5. Create a new forum and set 'RSS feed for this activity' to 'Discussions', set 'Number of RSS recent articles' to 10 and set 'Group mode' to 'Separate groups'.
      6. Click on the forum activity and create a forum discussion called 'Past' and set it to expire some time in the past.
      7. Create another forum discussion called 'Future' and set it to start some time in the future.
      8. Create a bunch of other forum discussions that do not expire.
      9. As student, visit the forum discussion
      10. Under forum admin, click on 'RSS feed of discussions'

      It will show the error.

        Issue Links

          Activity

          Hide
          Mark Nelson added a comment -

          For discussions: when enabled timed posts is on, a separate string and array of params are created to be used in a query. However, only the string is used in the query and the params array is never passed.

          For discussions and posts: When group mode is enabled the same situation occurs.

          Show
          Mark Nelson added a comment - For discussions: when enabled timed posts is on, a separate string and array of params are created to be used in a query. However, only the string is used in the query and the params array is never passed. For discussions and posts: When group mode is enabled the same situation occurs.
          Hide
          Dan Poltawski added a comment -

          Hi Mark,

          Sorry, but we can't do this by switching to concatinated sql statements. We must use placeholders:http://docs.moodle.org/dev/DB_layer_2.0_migration_docs#g6

          Show
          Dan Poltawski added a comment - Hi Mark, Sorry, but we can't do this by switching to concatinated sql statements. We must use placeholders: http://docs.moodle.org/dev/DB_layer_2.0_migration_docs#g6
          Hide
          Mark Nelson added a comment -

          Hi Dan, as discussed introducing the placeholder would require a logic change as everywhere this function is called it expects a SQL string to be returned. It may be overkill since the forum module is going to be replaced in the future. Also, the variables being used are generated by Moodle, they are not user input. I am still happy to change this if you feel it necessary.

          Show
          Mark Nelson added a comment - Hi Dan, as discussed introducing the placeholder would require a logic change as everywhere this function is called it expects a SQL string to be returned. It may be overkill since the forum module is going to be replaced in the future. Also, the variables being used are generated by Moodle, they are not user input. I am still happy to change this if you feel it necessary.
          Hide
          Dan Poltawski added a comment -

          Discussed with Mark - really this is completely broken, so rather than rush this fix in, I think we need to solve it.

          Mark has noticed we use the sql as the hash of the rss file, with the placeholders!

          Show
          Dan Poltawski added a comment - Discussed with Mark - really this is completely broken, so rather than rush this fix in, I think we need to solve it. Mark has noticed we use the sql as the hash of the rss file, with the placeholders!
          Hide
          Dan Poltawski added a comment -

          Hi Mark,

          Discussed with Petr to confirm & I think it is the right solution to correct this properly (with a two element array returned from those broken functions).

          Show
          Dan Poltawski added a comment - Hi Mark, Discussed with Petr to confirm & I think it is the right solution to correct this properly (with a two element array returned from those broken functions).
          Hide
          Mark Nelson added a comment -

          I changed the function forum_rss_feed_contents to take 4 parameters now, instead of 2. The 2 new variables have no default setting because the function was broken to begin with. The only reference to the function was passing 3 parameters, even though it expected only 2! The forum module is completely borked. I really look forward to the day it is replaced.

          Show
          Mark Nelson added a comment - I changed the function forum_rss_feed_contents to take 4 parameters now, instead of 2. The 2 new variables have no default setting because the function was broken to begin with. The only reference to the function was passing 3 parameters, even though it expected only 2! The forum module is completely borked. I really look forward to the day it is replaced.
          Hide
          CiBoT added a comment -

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

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

          The third parameter for forum_rss_feed_contents was removed in 2.3 onwards, but references to it were never changed. I think it's fine to alter the function since it was broken in later releases.

          Show
          Mark Nelson added a comment - The third parameter for forum_rss_feed_contents was removed in 2.3 onwards, but references to it were never changed. I think it's fine to alter the function since it was broken in later releases.
          Hide
          Dan Poltawski added a comment -

          Agreed. The whole thing is completely broken, so nobody can really be using it externally.

          Show
          Dan Poltawski added a comment - Agreed. The whole thing is completely broken, so nobody can really be using it externally.
          Hide
          Frédéric Massart added a comment -

          Thanks Mark, here are a few comments.

          • lib/rsslib.php
            • You probably want to add the new parameter informations to the docblock.
          • mod/forum/rsslib.php
            • #74, is it necessary to keep the 4th unused argument passed to forum_rss_feed_contents()?
            • #110, would be able to use records_count() here? As this just returns true or false, fetch all the data might be a bit of an overkill. I know this was the current behaviour, but well...
            • #124, you're not making sure that $forum->rsstype is set any more, can we assume it will always be?
            • #167, you are setting $newsince as data to use for :newsince, but changed its content before adding it to $params. Result will be "AND p.modified > AND p.modified > :newsince"
            • Should we update the similar functions like forum_rss_get_group_sql() to return an array() of $sql and $params too?
            • #291, missing information about $params in the docblock.
            • #303, could you justify why we don't need to confirm that the forum exists any more?

          As a general note, it appears that this change will break the existing RSS urls for Q&A forums, I am not really against it, but I wanted to raise this here, especially if we backport this issue. Thanks!

          Show
          Frédéric Massart added a comment - Thanks Mark, here are a few comments. lib/rsslib.php You probably want to add the new parameter informations to the docblock. mod/forum/rsslib.php #74, is it necessary to keep the 4th unused argument passed to forum_rss_feed_contents()? #110, would be able to use records_count() here? As this just returns true or false, fetch all the data might be a bit of an overkill. I know this was the current behaviour, but well... #124, you're not making sure that $forum->rsstype is set any more, can we assume it will always be? #167, you are setting $newsince as data to use for :newsince, but changed its content before adding it to $params. Result will be "AND p.modified > AND p.modified > :newsince" Should we update the similar functions like forum_rss_get_group_sql() to return an array() of $sql and $params too? #291, missing information about $params in the docblock. #303, could you justify why we don't need to confirm that the forum exists any more? As a general note, it appears that this change will break the existing RSS urls for Q&A forums, I am not really against it, but I wanted to raise this here, especially if we backport this issue. Thanks!
          Hide
          Mark Nelson added a comment - - edited

          Thanks, a lot of good points.

          You probably want to add the new parameter informations to the docblock.

          • Done.

          Is it necessary to keep the 4th unused argument passed to forum_rss_feed_contents()?

          • Yes, I removed the logic in that function that generated the context, so am now using the context that is passed. No point doing this operation twice.

          Would be able to use records_count() here? As this just returns true or false, fetch all the data might be a bit of an overkill. I know this was the current behaviour, but well...

          • Done.

          You're not making sure that $forum->rsstype is set any more, can we assume it will always be?

          • I made sure that everywhere that this function was called that the check had already been performed before. It should never happen.

          You are setting $newsince as data to use for :newsince, but changed its content before adding it to $params. Result will be "AND p.modified > AND p.modified > :newsince"

          • Done.

          Should we update the similar functions like forum_rss_get_group_sql() to return an array() of $sql and $params too?

          • The other functions use this to create the SQL, then declare the placeholder value afterwards. I did change this as you said to keep it consistent so that the placeholder variable is defined in that function, rather than outside of it.

          Missing information about $params in the docblock.

          • Done.

          Could you justify why we don't need to confirm that the forum exists any more?

          • I shouldn't have deleted the course module. I initially thought it was only used to create the context but it is used elsewhere.
          Show
          Mark Nelson added a comment - - edited Thanks, a lot of good points. You probably want to add the new parameter informations to the docblock. Done. Is it necessary to keep the 4th unused argument passed to forum_rss_feed_contents()? Yes, I removed the logic in that function that generated the context, so am now using the context that is passed. No point doing this operation twice. Would be able to use records_count() here? As this just returns true or false, fetch all the data might be a bit of an overkill. I know this was the current behaviour, but well... Done. You're not making sure that $forum->rsstype is set any more, can we assume it will always be? I made sure that everywhere that this function was called that the check had already been performed before. It should never happen. You are setting $newsince as data to use for :newsince, but changed its content before adding it to $params. Result will be "AND p.modified > AND p.modified > :newsince" Done. Should we update the similar functions like forum_rss_get_group_sql() to return an array() of $sql and $params too? The other functions use this to create the SQL, then declare the placeholder value afterwards. I did change this as you said to keep it consistent so that the placeholder variable is defined in that function, rather than outside of it. Missing information about $params in the docblock. Done. Could you justify why we don't need to confirm that the forum exists any more? I shouldn't have deleted the course module. I initially thought it was only used to create the context but it is used elsewhere.
          Hide
          Mark Nelson added a comment -

          Fred my dear, can you please peer review this again? Thanks.

          Show
          Mark Nelson added a comment - Fred my dear, can you please peer review this again? Thanks.
          Hide
          Frédéric Massart added a comment -

          Looks good to me Mark. Pushing for integration. Could you just comment on the modification of the RSS URL that I mention about line 65 change? Cheers

          Show
          Frédéric Massart added a comment - Looks good to me Mark. Pushing for integration. Could you just comment on the modification of the RSS URL that I mention about line 65 change? Cheers
          Hide
          Mark Nelson added a comment -

          Hey Fred, it won't break the Q&A forums. Originally the code was creating a unique filename for each user for just the Q&A forum type, and not the others. This is wrong, as the other forums RSS feeds can be unique for users, eg. one user may have the ability to view timed posts. The filename will just be generated differently now, and recreated if it doesn't exist. There is no issue the user should see.

          Show
          Mark Nelson added a comment - Hey Fred, it won't break the Q&A forums. Originally the code was creating a unique filename for each user for just the Q&A forum type, and not the others. This is wrong, as the other forums RSS feeds can be unique for users, eg. one user may have the ability to view timed posts. The filename will just be generated differently now, and recreated if it doesn't exist. There is no issue the user should see.
          Hide
          Sam Hemelryk added a comment -

          Thanks Mark, this has been integrated now.

          Show
          Sam Hemelryk added a comment - Thanks Mark, this has been integrated now.
          Hide
          David Monllaó added a comment -

          Testing in master, after setting up the 2 forum activities; begin logged as a student that is not a member of the group, when I access the forums with discussions restricted to the group and I go to the RSS feed I see the attached error

          Show
          David Monllaó added a comment - Testing in master, after setting up the 2 forum activities; begin logged as a student that is not a member of the group, when I access the forums with discussions restricted to the group and I go to the RSS feed I see the attached error
          Hide
          Dan Poltawski added a comment -

          Ping, Mark - can you fix this?

          Show
          Dan Poltawski added a comment - Ping, Mark - can you fix this?
          Hide
          Mark Nelson added a comment -

          Yep, got a fix now, just testing.

          Show
          Mark Nelson added a comment - Yep, got a fix now, just testing.
          Hide
          Mark Nelson added a comment -

          The issue occurred when I changed the SQL to use count_records_sql, rather than get_records, as Fred suggested in the peer review. However, what I failed to realise is that count_records_sql expects a query that returns the count, it doesn't take a given query and then provide the number of records it returns. All it really does is pass the given parameters (sql, params, strictness) to get_record_sql and returns the result.

          Show
          Mark Nelson added a comment - The issue occurred when I changed the SQL to use count_records_sql, rather than get_records, as Fred suggested in the peer review. However, what I failed to realise is that count_records_sql expects a query that returns the count, it doesn't take a given query and then provide the number of records it returns. All it really does is pass the given parameters (sql, params, strictness) to get_record_sql and returns the result.
          Hide
          Mark Nelson added a comment -

          I have pushed a new commit to (hopefully) fix this issue.

          Show
          Mark Nelson added a comment - I have pushed a new commit to (hopefully) fix this issue.
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Sorry Mark but I don't think that is acceptable:

          1) Or you use correct count_records (if you're interested in the count).
          2) Or you use correct record_exists (if you're not interested in the count).

          But not way that that getting all records (imagine they are 20000), to just count() them is the correct way to do so. At least if you had limited it to only get one or so... but you haven't.

          Also, your commit (d23fd79) introduces a change in the behavior of forum_rss_feed_contents(). It used to return string (with contents) or false and now it returns always string (with empty articles).

          So, or I'm missing something or that 2nd commit is not correct really and I'm not confident enough to push it.

          Ciao

          Show
          Eloy Lafuente (stronk7) added a comment - Sorry Mark but I don't think that is acceptable: 1) Or you use correct count_records (if you're interested in the count). 2) Or you use correct record_exists (if you're not interested in the count). But not way that that getting all records (imagine they are 20000), to just count() them is the correct way to do so. At least if you had limited it to only get one or so... but you haven't. Also, your commit (d23fd79) introduces a change in the behavior of forum_rss_feed_contents(). It used to return string (with contents) or false and now it returns always string (with empty articles). So, or I'm missing something or that 2nd commit is not correct really and I'm not confident enough to push it. Ciao
          Hide
          Mark Nelson added a comment -

          Hi Eloy,

          I had the same thoughts when I was adding this change, however, this is how this function used to behave so decided to revert my use of count_records to how it was before (except I forgot to add a limit). I will use, as you suggested, records_exists_sql. I will also cherry-pick to other branches.

          Thanks for pointing this out.

          Show
          Mark Nelson added a comment - Hi Eloy, I had the same thoughts when I was adding this change, however, this is how this function used to behave so decided to revert my use of count_records to how it was before (except I forgot to add a limit). I will use, as you suggested, records_exists_sql. I will also cherry-pick to other branches. Thanks for pointing this out.
          Hide
          Eloy Lafuente (stronk7) added a comment -

          New commit added to all branches. sending this towards another testing round, thanks!

          Show
          Eloy Lafuente (stronk7) added a comment - New commit added to all branches. sending this towards another testing round, thanks!
          Hide
          Dan Poltawski added a comment -

          Hi David,

          Can you test this?

          Show
          Dan Poltawski added a comment - Hi David, Can you test this?
          Hide
          David Monllaó added a comment -

          It passes, I can only see the 'present' discussion with the student who is member of the group, the RSS don't show items if there are no discussions or posts availables. Tested in 22, 23 and master

          Show
          David Monllaó added a comment - It passes, I can only see the 'present' discussion with the student who is member of the group, the RSS don't show items if there are no discussions or posts availables. Tested in 22, 23 and master
          Hide
          Eloy Lafuente (stronk7) added a comment -

          And your fantastic code has met core, hope they become good friends for a long period.

          Closing, thanks!

          Show
          Eloy Lafuente (stronk7) added a comment - And your fantastic code has met core, hope they become good friends for a long period. Closing, thanks!

            People

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

              Dates

              • Created:
                Updated:
                Resolved: