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

Notice: Undefined variable: ldap_cookie in /var/www/moodle/auth/ldap/auth.php on line 1539

    Details

      Description

      This Notice appears if CAS plugin is used.
      If I set a group creator filter (in CAS plugin configuration) and I try to login using CAS, I get this error :

      Notice: Undefined variable: ldap_cookie in /var/www/moodle/auth/ldap/auth.php on line 1539

      (I see PHP Notice because I have my moodle in debug mode). After that, CAS can't redirect me because this error is displayed on the screen. I could simply disable PHP Notices but I guess it would be better to fix this problem.

        Gliffy Diagrams

          Activity

          Hide
          dobedobedoh Andrew Nicols added a comment -

          Hi Fabrice Ménard ,

          Thank you for working on this. The patch itself looks spot on. I have a few other comments for you, but they're easy to address.

          [Y] Syntax
          [Y] Whitespace
          [Y] Output
          [-] Language
          [-] Databases
          [N] Testing (instructions and automated tests)
          [-] Security
          [-] Documentation
          [N] Git
          [-] Third party code
          [Y] Sanity check

          Testing

          If you could provide some testing instructions, that would be really helpful to our testing team. We don't accept issues for integration without them, but I believe it should be possible to test this without CAS itself. Any LDAP server which supports paged results will, I think, display this error during login. At the very least, we can log it to the apache error log to confirm the fix.

          Git

          We try to provide meaningful summaries for our git commits so as to make life easier for those coming to look at things later on. There's some helpful guidance on appropriate git commits at http://docs.moodle.org/dev/Coding_style#Git_commits and you can also view existing commit messages in Moodle using `git log` to see what I mean.
          You don't need to include the error message in the commit either as this is noted in the issue itself.

          Thank you for taking the time to both report, and fix this issue. If you need any help with it, feel free to ping me and I'll do my best. Alternatively, if you aren't able to address these issues for some other reason, we can help you to find a sponsor to tidy things up and get it integrated.

          All the best and Happy New Year,

          Andrew

          Show
          dobedobedoh Andrew Nicols added a comment - Hi Fabrice Ménard , Thank you for working on this. The patch itself looks spot on. I have a few other comments for you, but they're easy to address. [Y] Syntax [Y] Whitespace [Y] Output [-] Language [-] Databases [N] Testing (instructions and automated tests) [-] Security [-] Documentation [N] Git [-] Third party code [Y] Sanity check Testing If you could provide some testing instructions, that would be really helpful to our testing team. We don't accept issues for integration without them, but I believe it should be possible to test this without CAS itself. Any LDAP server which supports paged results will, I think, display this error during login. At the very least, we can log it to the apache error log to confirm the fix. Git We try to provide meaningful summaries for our git commits so as to make life easier for those coming to look at things later on. There's some helpful guidance on appropriate git commits at http://docs.moodle.org/dev/Coding_style#Git_commits and you can also view existing commit messages in Moodle using `git log` to see what I mean. You don't need to include the error message in the commit either as this is noted in the issue itself. Thank you for taking the time to both report, and fix this issue. If you need any help with it, feel free to ping me and I'll do my best. Alternatively, if you aren't able to address these issues for some other reason, we can help you to find a sponsor to tidy things up and get it integrated. All the best and Happy New Year, Andrew
          Hide
          markn Mark Nelson added a comment -

          I have assigned this issue to myself (as I also did the same for MDL-34319 which is another issue that has a patch from Fabrice on the same branch) so we can get this fix into core.

          Show
          markn Mark Nelson added a comment - I have assigned this issue to myself (as I also did the same for MDL-34319 which is another issue that has a patch from Fabrice on the same branch) so we can get this fix into core.
          Hide
          markn Mark Nelson added a comment -

          Note to integrator: Since this issue requires CAS to test (which may take some time to set-up) I have included testing this issue in MDL-34319 (since it takes exactly the same steps to replicate - we are only making sure another notice is not displayed). So, this issue must be integrated before testing MDL-34319.

          Show
          markn Mark Nelson added a comment - Note to integrator: Since this issue requires CAS to test (which may take some time to set-up) I have included testing this issue in MDL-34319 (since it takes exactly the same steps to replicate - we are only making sure another notice is not displayed). So, this issue must be integrated before testing MDL-34319 .
          Hide
          stronk7 Eloy Lafuente (stronk7) 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
          stronk7 Eloy Lafuente (stronk7) 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
          cibot CiBoT added a comment -

          Moving this issue to current integration cycle, will be reviewed soon. Thanks for the hard work!

          Show
          cibot CiBoT added a comment - Moving this issue to current integration cycle, will be reviewed soon. Thanks for the hard work!
          Hide
          poltawski Dan Poltawski added a comment -

          Thanks Fabrice/Mark - i've integrated this to master, 26 and 25

          Show
          poltawski Dan Poltawski added a comment - Thanks Fabrice/Mark - i've integrated this to master, 26 and 25
          Hide
          dmonllao David Monllaó added a comment -

          It passes; tested in 25, 26 and master. Info about how to configure CAS added to the CAS VM readme

          Show
          dmonllao David Monllaó added a comment - It passes; tested in 25, 26 and master. Info about how to configure CAS added to the CAS VM readme
          Hide
          damyon Damyon Wiese added a comment -

          Parallel programmParallel programming is harding is hard

          Thanks for reporting/fixing/testing/improving Moodle. This issue has now been released.

          Show
          damyon Damyon Wiese added a comment - Parallel programmParallel programming is harding is hard Thanks for reporting/fixing/testing/improving Moodle. This issue has now been released.

            People

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

              Dates

              • Created:
                Updated:
                Resolved:
                Fix Release Date:
                10/Mar/14