Guest are continually asked for an enrolment password when trying to enter courses they should have access to

Just noting behavior in master currently:

In a new installation with default settings I create a new course with default settings. Upon inspecting the database I can see that the enrollment instance for guest access (currently disabled) has password=null.
I browse to Course administration > Users > Enrollment methods and use the eye to enable to plugin. Everything stays as it is.
I browse to Course administration > Edit settings. Without changing things I click save changes. guest enrol password in the DB changes from NULL to ''

Looks like we need to decide whether the enrol_guest support null password or whether it should default passwords to ''.

Having looked at the code I think we need to allow the code to support null passwords as well as '' (given the field supports null).
I also think that default instances should be added with a password of '' as well (makes it clear password is being used).

I've created a pair of patches for 2.2 and master to do this:
Git repo: git://github.com/samhemelryk/moodle.git

2.2 branch: wip-MDL-30623-m22
2.2 diff: https://github.com/samhemelryk/moodle/compare/MOODLE_22_STABLE...wip-MDL-30623-m22

master branch: wip-MDL-30623-m23
master diff: https://github.com/samhemelryk/moodle/compare/master...wip-MDL-30623-m23

Up for your consideration Petr

Cheers
Sam

Also noting that a quick fix SQL statement for moodle.org in the mean time would be:

UPDATE enrol SET password = '' WHERE password IS NULL AND enrol='guest';

That should update all guest enrollment plugins with a null password to a '' password which passes the !== and === '' checks.

Petr can you peer-review this please?

I've run that SQL on moodle.org and it's fixed the issue. Thanks, Sam!

Hi, sorry for the trouble. I have created a bit different fix that makes sure that all guest instances have empty password instead of null and added necessary upgrade code.

To integrators: I have intentionally excluded the install.php from master because it is used only during upgrade from 1.9 see MDL-30610 for more details.

thanks a lot for the report and proposed patch!

So... now... we are storing '' (empty strings) with meaning "no password". If so, and we aren't accepting nulls there anymore... wouldn't it be better to change the column specs to avoid accepting NULLs anymore?

Also, being realistic, I think that NULL is a far better value for the "no password" meaning, in fact that seemed to be your 1st idea.

But well, if we want to have one more "silly" empty-string field, oki, np. But then keep out nulls completely.

Ciao

PS: About the missing install.php... oki, np we can consider it ok.

Thanks Petr - indeed that was the other route

Thanks for the fix it is has been integrated now

Yes, I thought NULL was the standard way to have "nothing" now.

hmm, you are right, NULLs might be better choice, I did not think about that much, I just tried to make it consistent

oh, it is already integrated!

Yeah, we commented exactly at the same moment, grrr.

So there are 2 (perhaps 3) solutions:

1) or we transform it all to use NULLs
2) or we agree about to use empties, so we transform the DB to stop accepting nulls and defaulting to empty (same upgrade step would be ok IMO).
3) or we do nothing

For your consideration, I'm happy with any of 1) or 2).

Ciao

my patch did roughly 2/, the logic should be fine, I could have implemented it as NULLS, but it was already using empties in 2.2

Wow, I must be losing explaining skills,

I saw your upgrade script and the SQL update on it, obviouly. What I'm saying, is that we should ALTER the table to stop accepting nulls (right now it continues accepting them, you just modified the contents).

I'm a noob here so I'm going to be quite terse in case I've made a false assumption in my test process. This is how I interpreted the test instructions and performed them:
1)

• git checkout -b preGuestFix da3494a8c42a1bf08fa921f4a9cd73fd4c1df443; # The commit just before the fix was applied to moodle22stable integration on git.moodle.org/integration.git
• created a blank database moodle_int_moodle22stable the with appropriate settings.
• ran the following command to set everything up:

  php ./admin/cli/install.php --lang=en --wwwroot="http://<mydomain>/moodle/int/moodle22stable" --dataroot="<dataroot>/int/moodle22stable" --dbtype=mysqli \
              --dbhost="localhost" --dbname="moodle_int_moodle22stable" --dbuser="<mydbuser>" --dbpass="<mydbpassword>" --fullname="Moodle Integration version 2.2" \
              --shortname="moodleInt22" --adminuser="<adminuser>" --adminpass="<adminpassword>" --non-interactive --agree-license --allow-unstable
  

• created a test course with guest access and no course password.
• Logged out and logged in as guest on the course
• Logged back into admin and out again, to clear any guest session.
• Change the mdl_enrol.password field for guest to NULL

2)

• Tried to "Login as a Guest" and recieved a password box
• cat enrol/guest/version.php | grep "plugin->version"; # outputs: $plugin->version = 2011112900; // The current plugin version (Date: YYYYMMDDXX)

3)

• git checkout MOODLE_22_STABLE # git rev-parse HEAD returns 4dbdc834ffc4adf09c4ae104b9024f61388a2c53
  Just to prove that the patch is applied:
• git remote add skodak git://github.com/skodak/moodle.git
• git fetch skodak
• git merge skodak/w50_MDL-30623_m22_guest # returns "Already up-to-date."
• cat enrol/guest/version.php | grep "plugin->version"; # outputs: $plugin->version = 2011112901; // The current plugin version (Date: YYYYMMDDXX)
• logged into admin, went to home and performed upgrade
• logged out.
• Clicked on "Login as a Guest" for the course
• It worked, WOOT!

4)

• Setup 1.9 instance with test course that has guess access (without a key)
• Login as guest and enter the course as a guest.
• Login as admin to clear the guest session.
• git fetch integration
• git checkout -b MOODLE_22_STABLE integration/MOODLE_22_STABLE
• Run upgrade
• Sign out of admin
• Log into test course as guest

TEST PASSED.

Thanks to Rajesh, Jason and Aparup for your help with this.

Eloy: that table is general, each plugin defines what each filed means - changing to NOT NULL would break BC which is not acceptable for stable, this was one of the reasons why I fixed the trouble with '' because in current 2.2 stable it is expecting empty strings, in older 2.0 and 2.1 we have a mixture there and use sloppy empty(). And no, it is not your fault, I am thinking very slowly these days...

Yes, you did it!

Now your code is part of the best weeklies released ever, many thanks!

Closing, ciao