catalyst / moodle-local_cohortauto

Automatically add users into cohorts. (Previously moodle-auth_mcae.)
https://moodle.org/plugins/local_cohortauto
11 stars 14 forks source link

Prevent warnings on empty emails #29

Closed jpahullo closed 6 months ago

jpahullo commented 6 months ago

Hi,

This is to ask you suport and consider in your plugin that empty email may exist.

Rationale

In our institution we run user sincronization from Moodle against several sources. Not all of these systems are able to require email as mandatory for users. So, it is probable that some cases passes through the systems and arrives into Moodle with user accounts without email.

We are making every now and then pedagogy of the use of those systems to set up always email addresses.

But, it is difficult to remove this problem.

What we face

We love this plugin and commits to the task we ask it perfectly. However, the cli/sync_users.php process throws a lot of warnings related to empty email addresses, from its lib.php, when consider de domain name of the email address.

We see these lines for every single user that has no email:

PHP Warning:  Undefined array key 1 in /var/www/html/local/cohortauto/lib.php on line 218

Warning: Undefined array key 1 in /var/www/html/local/cohortauto/lib.php on line 218
PHP Deprecated:  explode(): Passing null to parameter #2 ($string) of type string is deprecated in /var/www/html/local/cohortauto/lib.php on line 221

Deprecated: explode(): Passing null to parameter #2 ($string) of type string is deprecated in /var/www/html/local/cohortauto/lib.php on line 221

What we propose

Consider that empty email may happen and proceed the same way, but with empty strings in all those cases.

In particular, we have this patch in our institution:

diff --git a/local/cohortauto/lib.php b/local/cohortauto/lib.php
index 166b7fe4b91..45b46fc794c 100644
--- a/local/cohortauto/lib.php
+++ b/local/cohortauto/lib.php
@@ -214,23 +214,32 @@ class local_cohortauto_handler {
         profile_load_custom_fields($user);
         $userprofiledata = cohortauto_prepare_profile_data($user, $this->config->secondrule_fld);

-        // Additional values for email.
-        list($emailusername, $emaildomain) = explode("@", $userprofiledata['email']);
-
-        // Email root domain.
-        $emaildomainarray = explode('.', $emaildomain);
-        if (count($emaildomainarray) > 2) {
-            $emailrootdomain = $emaildomainarray[count($emaildomainarray) - 2].'.'.
-                               $emaildomainarray[count($emaildomainarray) - 1];
+        if (empty($userprofiledata['email'])) {
+            $userprofiledata['email'] = array(
+                'full' => '',
+                'username' => '',
+                'domain' => '',
+                'rootdomain' => '',
+            );
         } else {
-            $emailrootdomain = $emaildomain;
+            // Additional values for email.
+            list($emailusername, $emaildomain) = explode("@", $userprofiledata['email']);
+
+            // Email root domain.
+            $emaildomainarray = explode('.', $emaildomain);
+            if (count($emaildomainarray) > 2) {
+                $emailrootdomain = $emaildomainarray[count($emaildomainarray) - 2] . '.' .
+                    $emaildomainarray[count($emaildomainarray) - 1];
+            } else {
+                $emailrootdomain = $emaildomain;
+            }
+            $userprofiledata['email'] = array(
+                'full' => $userprofiledata['email'],
+                'username' => $emailusername,
+                'domain' => $emaildomain,
+                'rootdomain' => $emailrootdomain
+            );
         }
-        $userprofiledata['email'] = array(
-            'full' => $userprofiledata['email'],
-            'username' => $emailusername,
-            'domain' => $emaildomain,
-            'rootdomain' => $emailrootdomain
-        );

         // Set delimiter in use.
         $delimiter = $this->config->delim;

The result

We have applied locally this patch and works like a charm:

# time php local/cohortauto/cli/sync_users.php 
Beginning user cohort sync...
Sync for 79055 users finished.

real    6m52.500s
user    4m36.288s
sys     0m10.032s

And it does not present any warning about empty email.

If you consider this, we are pleased to provide a PR with it.

Let us know.

jpahullo commented 6 months ago

I finally added the PR #30 for you, ready to consider and review.

The CI is blocked until someone review it and allows it to run.

Thanks a lot for your time.

jpahullo commented 6 months ago

Thank you so much @danmarsden.