LafColITS / moodle-local_ldap

Various synchronization scripts between Moodle and LDAP directories (see https://tracker.moodle.org/browse/MDL-25011 )
9 stars 7 forks source link

PHP 8.0 countable issue #34

Closed mackensen closed 1 year ago

mackensen commented 1 year ago

Report from the Moodle plugins repository on PHP 8.0 and PHP 8.1 (see #33):

Execute scheduled task: Synchronize cohorts from LDAP groups (local_ldap\task\group_sync_task) ... started 10:47:40. Current memory use 14.5 MB. Debugging increased temporarily due to faildelay of 120 ... used 1 dbqueries ... used 0.035255908966064 seconds Scheduled task failed: Synchronize cohorts from LDAP groups (local_ldap\task\group_sync_task),count(): Argument #1 ($value) must be of type Countable|array, null given Backtrace:

  • line 496 of /local/ldap/locallib.php: call to local_ldap->ldap_get_group_members_ad()
  • line 771 of /local/ldap/locallib.php: call to local_ldap->ldap_get_group_members()
  • line 55 of /local/ldap/classes/task/group_sync_task.php: call to local_ldap->sync_cohorts_by_group()
  • line 259 of /lib/cronlib.php: call to local_ldap\task\group_sync_task->execute()
  • line 167 of /admin/cli/scheduled_task.php: call to cron_run_inner_scheduled_task()

This may be an issue with the loop in ldap_get_group_members_ad().

mackensen commented 1 year ago

I don't have an AD environment to test against right now, but the code on the countable branch may resolve the issue. It can also be downloaded at https://github.com/LafColITS/moodle-local_ldap/zipball/countable.

gymall commented 1 year ago

We got still an error:

Scheduled task failed: Synchronize cohorts from LDAP groups (local_ldap\task\group_sync_task),count(): Argument #1 ($value) must be of type Countable|array, null given Backtrace:

Do you have another idea?

Best regards

mackensen commented 1 year ago

Slightly different part of the code base; sounds like you're running on OpenLDAP. I've pushed a new commit on the countable branch, this is the change:

--- a/locallib.php
+++ b/locallib.php
@@ -769,7 +769,7 @@ class local_ldap extends auth_plugin_ldap {
                     continue;
                 }
                 $ldapmembers = $this->ldap_get_group_members($groupname);
-                if (!$ldapmembers || count($ldapmembers) == 0) {
+                if (!is_array($ldapmembers) || count($ldapmembers) == 0) {
                     // Do not create an empty cohort.
                     continue;
                 }
gymall commented 1 year ago

Thank you for helping. But I got the same error.

Scheduled task failed: Synchronize cohorts from LDAP groups (local_ldap\task\group_sync_task),count(): Argument #1 ($value) must be of type Countable|array, null given Backtrace:

mackensen commented 1 year ago

Is that the complete stack trace? Line 498 shouldn't trigger a failure on its own, it's just calling the function. If the failure is within ldap_get_group_members_rfc(), the line number should be within 222-315. I'll start reviewing that section of the code.

gymall commented 1 year ago

Yes, this was the complete stack trace.

mackensen commented 1 year ago

Okay, I've pushed another commit to address what I think is the only countable situation in that function. Unfortunately, it's hard (working with the LDAP PHP library) to create an empty LDAP group for testing purposes.

gymall commented 1 year ago

Thank you for your work and help!!! Now the task only says:

Cannot obtain task lock

mackensen commented 1 year ago

That can happen for a number of reasons. Usually, it's because the previous time a scheduled task ran the task quit uncleanly, and the lock was not released. It doesn't necessarily indicate a problem with the code.

gymall commented 1 year ago

You´re right! And you got the fix! :) Very nice! Thank you so much! It only takes a long time to run the task...

... started 20:05:50. Current memory use 2.6 MB. Debugging increased temporarily due to faildelay of 86400 ... used 16625 dbqueries ... used 1051.52014184 seconds Scheduled task complete: Synchronize cohorts from LDAP groups (local_ldap\task\group_sync_task)