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

      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()

        Gliffy Diagrams

          Issue Links

            Activity

            Fred Woolard created issue -
            Fred Woolard made changes -
            Field Original Value New Value
            Database Oracle [ 10011 ]
            Show
            Fred Woolard added a comment - https://github.com/appalachianstate/moodle/tree/MDL-34072
            Fred Woolard made changes -
            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.
            Michael de Raadt made changes -
            Fix Version/s STABLE backlog [ 10463 ]
            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()

            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.

            {noformat}
            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()
            {noformat}
            Labels patch triaged
            Component/s Administration [ 10050 ]
            Component/s Database SQL/XMLDB [ 10131 ]
            Component/s Course [ 10057 ]
            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.
            Michael de Raadt made changes -
            Priority Minor [ 4 ] Major [ 3 ]
            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.
            Fred Woolard made changes -
            Testing Instructions Access the site registration page from the Settings block (Site Administration -> Registration)
            Dan Marsden made changes -
            Assignee moodle.com [ moodle.com ] Dan Marsden [ danmarsden ]
            Dan Marsden made changes -
            Peer reviewer danmarsden
            Dan Marsden made changes -
            Testing Instructions Access the site registration page from the Settings block (Site Administration -> Registration) While using a site running under Oracle
            Access the site registration page from the Settings block (Site Administration -> Registration)
            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!
            Dan Marsden made changes -
            Status Open [ 1 ] Waiting for integration review [ 10010 ]
            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
            Sam Hemelryk made changes -
            Currently in integration Yes [ 10041 ]
            Sam Hemelryk made changes -
            Status Waiting for integration review [ 10010 ] Integration review in progress [ 10004 ]
            Integrator samhemelryk
            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
            Sam Hemelryk made changes -
            Status Integration review in progress [ 10004 ] Waiting for testing [ 10005 ]
            Fix Version/s 2.3.2 [ 12353 ]
            Fix Version/s STABLE backlog [ 10463 ]
            Tim Barker made changes -
            Tester abgreeve
            Adrian Greeve made changes -
            Tester abgreeve salvetore
            Michael de Raadt made changes -
            Status Waiting for testing [ 10005 ] Testing in progress [ 10011 ]
            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.
            Michael de Raadt made changes -
            Status Testing in progress [ 10011 ] Tested [ 10006 ]
            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}
            Dan Poltawski made changes -
            Status Tested [ 10006 ] Closed [ 6 ]
            Resolution Fixed [ 1 ]
            Currently in integration Yes [ 10041 ]
            Integration date 19/Jul/12
            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.
            Dan Marsden made changes -
            Link This issue caused a regression MDL-34440 [ MDL-34440 ]
            Frédéric Massart made changes -
            Link This issue caused a regression MDL-34473 [ MDL-34473 ]
            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.
            Eloy Lafuente (stronk7) made changes -
            Link This issue has a non-specific relationship to MDL-30514 [ MDL-30514 ]

              People

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

                Dates

                • Created:
                  Updated:
                  Resolved: