Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Prevent warnings on empty emails #29

Closed
jpahullo opened this issue Mar 7, 2024 · 2 comments
Closed

Prevent warnings on empty emails #29

jpahullo opened this issue Mar 7, 2024 · 2 comments

Comments

@jpahullo
Copy link

jpahullo commented Mar 7, 2024

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
Copy link
Author

jpahullo commented Mar 7, 2024

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.

danmarsden added a commit that referenced this issue Mar 11, 2024
 #29 - 401 - prevent warnings on empty email addresses
@jpahullo
Copy link
Author

Thank you so much @danmarsden.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants