From 611a1d647a3a63013df472b469bf1f3e6e7bd657 Mon Sep 17 00:00:00 2001
From: Cullen Walsh <ckwalsh@phpbb.com>
Date: Wed, 20 Apr 2011 09:46:36 -0700
Subject: [PATCH] [feature/avatars] Refactor avatar's handle_form

Since it was performing two distinct operations, refactor handle_form
to separate functions that prepare and process forms.

PHPBB3-10018
---
 phpBB/includes/avatar/driver.php        |  10 +-
 phpBB/includes/avatar/driver/local.php  | 105 ++++++++------
 phpBB/includes/avatar/driver/remote.php | 178 ++++++++++++------------
 phpBB/includes/avatar/driver/upload.php | 124 +++++++++--------
 phpBB/includes/ucp/ucp_profile.php      |   4 +-
 5 files changed, 229 insertions(+), 192 deletions(-)

diff --git a/phpBB/includes/avatar/driver.php b/phpBB/includes/avatar/driver.php
index a2ff5d804d..5d3b734f7b 100644
--- a/phpBB/includes/avatar/driver.php
+++ b/phpBB/includes/avatar/driver.php
@@ -102,7 +102,15 @@ abstract class phpbb_avatar_driver
 	/**
 	* @TODO
 	**/
-	public function handle_form($template, $user_row, &$error, $submitted = false)
+	public function prepare_form($template, $user_row, &$error)
+	{
+		return false;
+	}
+
+	/**
+	* @TODO
+	**/
+	public function process_form($template, $user_row, &$error)
 	{
 		return false;
 	}
diff --git a/phpBB/includes/avatar/driver/local.php b/phpBB/includes/avatar/driver/local.php
index f9b7662e93..47ae143ec9 100644
--- a/phpBB/includes/avatar/driver/local.php
+++ b/phpBB/includes/avatar/driver/local.php
@@ -47,7 +47,65 @@ class phpbb_avatar_driver_local extends phpbb_avatar_driver
 	/**
 	* @inheritdoc
 	*/
-	public function handle_form($template, $user_row, &$error, $submitted = false)
+	public function prepare_form($template, $user_row, &$error)
+	{
+		$avatar_list = $this->get_avatar_list();
+		$category = request_var('av_local_cat', '');
+		
+		$categories = array_keys($avatar_list);
+
+		foreach ($categories as $cat)
+		{
+			if (!empty($avatar_list[$cat]))
+			{
+				$template->assign_block_vars('av_local_cats', array(
+					'NAME' => $cat,
+					'SELECTED' => ($cat == $category),
+				));
+			}
+		}
+
+		if (!empty($avatar_list[$category]))
+		{
+			foreach ($avatar_list[$category] as $img => $data)
+			{
+				$template->assign_block_vars('av_local_imgs', array(
+					'AVATAR_IMAGE'  => $path . '/' . $data['file'],
+					'AVATAR_NAME' => $data['name'],
+					'AVATAR_FILE' => $data['filename'],
+				));
+			}
+		}
+
+		return true;
+	}
+	
+	/**
+	* @inheritdoc
+	*/
+	public function process_form($template, $user_row, &$error)
+	{
+		$avatar_list = $this->get_avatar_list();
+		$category = request_var('av_local_cat', '');
+		
+		$file = request_var('av_local_file', '');
+		if (!isset($avatar_list[$category][urldecode($file)]))
+		{
+			$error[] = 'AVATAR_URL_NOT_FOUND';
+			return false;
+		}
+
+		return array(
+			'user_avatar' => $category . '/' . $file,
+			'user_avatar_width' => $avatar_list[$category][urldecode($file)]['width'],
+			'user_avatar_height' => $avatar_list[$category][urldecode($file)]['height'],
+		);
+	}
+
+	/**
+	* @TODO
+	*/
+	private function get_avatar_list()
 	{
 		$avatar_list = ($this->cache == null) ? false : $this->cache->get('av_local_list');
 
@@ -101,50 +159,7 @@ class phpbb_avatar_driver_local extends phpbb_avatar_driver
 				$this->cache->put('av_local_list', $avatar_list);
 			}
 		}
-		
-		$category = request_var('av_local_cat', '');
-		
-		if ($submitted) {
-			$file = request_var('av_local_file', '');
-			if (!isset($avatar_list[$category][urldecode($file)]))
-			{
-				$error[] = 'AVATAR_URL_NOT_FOUND';
-				return false;
-			}
 
-			return array(
-				'user_avatar' => $category . '/' . $file,
-				'user_avatar_width' => $avatar_list[$category][urldecode($file)]['width'],
-				'user_avatar_height' => $avatar_list[$category][urldecode($file)]['height'],
-			);
-		}
-
-
-		$categories = array_keys($avatar_list);
-
-		foreach ($categories as $cat)
-		{
-			if (!empty($avatar_list[$cat]))
-			{
-				$template->assign_block_vars('av_local_cats', array(
-					'NAME' => $cat,
-					'SELECTED' => ($cat == $category),
-				));
-			}
-		}
-
-		if (!empty($avatar_list[$category]))
-		{
-			foreach ($avatar_list[$category] as $img => $data)
-			{
-				$template->assign_block_vars('av_local_imgs', array(
-					'AVATAR_IMAGE'  => $path . '/' . $data['file'],
-					'AVATAR_NAME' => $data['name'],
-					'AVATAR_FILE' => $data['filename'],
-				));
-			}
-		}
-
-		return true;
+		return $avatar_list;
 	}
 }
diff --git a/phpBB/includes/avatar/driver/remote.php b/phpBB/includes/avatar/driver/remote.php
index 48f86cac3f..32f93c7928 100644
--- a/phpBB/includes/avatar/driver/remote.php
+++ b/phpBB/includes/avatar/driver/remote.php
@@ -47,115 +47,115 @@ class phpbb_avatar_driver_remote extends phpbb_avatar_driver
 	/**
 	* @inheritdoc
 	*/
-	public function handle_form($template, $user_row, &$error, $submitted = false)
+	public function prepare_form($template, $user_row, &$error)
 	{
-		if ($submitted)
-		{
-			$url = request_var('av_remote_url', '');
-			$width = request_var('av_remote_width', 0);
-			$height = request_var('av_remote_height', 0);
+		$template->assign_vars(array(
+			'AV_REMOTE_WIDTH' => (($user_row['user_avatar_type'] == AVATAR_REMOTE || $user_row['user_avatar_type'] == 'remote') && $user_row['user_avatar_width']) ? $user_row['user_avatar_width'] : request_var('av_local_width', 0),
+			'AV_REMOTE_HEIGHT' => (($user_row['user_avatar_type'] == AVATAR_REMOTE || $user_row['user_avatar_type'] == 'remote') && $user_row['user_avatar_height']) ? $user_row['user_avatar_height'] : request_var('av_local_width', 0),
+			'AV_REMOTE_URL' => (($user_row['user_avatar_type'] == AVATAR_REMOTE || $user_row['user_avatar_type'] == 'remote') && $user_row['user_avatar']) ? $user_row['user_avatar'] : '',
+		));
+
+		return true;
+	}
+
+	/**
+	* @inheritdoc
+	*/
+	public function process_form($template, $user_row, &$error)
+	{
+		$url = request_var('av_remote_url', '');
+		$width = request_var('av_remote_width', 0);
+		$height = request_var('av_remote_height', 0);
 			
-			if (!preg_match('#^(http|https|ftp)://#i', $url))
-			{
-				$url = 'http://' . $url;
-			}
+		if (!preg_match('#^(http|https|ftp)://#i', $url))
+		{
+			$url = 'http://' . $url;
+		}
 
-			$error = array_merge($error, validate_data(array(
-				'url' => $url,
-			), array(
-				'url' => array('string', true, 5, 255),
-			)));
+		$error = array_merge($error, validate_data(array(
+			'url' => $url,
+		), array(
+			'url' => array('string', true, 5, 255),
+		)));
 
-			if (!empty($error))
+		if (!empty($error))
+		{
+			return false;
+		}
+
+		// Check if this url looks alright
+		// This isn't perfect, but it's what phpBB 3.0 did, and might as well make sure everything is compatible
+		if (!preg_match('#^(http|https|ftp)://(?:(.*?\.)*?[a-z0-9\-]+?\.[a-z]{2,4}|(?:\d{1,3}\.){3,5}\d{1,3}):?([0-9]*?).*?\.(gif|jpg|jpeg|png)$#i', $url))
+		{
+			$error[] = 'AVATAR_URL_INVALID';
+			return false;
+		}
+
+		// Make sure getimagesize works...
+		if (function_exists('getimagesize'))
+		{
+			if (($width <= 0 || $height <= 0) && (($image_data = @getimagesize($url)) === false))
 			{
+				$error[] = 'UNABLE_GET_IMAGE_SIZE';
 				return false;
 			}
 
-			// Check if this url looks alright
-			// This isn't perfect, but it's what phpBB 3.0 did, and might as well make sure everything is compatible
-			if (!preg_match('#^(http|https|ftp)://(?:(.*?\.)*?[a-z0-9\-]+?\.[a-z]{2,4}|(?:\d{1,3}\.){3,5}\d{1,3}):?([0-9]*?).*?\.(gif|jpg|jpeg|png)$#i', $url))
-			{
-				$error[] = 'AVATAR_URL_INVALID';
-				return false;
-			}
-
-			// Make sure getimagesize works...
-			if (function_exists('getimagesize'))
-			{
-				if (($width <= 0 || $height <= 0) && (($image_data = @getimagesize($url)) === false))
-				{
-					$error[] = 'UNABLE_GET_IMAGE_SIZE';
-					return false;
-				}
-
-				if (!empty($image_data) && ($image_data[0] <= 0 || $image_data[1] <= 0))
-				{
-					$error[] = 'AVATAR_NO_SIZE';
-					return false;
-				}
-
-				$width = ($width && $height) ? $width : $image_data[0];
-				$height = ($width && $height) ? $height : $image_data[1];
-			}
-
-			if ($width <= 0 || $height <= 0)
+			if (!empty($image_data) && ($image_data[0] <= 0 || $image_data[1] <= 0))
 			{
 				$error[] = 'AVATAR_NO_SIZE';
 				return false;
 			}
 
-			include_once($this->phpbb_root_path . 'includes/functions_upload.' . $this->phpEx);
-			$types = fileupload::image_types();
-			$extension = strtolower(filespec::get_extension($url));
+			$width = ($width && $height) ? $width : $image_data[0];
+			$height = ($width && $height) ? $height : $image_data[1];
+		}
 
-			if (!empty($image_data) && (!isset($types[$image_data[2]]) || !in_array($extension, $types[$image_data[2]])))
+		if ($width <= 0 || $height <= 0)
+		{
+			$error[] = 'AVATAR_NO_SIZE';
+			return false;
+		}
+
+		include_once($this->phpbb_root_path . 'includes/functions_upload.' . $this->phpEx);
+		$types = fileupload::image_types();
+		$extension = strtolower(filespec::get_extension($url));
+
+		if (!empty($image_data) && (!isset($types[$image_data[2]]) || !in_array($extension, $types[$image_data[2]])))
+		{
+			if (!isset($types[$image_data[2]]))
 			{
-				if (!isset($types[$image_data[2]]))
-				{
-					$error[] = 'UNABLE_GET_IMAGE_SIZE';
-				}
-				else
-				{
-					$error[] = array('IMAGE_FILETYPE_MISMATCH', $types[$image_data[2]][0], $extension);
-				}
+				$error[] = 'UNABLE_GET_IMAGE_SIZE';
+			}
+			else
+			{
+				$error[] = array('IMAGE_FILETYPE_MISMATCH', $types[$image_data[2]][0], $extension);
+			}
 
+			return false;
+		}
+
+		if ($this->config['avatar_max_width'] || $this->config['avatar_max_height'])
+		{
+			if ($width > $this->config['avatar_max_width'] || $height > $this->config['avatar_max_height'])
+			{
+				$error[] = array('AVATAR_WRONG_SIZE', $this->config['avatar_min_width'], $this->config['avatar_min_height'], $this->config['avatar_max_width'], $this->config['avatar_max_height'], $width, $height);
 				return false;
 			}
-
-			if ($this->config['avatar_max_width'] || $this->config['avatar_max_height'])
-			{
-				if ($width > $this->config['avatar_max_width'] || $height > $this->config['avatar_max_height'])
-				{
-					$error[] = array('AVATAR_WRONG_SIZE', $this->config['avatar_min_width'], $this->config['avatar_min_height'], $this->config['avatar_max_width'], $this->config['avatar_max_height'], $width, $height);
-					return false;
-				}
-			}
-
-			if ($this->config['avatar_min_width'] || $this->config['avatar_min_height'])
-			{
-				if ($width < $this->config['avatar_min_width'] || $height < $this->config['avatar_min_height'])
-				{
-					$error[] = array('AVATAR_WRONG_SIZE', $this->config['avatar_min_width'], $this->config['avatar_min_height'], $this->config['avatar_max_width'], $this->config['avatar_max_height'], $width, $height);
-					return false;
-				}
-			}
-
-			$result = array(
-				'user_avatar' => $url,
-				'user_avatar_width' => $width,
-				'user_avatar_height' => $height,
-			);
-
-			return $result;
 		}
-		else
+
+		if ($this->config['avatar_min_width'] || $this->config['avatar_min_height'])
 		{
-			$template->assign_vars(array(
-				'AV_REMOTE_WIDTH' => (($user_row['user_avatar_type'] == AVATAR_REMOTE || $user_row['user_avatar_type'] == 'remote') && $user_row['user_avatar_width']) ? $user_row['user_avatar_width'] : request_var('av_local_width', 0),
-				'AV_REMOTE_HEIGHT' => (($user_row['user_avatar_type'] == AVATAR_REMOTE || $user_row['user_avatar_type'] == 'remote') && $user_row['user_avatar_height']) ? $user_row['user_avatar_height'] : request_var('av_local_width', 0),
-				'AV_REMOTE_URL' => (($user_row['user_avatar_type'] == AVATAR_REMOTE || $user_row['user_avatar_type'] == 'remote') && $user_row['user_avatar']) ? $user_row['user_avatar'] : '',
-			));
-			return true;
+			if ($width < $this->config['avatar_min_width'] || $height < $this->config['avatar_min_height'])
+			{
+				$error[] = array('AVATAR_WRONG_SIZE', $this->config['avatar_min_width'], $this->config['avatar_min_height'], $this->config['avatar_max_width'], $this->config['avatar_max_height'], $width, $height);
+				return false;
+			}
 		}
+
+		return array(
+			'user_avatar' => $url,
+			'user_avatar_width' => $width,
+			'user_avatar_height' => $height,
+		);
 	}
 }
diff --git a/phpBB/includes/avatar/driver/upload.php b/phpBB/includes/avatar/driver/upload.php
index cbc46e0e49..dd1dbfa5a0 100644
--- a/phpBB/includes/avatar/driver/upload.php
+++ b/phpBB/includes/avatar/driver/upload.php
@@ -47,66 +47,13 @@ class phpbb_avatar_driver_upload extends phpbb_avatar_driver
 	/**
 	* @inheritdoc
 	*/
-	public function handle_form($template, $user_row, &$error, $submitted = false)
+	public function prepare_form($template, $user_row, &$error)
 	{
-		$can_upload = (file_exists($this->phpbb_root_path . $this->config['avatar_path']) && phpbb_is_writable($this->phpbb_root_path . $this->config['avatar_path']) && (@ini_get('file_uploads') || strtolower(@ini_get('file_uploads')) == 'on')) ? true : false;
-
-		if ($can_upload == false)
+		if (!$this->can_upload())
 		{
 			return false;
 		}
 
-		if ($submitted)
-		{
-			include_once($this->phpbb_root_path . 'includes/functions_upload.' . $this->phpEx);
-
-			$upload = new fileupload('AVATAR_', array('jpg', 'jpeg', 'gif', 'png'), $this->config['avatar_filesize'], $this->config['avatar_min_width'], $this->config['avatar_min_height'], $this->config['avatar_max_width'], $this->config['avatar_max_height'], (isset($this->config['mime_triggers']) ? explode('|', $this->config['mime_triggers']) : false));
-
-			$url = request_var('av_upload_url', '');
-
-			if (!empty($_FILES['av_upload_file']['name']))
-			{
-				$file = $upload->form_upload('av_upload_file');
-			}
-			else
-			{
-				$file = $upload->remote_upload($url);
-			}
-
-			$prefix = $this->config['avatar_salt'] . '_';
-			$file->clean_filename('avatar', $prefix, $user_row['user_id']);
-
-			$destination = $this->config['avatar_path'];
-
-			// Adjust destination path (no trailing slash)
-			if (substr($destination, -1, 1) == '/' || substr($destination, -1, 1) == '\\')
-			{
-				$destination = substr($destination, 0, -1);
-			}
-
-			$destination = str_replace(array('../', '..\\', './', '.\\'), '', $destination);
-			if ($destination && ($destination[0] == '/' || $destination[0] == "\\"))
-			{
-				$destination = '';
-			}
-
-			// Move file and overwrite any existing image
-			$file->move_file($destination, true);
-
-			if (sizeof($file->error))
-			{
-				$file->remove();
-				$error = array_merge($error, $file->error);
-				return false;
-			}
-
-			return array(
-				'user_avatar' => $user_row['user_id'] . '_' . time() . '.' . $file->get('extension'),
-				'user_avatar_width' => $file->get('width'),
-				'user_avatar_height' => $file->get('height'),
-			);
-		}
-		
 		$template->assign_vars(array(
 			'S_UPLOAD_AVATAR_URL' => ($this->config['allow_avatar_remote_upload']) ? true : false,
 			'AV_UPLOAD_SIZE' => $this->config['avatar_filesize'],
@@ -114,4 +61,71 @@ class phpbb_avatar_driver_upload extends phpbb_avatar_driver
 		
 		return true;
 	}
+
+	/**
+	* @inheritdoc
+	*/
+	public function process_form($template, $user_row, &$error)
+	{
+		if (!$this->can_upload())
+		{
+			return false;
+		}
+
+		include_once($this->phpbb_root_path . 'includes/functions_upload.' . $this->phpEx);
+
+		$upload = new fileupload('AVATAR_', array('jpg', 'jpeg', 'gif', 'png'), $this->config['avatar_filesize'], $this->config['avatar_min_width'], $this->config['avatar_min_height'], $this->config['avatar_max_width'], $this->config['avatar_max_height'], (isset($this->config['mime_triggers']) ? explode('|', $this->config['mime_triggers']) : false));
+
+		$url = request_var('av_upload_url', '');
+
+		if (!empty($_FILES['av_upload_file']['name']))
+		{
+			$file = $upload->form_upload('av_upload_file');
+		}
+		else
+		{
+			$file = $upload->remote_upload($url);
+		}
+
+		$prefix = $this->config['avatar_salt'] . '_';
+		$file->clean_filename('avatar', $prefix, $user_row['user_id']);
+
+		$destination = $this->config['avatar_path'];
+
+		// Adjust destination path (no trailing slash)
+		if (substr($destination, -1, 1) == '/' || substr($destination, -1, 1) == '\\')
+		{
+			$destination = substr($destination, 0, -1);
+		}
+
+		$destination = str_replace(array('../', '..\\', './', '.\\'), '', $destination);
+		if ($destination && ($destination[0] == '/' || $destination[0] == "\\"))
+		{
+			$destination = '';
+		}
+
+		// Move file and overwrite any existing image
+		$file->move_file($destination, true);
+
+		if (sizeof($file->error))
+		{
+			$file->remove();
+			$error = array_merge($error, $file->error);
+			return false;
+		}
+
+		return array(
+			'user_avatar' => $user_row['user_id'] . '_' . time() . '.' . $file->get('extension'),
+			'user_avatar_width' => $file->get('width'),
+			'user_avatar_height' => $file->get('height'),
+		);
+	}
+
+	/**
+	* @TODO
+	*/
+	private function can_upload()
+	{
+		return (file_exists($this->phpbb_root_path . $this->config['avatar_path']) && phpbb_is_writable($this->phpbb_root_path . $this->config['avatar_path']) && (@ini_get('file_uploads') || strtolower(@ini_get('file_uploads')) == 'on'));
+	}
 }
diff --git a/phpBB/includes/ucp/ucp_profile.php b/phpBB/includes/ucp/ucp_profile.php
index a815ec7987..a712547bd1 100644
--- a/phpBB/includes/ucp/ucp_profile.php
+++ b/phpBB/includes/ucp/ucp_profile.php
@@ -568,7 +568,7 @@ class ucp_profile
 							{
 								if (check_form_key('ucp_avatar'))
 								{
-									$result = $avatar->handle_form($template, $user->data, $error, true);
+									$result = $avatar->process_form($template, $user->data, $error);
 								
 									if ($result && empty($error))
 									{
@@ -592,7 +592,7 @@ class ucp_profile
 								}
 							}
 
-							if ($avatar->handle_form($template, $user->data, $error)) {
+							if ($avatar->prepare_form($template, $user->data, $error)) {
 								$driver_u = strtoupper($driver);
 
 								$template->assign_block_vars('avatar_drivers', array(