Moodle
  1. Moodle
  2. MDL-34072

Site registration fails on Oracle

    Details

    • Database:
      Oracle
    • Testing Instructions:
      Hide

      While using a site running under Oracle
      Access the site registration page from the Settings block (Site Administration -> Registration)

      Show
      While using a site running under Oracle Access the site registration page from the Settings block (Site Administration -> Registration)
    • Affected Branches:
      MOODLE_23_STABLE
    • Fixed Branches:
      MOODLE_23_STABLE
    • Rank:
      42377

      Description

      Bad SQL in course/lib.php functions average_number_of_participants() and average_number_of_course_modules() will cause the site registration to fail on Oracle (error below).

      Suggest to remove the 'as' preceding subquery alias 'total'

      Without the 'as' it will run correctly on MySQL, PostgreSQL, and Oracle.

      Debug info: ORA-00933: SQL command not properly ended
      SELECT COUNT(*) FROM (
      SELECT DISTINCT ue.userid, e.courseid
      FROM p_user_enrolments ue, p_enrol e, p_course c
      WHERE ue.enrolid = e.id
      AND e.courseid <> :o_siteid
      AND c.id = e.courseid
      AND c.visible = 1) as total
      [array (
      'o_siteid' => '1',
      )]
      Error code: dmlreadexception
      Stack trace:
      
          line 407 of /lib/dml/moodle_database.php: dml_read_exception thrown
          line 284 of /lib/dml/oci_native_moodle_database.php: call to moodle_database->query_end()
          line 1111 of /lib/dml/oci_native_moodle_database.php: call to oci_native_moodle_database->query_end()
          line 1337 of /lib/dml/moodle_database.php: call to oci_native_moodle_database->get_records_sql()
          line 1045 of /lib/dml/oci_native_moodle_database.php: call to moodle_database->get_record_sql()
          line 1410 of /lib/dml/moodle_database.php: call to oci_native_moodle_database->get_record_sql()
          line 1581 of /lib/dml/moodle_database.php: call to moodle_database->get_field_sql()
          line 4038 of /course/lib.php: call to moodle_database->count_records_sql()
          line 373 of /admin/registration/forms.php: call to average_number_of_participants()
          line 173 of /lib/formslib.php: call to site_registration_form->definition()
          line 61 of /admin/registration/register.php: call to moodleform->moodleform()
      

        Issue Links

          Activity

          Show
          Fred Woolard added a comment - https://github.com/appalachianstate/moodle/tree/MDL-34072
          Hide
          Michael de Raadt added a comment -

          Thanks for spotting that and providing a solution.

          Show
          Michael de Raadt added a comment - Thanks for spotting that and providing a solution.
          Hide
          Michael de Raadt added a comment -

          The problem is on line 4036 of /course/lib.php

          This alias is not needed at all, so the "total" could be removed as well as the "as".

          Fred: could you please provided a link to your diff in Git? It will be easier to work with your patch that way.

          Show
          Michael de Raadt added a comment - The problem is on line 4036 of /course/lib.php This alias is not needed at all, so the "total" could be removed as well as the "as". Fred: could you please provided a link to your diff in Git? It will be easier to work with your patch that way.
          Hide
          Fred Woolard added a comment -

          Apologies, I'm still learning much about git/github. Is this what you are requesting?
          https://github.com/appalachianstate/moodle/commit/22129059a6e16f7cc4c1a6e86ad7b4648117534b

          Show
          Fred Woolard added a comment - Apologies, I'm still learning much about git/github. Is this what you are requesting? https://github.com/appalachianstate/moodle/commit/22129059a6e16f7cc4c1a6e86ad7b4648117534b
          Show
          Fred Woolard added a comment - Or this? https://github.com/appalachianstate/moodle/compare/master...MDL-34072
          Hide
          Dan Marsden added a comment -

          Hi Fred - that's the right way to link to the git diff - but as Michael mentions, - the alias isn't needed so you should probably remove the "total" text as well. - if you update your diff someone can probably push this through.

          Show
          Dan Marsden added a comment - Hi Fred - that's the right way to link to the git diff - but as Michael mentions, - the alias isn't needed so you should probably remove the "total" text as well. - if you update your diff someone can probably push this through.
          Hide
          Fred Woolard added a comment -

          Updated to remove alias altogether.

          Show
          Fred Woolard added a comment - Updated to remove alias altogether.
          Hide
          Dan Marsden added a comment -

          looks pretty good to me - only thing I'd suggest is to rebase your 2 commits into a single commit to make the integrators life easier - they would probably do this themselves but it's possible they would reject it until it was rebased.

          All that is needed after rebasing is to add some testing instructions to this issue so that the QA team can test that the fix is correct. I haven't looked - does this appear on the registration page before or after you try to submit for registration?

          Show
          Dan Marsden added a comment - looks pretty good to me - only thing I'd suggest is to rebase your 2 commits into a single commit to make the integrators life easier - they would probably do this themselves but it's possible they would reject it until it was rebased. All that is needed after rebasing is to add some testing instructions to this issue so that the QA team can test that the fix is correct. I haven't looked - does this appear on the registration page before or after you try to submit for registration?
          Hide
          Fred Woolard added a comment -

          Commits merged using rebase (thanks for that handy tip).

          Those two methods only accessed by the registration form & registration_manager when fetching site metrics. Easiest test is to access site registration page, that'll do it. Will add that to the ticket in appropriate field.

          Show
          Fred Woolard added a comment - Commits merged using rebase (thanks for that handy tip). Those two methods only accessed by the registration form & registration_manager when fetching site metrics. Easiest test is to access site registration page, that'll do it. Will add that to the ticket in appropriate field.
          Hide
          Dan Marsden added a comment -

          looks good to me - Thanks Fred, pushing this up for integration.

          NOTE TO INTEGRATOR: please cherry-pick onto 2.3Stable and master - thanks!

          Show
          Dan Marsden added a comment - looks good to me - Thanks Fred, pushing this up for integration. NOTE TO INTEGRATOR: please cherry-pick onto 2.3Stable and master - thanks!
          Hide
          Dan Poltawski 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
          Dan Poltawski 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
          Sam Hemelryk added a comment -

          Thanks guys. This has been integrated now.
          Merged to 23 and cherry-picked to master

          Show
          Sam Hemelryk added a comment - Thanks guys. This has been integrated now. Merged to 23 and cherry-picked to master
          Hide
          Michael de Raadt added a comment -

          Test result: Success!

          Tested under Oracle on 2.3 and Master.

          Thanks for your efforts, Fred and Dan.

          Show
          Michael de Raadt added a comment - Test result: Success! Tested under Oracle on 2.3 and Master. Thanks for your efforts, Fred and Dan.
          Hide
          Dan Poltawski added a comment -

          *Notice*: Undefined variable: friendlyintegrator in /Users/danp/git/tokenintegrationthanks.php on line 26

          Congratulations

          {tracker.user.name}

          !

          You've made into Moodle

          {tracker.fixversion-1}

          +

          I would like to personally thank you for this contribution on behalf of all Moodle users throughout the world.

          cheers!

          {tracker.friendlyintegrator}
          Show
          Dan Poltawski added a comment - * Notice *: Undefined variable: friendlyintegrator in /Users/danp/git/tokenintegrationthanks.php on line 26 Congratulations {tracker.user.name} ! You've made into Moodle {tracker.fixversion-1} + I would like to personally thank you for this contribution on behalf of all Moodle users throughout the world. cheers! {tracker.friendlyintegrator}
          Hide
          Fred Woolard added a comment -

          Apparently, the alias is still needed for the native/mysqli connections. The updated query will work with older mysql connection.

          Show
          Fred Woolard added a comment - Apparently, the alias is still needed for the native/mysqli connections. The updated query will work with older mysql connection.
          Hide
          Fred Woolard added a comment - - edited

          Updated the pull branch to put the 'total' alias back into the queries.

          This form of the query I have tested successfully against Oracle, PostgreSQL, MySQL old standard, and MySQL with mysqli connection.

          Show
          Fred Woolard added a comment - - edited Updated the pull branch to put the 'total' alias back into the queries. This form of the query I have tested successfully against Oracle, PostgreSQL, MySQL old standard, and MySQL with mysqli connection.
          Hide
          Frédéric Massart added a comment -

          MDL-34440 will fix the issue reported by Fred Woolard.
          Fred, could you create a new branch including your fix, and post it on MDL-34440?

          Cheers!

          Show
          Frédéric Massart added a comment - MDL-34440 will fix the issue reported by Fred Woolard. Fred, could you create a new branch including your fix, and post it on MDL-34440 ? Cheers!
          Hide
          Fred Woolard added a comment - - edited

          Done. I do not have edit privilege on the MDL-34440 ticket, so added diff link in comment.

          Show
          Fred Woolard added a comment - - edited Done. I do not have edit privilege on the MDL-34440 ticket, so added diff link in comment.
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Note: I'm backporting the final solution (take rid of the AS keyword) @ MDL-30514. Not sure why both MDL-34072 and MDL-34440 were not backported... but the problem remains there.

          Show
          Eloy Lafuente (stronk7) added a comment - Note: I'm backporting the final solution (take rid of the AS keyword) @ MDL-30514 . Not sure why both MDL-34072 and MDL-34440 were not backported... but the problem remains there.

            People

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

              Dates

              • Created:
                Updated:
                Resolved: