Uploaded image for project: 'Moodle'
  1. Moodle
  2. MDL-41259

Duplicate shortname can crash LDAP sync

    Details

    • Database:
      Any
    • Testing Instructions:
      Hide

      1/ configure ldap sync with an LDAP server
      2/ configure mapping for shortname, fullname and idnumber of courses
      3/ enable course auto creation
      4/ add two course objects with same value for shortname field to the ldap server
      5/ run cli script enrol/ldap/cli/sync.php

      Show
      1/ configure ldap sync with an LDAP server 2/ configure mapping for shortname, fullname and idnumber of courses 3/ enable course auto creation 4/ add two course objects with same value for shortname field to the ldap server 5/ run cli script enrol/ldap/cli/sync.php
    • Affected Branches:
      MOODLE_24_STABLE, MOODLE_25_STABLE
    • Fixed Branches:
      MOODLE_24_STABLE, MOODLE_25_STABLE, MOODLE_26_STABLE
    • Pull from Repository:

      Description

      In LDAP enrolment if two course records have the same short name the sync proccess will crash with a "Duplicate Short Name" moodle_exception.

        Gliffy Diagrams

          Attachments

            Activity

            Hide
            nadavkav Nadav Kavalerchik added a comment -

            Hi Michael de Raadt,
            Please elevate Nitzan Bar's permissions so he can request peer-review/code-review.
            (He is a developer on my team)

            Show
            nadavkav Nadav Kavalerchik added a comment - Hi Michael de Raadt , Please elevate Nitzan Bar 's permissions so he can request peer-review/code-review. (He is a developer on my team)
            Hide
            salvetore Michael de Raadt added a comment -

            Thanks for recommending Nitzan, Nadav.

            Nitzan: I've added you to the developers group here in tracker.

            It's good to have another person with LDAP skills around.

            Show
            salvetore Michael de Raadt added a comment - Thanks for recommending Nitzan, Nadav. Nitzan: I've added you to the developers group here in tracker. It's good to have another person with LDAP skills around.
            Hide
            nitzan Nitzan Bar added a comment -
            Show
            nitzan Nitzan Bar added a comment - Thanks Michael de Raadt
            Hide
            skodak Petr Skoda added a comment -

            hi, there are a few cosmetic problems:

            1. the comments do not comply with are coding style rules http://docs.moodle.org/dev/Coding_style#Inline_comments
            2. the lang strings should be ordered by keys
            3. the new string has two dots intead of three, and the idnumber should be probably before that

            My +1 to get this to master if the above issues get resolved, thanks for the report and patch.

            Show
            skodak Petr Skoda added a comment - hi, there are a few cosmetic problems: the comments do not comply with are coding style rules http://docs.moodle.org/dev/Coding_style#Inline_comments the lang strings should be ordered by keys the new string has two dots intead of three, and the idnumber should be probably before that My +1 to get this to master if the above issues get resolved, thanks for the report and patch.
            Hide
            nitzan Nitzan Bar added a comment -

            Hi,
            Thanks for your comments.
            I fixed everything.

            Show
            nitzan Nitzan Bar added a comment - Hi, Thanks for your comments. I fixed everything.
            Hide
            skodak Petr Skoda added a comment -

            hello, looks better now, there are two remaining issue, according to moodle coding style guide the comments should end with full stop and the MDL should not be there because we can use git log to find out which issue is related.

            // Check if the shortname already exists if it does - skip course creation.
            

            Show
            skodak Petr Skoda added a comment - hello, looks better now, there are two remaining issue, according to moodle coding style guide the comments should end with full stop and the MDL should not be there because we can use git log to find out which issue is related. // Check if the shortname already exists if it does - skip course creation.
            Hide
            nitzan Nitzan Bar added a comment -

            Thanks. Fixed this also

            Show
            nitzan Nitzan Bar added a comment - Thanks. Fixed this also
            Hide
            skodak Petr Skoda added a comment -

            Perfect thanks, submitting for next weeks integration.

            To integrators: feel free to cherry pick to older stable branches.

            Show
            skodak Petr Skoda added a comment - Perfect thanks, submitting for next weeks integration. To integrators: feel free to cherry pick to older stable branches.
            Hide
            damyon Damyon Wiese added a comment -

            Integrated to 24, 25 and master.

            This issue could do with some more detailed testing instructions.

            Thanks!

            Show
            damyon Damyon Wiese added a comment - Integrated to 24, 25 and master. This issue could do with some more detailed testing instructions. Thanks!
            Hide
            nitzan Nitzan Bar added a comment -

            I have added more detailed testing instructions.
            Hope this is enough

            Thanks!

            Show
            nitzan Nitzan Bar added a comment - I have added more detailed testing instructions. Hope this is enough Thanks!
            Hide
            damyon Damyon Wiese added a comment -

            Thanks for the extra detail - looks good now.

            Show
            damyon Damyon Wiese added a comment - Thanks for the extra detail - looks good now.
            Hide
            fred Frédéric Massart added a comment -

            Failing on 2.4 (2.5 and master are OK):

            == Synching course 'LDAP 4b' for role 'student'
            [ENROL LDAP] CREATE User enrolled to a nonexistant course 'LDAP 4b'
            PHP Notice:  Undefined variable: trace in /home/test/moodles/integration_24/moodle/enrol/ldap/lib.php on line 982
             
            Notice: Undefined variable: trace in /home/test/moodles/integration_24/moodle/enrol/ldap/lib.php on line 982
            PHP Fatal error:  Call to a member function output() on a non-object in /home/test/moodles/integration_24/moodle/enrol/ldap/lib.php on line 982
             
            Fatal error: Call to a member function output() on a non-object in /home/test/moodles/integration_24/moodle/enrol/ldap/lib.php on line 982
            

            Show
            fred Frédéric Massart added a comment - Failing on 2.4 (2.5 and master are OK): == Synching course 'LDAP 4b' for role 'student' [ENROL LDAP] CREATE User enrolled to a nonexistant course 'LDAP 4b' PHP Notice: Undefined variable: trace in /home/test/moodles/integration_24/moodle/enrol/ldap/lib.php on line 982   Notice: Undefined variable: trace in /home/test/moodles/integration_24/moodle/enrol/ldap/lib.php on line 982 PHP Fatal error: Call to a member function output() on a non-object in /home/test/moodles/integration_24/moodle/enrol/ldap/lib.php on line 982   Fatal error: Call to a member function output() on a non-object in /home/test/moodles/integration_24/moodle/enrol/ldap/lib.php on line 982
            Hide
            damyon Damyon Wiese added a comment -

            Thanks Fred,

            Pushed a fix for the 24 backport.

            Back to testing.

            Show
            damyon Damyon Wiese added a comment - Thanks Fred, Pushed a fix for the 24 backport. Back to testing.
            Hide
            fred Frédéric Massart added a comment -

            Re-failed:

            [ENROL LDAP] Course creation failed. Duplicate short name. Skipping course with idnumber 'LDAP 4b'...
            PHP Notice:  Trying to get property of non-object in /home/test/moodles/integration_24/moodle/enrol/ldap/lib.php on line 470
             
            Notice: Trying to get property of non-object in /home/test/moodles/integration_24/moodle/enrol/ldap/lib.php on line 470
            PHP Notice:  Trying to get property of non-object in /home/test/moodles/integration_24/moodle/enrol/ldap/lib.php on line 471
             
            Notice: Trying to get property of non-object in /home/test/moodles/integration_24/moodle/enrol/ldap/lib.php on line 471
            Default exception handler: Can not find data record in database table course. Debug: SELECT id,category FROM {course} WHERE id IS NULL
            [array (
            )]
            Error code: invalidrecord
            * line 1357 of /lib/dml/moodle_database.php: dml_missing_record_exception thrown
            * line 1333 of /lib/dml/moodle_database.php: call to moodle_database->get_record_select()
            * line 6558 of /lib/accesslib.php: call to moodle_database->get_record()
            * line 471 of /enrol/ldap/lib.php: call to context_course::instance()
            * line 59 of /enrol/ldap/cli/sync.php: call to enrol_ldap_plugin->sync_enrolments()
             
            !!! Can not find data record in database table course. !!
            

            It is missing a continue when the course has not been created.

            Show
            fred Frédéric Massart added a comment - Re-failed: [ENROL LDAP] Course creation failed. Duplicate short name. Skipping course with idnumber 'LDAP 4b'... PHP Notice: Trying to get property of non-object in /home/test/moodles/integration_24/moodle/enrol/ldap/lib.php on line 470   Notice: Trying to get property of non-object in /home/test/moodles/integration_24/moodle/enrol/ldap/lib.php on line 470 PHP Notice: Trying to get property of non-object in /home/test/moodles/integration_24/moodle/enrol/ldap/lib.php on line 471   Notice: Trying to get property of non-object in /home/test/moodles/integration_24/moodle/enrol/ldap/lib.php on line 471 Default exception handler: Can not find data record in database table course. Debug: SELECT id,category FROM {course} WHERE id IS NULL [array ( )] Error code: invalidrecord * line 1357 of /lib/dml/moodle_database.php: dml_missing_record_exception thrown * line 1333 of /lib/dml/moodle_database.php: call to moodle_database->get_record_select() * line 6558 of /lib/accesslib.php: call to moodle_database->get_record() * line 471 of /enrol/ldap/lib.php: call to context_course::instance() * line 59 of /enrol/ldap/cli/sync.php: call to enrol_ldap_plugin->sync_enrolments()   !!! Can not find data record in database table course. !! It is missing a continue when the course has not been created.
            Hide
            damyon Damyon Wiese added a comment -

            This was fixed on 25+ in MDL-35159 which was an improvement, so was not backported.

            Show
            damyon Damyon Wiese added a comment - This was fixed on 25+ in MDL-35159 which was an improvement, so was not backported.
            Hide
            damyon Damyon Wiese added a comment -

            Thanks Fred,

            I have backported that continue fix from 25 to 24. Back to testing.

            Show
            damyon Damyon Wiese added a comment - Thanks Fred, I have backported that continue fix from 25 to 24. Back to testing.
            Hide
            fred Frédéric Massart added a comment -

            Now passing on 2.4, thanks!

            Show
            fred Frédéric Massart added a comment - Now passing on 2.4, thanks!
            Hide
            poltawski Dan Poltawski added a comment -

            Congratulations! This change has been integrated upstream and is now available from our git and download mirrors. To celebrate, here is a joke:

            A SQL query goes into a bar, walks up to two tables and asks, "Can I join you?"

            Show
            poltawski Dan Poltawski added a comment - Congratulations! This change has been integrated upstream and is now available from our git and download mirrors. To celebrate, here is a joke: A SQL query goes into a bar, walks up to two tables and asks, "Can I join you?"

              People

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

                Dates

                • Created:
                  Updated:
                  Resolved:
                  Fix Release Date:
                  9/Sep/13