Merge branch 'w18_MDL-32628_m23_avatarperf2' of git://github.com/skod…

…ak/moodle Conflicts: lib/db/upgrade.php version.php
vsiguero · May 4, 2012 · 0dda790 · 0dda790
2 parents 313cdd4 + 4d25479
commit 0dda790
Show file tree

Hide file tree

Showing 13 changed files with 243 additions and 73 deletions.
diff --git a/admin/tool/uploaduser/picture.php b/admin/tool/uploaduser/picture.php
@@ -226,8 +226,8 @@ function process_file ($file, $userfield, $overwrite) {
         return PIX_FILE_SKIPPED;
     }
 
-    if (my_save_profile_image($user->id, $file)) {
-        $DB->set_field('user', 'picture', 1, array('id'=>$user->id));
+    if ($newrev = my_save_profile_image($user->id, $file)) {
+        $DB->set_field('user', 'picture', $newrev, array('id'=>$user->id));
         echo $OUTPUT->notification(get_string('uploadpicture_userupdated', 'tool_uploaduser', $user->username), 'notifysuccess');
         return PIX_FILE_UPDATED;
     } else {
@@ -244,7 +244,7 @@ function process_file ($file, $userfield, $overwrite) {
  *                picture file to.
  * @param string $originalfile the full path of the picture file.
  *
- * @return bool
+ * @return mixed new unique revision number or false if not saved
  */
 function my_save_profile_image($id, $originalfile) {
     $context = get_context_instance(CONTEXT_USER, $id);

diff --git a/auth/mnet/auth.php b/auth/mnet/auth.php
@@ -318,8 +318,8 @@ function confirm_mnet_session($token, $remotepeer) {
                             $imagefilename = $CFG->tempdir . '/mnet-usericon-' . $localuser->id;
                             $imagecontents = base64_decode($fetchrequest->response['f1']);
                             file_put_contents($imagefilename, $imagecontents);
-                            if (process_new_icon($usercontext, 'user', 'icon', 0, $imagefilename)) {
-                                $localuser->picture = 1;
+                            if ($newrev = process_new_icon($usercontext, 'user', 'icon', 0, $imagefilename)) {
+                                $localuser->picture = $newrev;
                             }
                             unlink($imagefilename);
                         }

diff --git a/lib/db/install.xml b/lib/db/install.xml
@@ -1,5 +1,5 @@
 <?xml version="1.0" encoding="UTF-8" ?>
-<XMLDB PATH="lib/db" VERSION="20120122" COMMENT="XMLDB file for core Moodle tables"
+<XMLDB PATH="lib/db" VERSION="20120427" COMMENT="XMLDB file for core Moodle tables"
     xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
     xsi:noNamespaceSchemaLocation="../../lib/xmldb/xmldb.xsd"
 >
@@ -734,7 +734,7 @@
         <FIELD NAME="currentlogin" TYPE="int" LENGTH="10" NOTNULL="true" DEFAULT="0" SEQUENCE="false" PREVIOUS="lastlogin" NEXT="lastip"/>
         <FIELD NAME="lastip" TYPE="char" LENGTH="45" NOTNULL="true" SEQUENCE="false" PREVIOUS="currentlogin" NEXT="secret"/>
         <FIELD NAME="secret" TYPE="char" LENGTH="15" NOTNULL="true" SEQUENCE="false" PREVIOUS="lastip" NEXT="picture"/>
-        <FIELD NAME="picture" TYPE="int" LENGTH="1" NOTNULL="true" DEFAULT="0" SEQUENCE="false" PREVIOUS="secret" NEXT="url"/>
+        <FIELD NAME="picture" TYPE="int" LENGTH="10" NOTNULL="true" DEFAULT="0" SEQUENCE="false" COMMENT="0 means no image uploaded, positive values are revisions thta prevent caching problems, negative values are reserved for future use" PREVIOUS="secret" NEXT="url"/>
         <FIELD NAME="url" TYPE="char" LENGTH="255" NOTNULL="true" SEQUENCE="false" PREVIOUS="picture" NEXT="description"/>
         <FIELD NAME="description" TYPE="text" NOTNULL="false" SEQUENCE="false" PREVIOUS="url" NEXT="descriptionformat"/>
         <FIELD NAME="descriptionformat" TYPE="int" LENGTH="2" NOTNULL="true" DEFAULT="0" SEQUENCE="false" PREVIOUS="description" NEXT="mailformat"/>

diff --git a/lib/db/upgrade.php b/lib/db/upgrade.php
@@ -422,5 +422,24 @@ function xmldb_main_upgrade($oldversion) {
         upgrade_main_savepoint(true, 2012042300.02);
     }
 
+    if ($oldversion < 2012050300.01) {
+        // Make sure deleted users do not have picture flag.
+        $DB->set_field('user', 'picture', 0, array('deleted'=>1, 'picture'=>1));
+        upgrade_main_savepoint(true, 2012050300.01);
+    }
+
+    if ($oldversion < 2012050300.02) {
+
+        // Changing precision of field picture on table user to (10)
+        $table = new xmldb_table('user');
+        $field = new xmldb_field('picture', XMLDB_TYPE_INTEGER, '10', null, XMLDB_NOTNULL, null, '0', 'secret');
+
+        // Launch change of precision for field picture
+        $dbman->change_field_precision($table, $field);
+
+        // Main savepoint reached
+        upgrade_main_savepoint(true, 2012050300.02);
+    }
+
     return true;
 }
diff --git a/lib/filelib.php b/lib/filelib.php
@@ -3404,35 +3404,44 @@ function file_pluginfile($relativepath, $forcedownload, $preview = null) {
     // ========================================================================================================================
     } else if ($component === 'user') {
         if ($filearea === 'icon' and $context->contextlevel == CONTEXT_USER) {
-            $redirect = false;
             if (count($args) == 1) {
                 $themename = theme_config::DEFAULT_THEME;
                 $filename = array_shift($args);
             } else {
                 $themename = array_shift($args);
                 $filename = array_shift($args);
             }
+
+            // fix file name automatically
+            if ($filename !== 'f1' and $filename !== 'f2') {
+                $filename = 'f1';
+            }
+
             if ((!empty($CFG->forcelogin) and !isloggedin()) ||
                     (!empty($CFG->forceloginforprofileimage) && (!isloggedin() || isguestuser()))) {
                 // protect images if login required and not logged in;
                 // also if login is required for profile images and is not logged in or guest
                 // do not use require_login() because it is expensive and not suitable here anyway
-                $redirect = true;
-            }
-            if (!$redirect and ($filename !== 'f1' and $filename !== 'f2')) {
-                $filename = 'f1';
-                $redirect = true;
+                $theme = theme_config::load($themename);
+                redirect($theme->pix_url('u/'.$filename, 'moodle')); // intentionally not cached
             }
-            if (!$redirect && !$file = $fs->get_file($context->id, 'user', 'icon', 0, '/', $filename.'/.png')) {
+
+            if (!$file = $fs->get_file($context->id, 'user', 'icon', 0, '/', $filename.'/.png')) {
                 if (!$file = $fs->get_file($context->id, 'user', 'icon', 0, '/', $filename.'/.jpg')) {
-                    $redirect = true;
+                    // bad reference - try to prevent future retries as hard as possible!
+                    if ($user = $DB->get_record('user', array('id'=>$context->instanceid), 'id, picture')) {
+                        if ($user->picture == 1 or $user->picture > 10) {
+                            $DB->set_field('user', 'picture', 0, array('id'=>$user->id));
+                        }
+                    }
+                    // no redirect here because it is not cached
+                    $theme = theme_config::load($themename);
+                    $imagefile = $theme->resolve_image_location('u/'.$filename, 'moodle');
+                    send_file($imagefile, basename($imagefile), 60*60*24*14);
                 }
             }
-            if ($redirect) {
-                $theme = theme_config::load($themename);
-                redirect($theme->pix_url('u/'.$filename, 'moodle'));
-            }
-            send_stored_file($file, 60*60*24, 0, false, array('preview' => $preview)); // enable long caching, there are many images on each page
+
+            send_stored_file($file, 60*60*24*7, 0, false, array('preview' => $preview)); // enable long caching, there are many images on each page
 
         } else if ($filearea === 'private' and $context->contextlevel == CONTEXT_USER) {
             require_login();

diff --git a/lib/gdlib.php b/lib/gdlib.php
@@ -90,11 +90,12 @@ function imagecopybicubic($dst_img, $src_img, $dst_x, $dst_y, $src_x, $src_y, $d
 /**
  * Stores optimised icon images in icon file area
  *
- * @param $context
- * @param component
- * @param $itemid
- * @param $originalfile
- * @return success
+ * @param context $context
+ * @param string $component
+ * @param string filearea
+ * @param int $itemid
+ * @param string $originalfile
+ * @return mixed new unique revision number or false if not saved
  */
 function process_new_icon($context, $component, $filearea, $itemid, $originalfile) {
     global $CFG;
@@ -206,7 +207,7 @@ function process_new_icon($context, $component, $filearea, $itemid, $originalfil
     imagedestroy($im1);
     $icon['filename'] = 'f1'.$imageext;
     $fs->delete_area_files($context->id, $component, $filearea, $itemid);
-    $fs->create_file_from_string($icon, $data);
+    $file1 = $fs->create_file_from_string($icon, $data);
 
     ob_start();
     if (!$imagefnc($im2, NULL, $quality, $filters)) {
@@ -219,7 +220,7 @@ function process_new_icon($context, $component, $filearea, $itemid, $originalfil
     $icon['filename'] = 'f2'.$imageext;
     $fs->create_file_from_string($icon, $data);
 
-    return true;
+    return $file1->get_id();
 }
 
 /**

diff --git a/lib/moodlelib.php b/lib/moodlelib.php
@@ -3912,6 +3912,7 @@ function delete_user($user) {
     $updateuser->username     = $delname;            // Remember it just in case
     $updateuser->email        = md5($user->username);// Store hash of username, useful importing/restoring users
     $updateuser->idnumber     = '';                  // Clear this field to free it up
+    $updateuser->picture      = 0;
     $updateuser->timemodified = time();
 
     $DB->update_record('user', $updateuser);

diff --git a/lib/outputcomponents.php b/lib/outputcomponents.php
@@ -177,6 +177,7 @@ class user_picture implements renderable {
      * User picture constructor.
      *
      * @param stdClass $user user record with at least id, picture, imagealt, firstname and lastname set.
+     *                 It is recommended to add also contextid of the user for performance reasons.
      */
     public function __construct(stdClass $user) {
         global $DB;
@@ -312,14 +313,6 @@ public function get_url(moodle_page $page, renderer_base $renderer = null) {
             $renderer = $page->get_renderer('core');
         }
 
-        if ((!empty($CFG->forcelogin) and !isloggedin()) ||
-                (!empty($CFG->forceloginforprofileimage) && (!isloggedin() || isguestuser()))) {
-            // protect images if login required and not logged in;
-            // also if login is required for profile images and is not logged in or guest
-            // do not use require_login() because it is expensive and not suitable here anyway
-            return $renderer->pix_url('u/f1');
-        }
-
         // Sort out the filename and size. Size is only required for the gravatar
         // implementation presently.
         if (empty($this->size)) {
@@ -336,17 +329,36 @@ public function get_url(moodle_page $page, renderer_base $renderer = null) {
             $size = (int)$this->size;
         }
 
-        // First we need to determine whether the user has uploaded a profile
-        // picture of not.
-        if (!empty($this->user->deleted) or !$context = context_user::instance($this->user->id, IGNORE_MISSING)) {
-            $hasuploadedfile = false;
-        } else {
-            $fs = get_file_storage();
-            $hasuploadedfile = ($fs->file_exists($context->id, 'user', 'icon', 0, '/', $filename.'/.png') || $fs->file_exists($context->id, 'user', 'icon', 0, '/', $filename.'/.jpg'));
+        $defaulturl = $renderer->pix_url('u/'.$filename); // default image
+
+        if ((!empty($CFG->forcelogin) and !isloggedin()) ||
+            (!empty($CFG->forceloginforprofileimage) && (!isloggedin() || isguestuser()))) {
+            // Protect images if login required and not logged in;
+            // also if login is required for profile images and is not logged in or guest
+            // do not use require_login() because it is expensive and not suitable here anyway.
+            return $defaulturl;
         }
 
-        $imageurl = $renderer->pix_url('u/'.$filename);
-        if ($hasuploadedfile && $this->user->picture == 1) {
+        // First try to detect deleted users - but do not read from database for performance reasons!
+        if (!empty($this->user->deleted) or strpos($this->user->email, '@') === false) {
+            // All deleted users should have email replaced by md5 hash,
+            // all active users are expected to have valid email.
+            return $defaulturl;
+        }
+
+        // Did the user upload a picture?
+        if ($this->user->picture > 0) {
+            if (!empty($this->user->contextid)) {
+                $contextid = $this->user->contextid;
+            } else {
+                $context = context_user::instance($this->user->id, IGNORE_MISSING);
+                if (!$context) {
+                    // This must be an incorrectly deleted user, all other users have context.
+                    return $defaulturl;
+                }
+                $contextid = $context->id;
+            }
+
             $path = '/';
             if (clean_param($page->theme->name, PARAM_THEME) == $page->theme->name) {
                 // We append the theme name to the file path if we have it so that
@@ -355,9 +367,13 @@ public function get_url(moodle_page $page, renderer_base $renderer = null) {
                 // picture for the correct theme.
                 $path .= $page->theme->name.'/';
             }
-            // Set the image URL to the URL for the uploaded file.
-            $imageurl = moodle_url::make_pluginfile_url($context->id, 'user', 'icon', NULL, $path, $filename);
-        } else if (!empty($CFG->enablegravatar)) {
+            // Set the image URL to the URL for the uploaded file and return.
+            $url = moodle_url::make_pluginfile_url($contextid, 'user', 'icon', NULL, $path, $filename);
+            $url->param('rev', $this->user->picture);
+            return $url;
+        }
+
+        if ($this->user->picture == 0 and !empty($CFG->enablegravatar)) {
             // Normalise the size variable to acceptable bounds
             if ($size < 1 || $size > 512) {
                 $size = 35;
@@ -368,14 +384,13 @@ public function get_url(moodle_page $page, renderer_base $renderer = null) {
             // If the currently requested page is https then we'll return an
             // https gravatar page.
             if (strpos($CFG->httpswwwroot, 'https:') === 0) {
-                $imageurl = new moodle_url("https://secure.gravatar.com/avatar/{$md5}", array('s' => $size, 'd' => $imageurl->out(false)));
+                return new moodle_url("https://secure.gravatar.com/avatar/{$md5}", array('s' => $size, 'd' => $defaulturl->out(false)));
             } else {
-                $imageurl = new moodle_url("http://www.gravatar.com/avatar/{$md5}", array('s' => $size, 'd' => $imageurl->out(false)));
+                return new moodle_url("http://www.gravatar.com/avatar/{$md5}", array('s' => $size, 'd' => $defaulturl->out(false)));
             }
         }
 
-        // Return the URL that has been generated.
-        return $imageurl;
+        return $defaulturl;
     }
 }
 

diff --git a/lib/outputrenderers.php b/lib/outputrenderers.php
@@ -1922,7 +1922,7 @@ protected function render_user_picture(user_picture $userpicture) {
 
         $class = $userpicture->class;
 
-        if ($user->picture != 1 && $user->picture != 2) {
+        if ($user->picture == 0) {
             $class .= ' defaultuserpic';
         }
 

diff --git a/lib/phpunit/generatorlib.php b/lib/phpunit/generatorlib.php
@@ -201,6 +201,7 @@ public function create_user($record=null, array $options=null) {
             $record['idnumber'] = '';
             $record['email']    = md5($record['username']);
             $record['username'] = $delname;
+            $record['picture']  = 0;
         }
 
         $userid = $DB->insert_record('user', $record);