[ticket/10631] Use exceptions for errors. Build action list dynamically.

PHPBB3-10631

phpBB/adm/style/acp_ext_list.html

@@ -26,11 +26,16 @@
 		<tr>
 			<td><strong>{enabled.EXT_NAME}</strong></a></td>
 			<td style="text-align: center;"><a href="{enabled.U_DETAILS}">{L_DETAILS}</a></td>
-			<td style="text-align: center;"><a href="{enabled.U_DISABLE}">{L_DISABLE}</a>&nbsp;
-			|&nbsp;<a href="{enabled.U_PURGE}">{L_PURGE}</a></td>
+			<td style="text-align: center;">
+				<!-- BEGIN actions -->
+					<a href="{enabled.actions.U_ACTION}" alt="{enabled.actions.L_ACTION}">{enabled.actions.L_ACTION}</a>
+					<!-- IF not enabled.actions.S_LAST_ROW -->&nbsp;|&nbsp;<!-- ENDIF -->
+				<!-- END actions -->
+			</td>
 		</tr>
 		<!-- END enabled -->
 		<!-- ENDIF -->
+
 		<!-- IF .disabled -->
 		<tr>
 			<td class="row3" colspan="3"><strong>{L_DISABLED} {L_EXTENSIONS}</strong></td>
@@ -38,9 +43,15 @@
 		<!-- BEGIN disabled -->
 		<tr>
 			<td><strong>{disabled.EXT_NAME}</strong></a></td>
-			<td style="text-align: center;"><a href="{disabled.U_DETAILS}">{L_DETAILS}</a></td>
-			<td style="text-align: center;"><a href="{disabled.U_ENABLE}">{L_ENABLE}</a>&nbsp;
-			<!-- IF disabled.U_PURGE -->|&nbsp;<a href="{disabled.U_PURGE}">{L_PURGE}</a>&nbsp;<!-- ENDIF --></td>
+			<td style="text-align: center;">
+				<!-- IF disabled.U_DETAILS --><a href="{disabled.U_DETAILS}">{L_DETAILS}</a><!-- ENDIF -->
+			</td>
+			<td style="text-align: center;">
+				<!-- BEGIN actions -->
+					<a href="{disabled.actions.U_ACTION}" alt="{disabled.actions.L_ACTION}">{disabled.actions.L_ACTION}</a>
+					<!-- IF not disabled.actions.S_LAST_ROW -->&nbsp;|&nbsp;<!-- ENDIF -->
+				<!-- END actions -->
+			</td>
 		</tr>
 		<!-- END disabled -->
 		<!-- ENDIF -->

phpBB/includes/acp/acp_extensions.php

@@ -22,11 +22,21 @@ class acp_extensions
 {
 	var $u_action;
 
+	private $db;
+	private $config;
+	private $template;
+	private $user;
+
 	function main()
 	{
 		// Start the page
 		global $config, $user, $template, $request, $phpbb_extension_manager, $db, $phpbb_root_path, $phpEx;
 
+		$this->db = $db;
+		$this->config = $config;
+		$this->template = $template;
+		$this->user = $user;
+
 		$user->add_lang(array('install', 'acp/extensions'));
 
 		$this->page_title = 'ACP_EXTENSIONS';
@@ -39,9 +49,10 @@ class acp_extensions
 		{
 			$md_manager = new phpbb_extension_metadata_manager($ext_name, $db, $phpbb_extension_manager, $phpbb_root_path, ".$phpEx", $template, $config);
 
-			if ($md_manager->get_metadata('all') === false)
-			{
-				trigger_error('EXTENSION_INVALID');
+			try{
+				$md_manager->get_metadata('all');
+			} catch( Exception $e ) {
+				trigger_error($e);
 			}
 		}
 
@@ -50,9 +61,9 @@ class acp_extensions
 		{
 			case 'list':
 			default:
-				$this->list_enabled_exts($phpbb_extension_manager, $template);
-				$this->list_disabled_exts($phpbb_extension_manager, $template);
-				$this->list_available_exts($phpbb_extension_manager, $template);
+				$this->list_enabled_exts($phpbb_extension_manager);
+				$this->list_disabled_exts($phpbb_extension_manager);
+				$this->list_available_exts($phpbb_extension_manager);
 
 				$this->tpl_name = 'acp_ext_list';
 			break;
@@ -155,19 +166,28 @@ class acp_extensions
 	 * @param  $template 					An instance of the template engine
 	 * @return null
 	 */
-	public function list_enabled_exts(phpbb_extension_manager $phpbb_extension_manager, phpbb_template $template)
+	public function list_enabled_exts(phpbb_extension_manager $phpbb_extension_manager)
 	{
 		foreach ($phpbb_extension_manager->all_enabled() as $name => $location)
 		{
-			$md_manager = $phpbb_extension_manager->get_extension_metadata_manager($name, $template);
+			$md_manager = $phpbb_extension_manager->get_extension_metadata_manager($name, $this->template);
 
-			$template->assign_block_vars('enabled', array(
+			try {
+				$this->template->assign_block_vars('enabled', array(
 					'EXT_NAME'		=> $md_manager->get_metadata('display-name'),
 
 					'U_DETAILS'		=> $this->u_action . '&action=details&ext_name=' . $name,
-				'U_PURGE'		=> $this->u_action . '&action=purge_pre&ext_name=' . $name,
-				'U_DISABLE'		=> $this->u_action . '&action=disable_pre&ext_name=' . $name,
 				));
+
+				$this->output_actions('enabled', array(
+					'DISABLE'		=> $this->u_action . '&action=disable_pre&ext_name=' . $name,
+					'PURGE'			=> $this->u_action . '&action=purge_pre&ext_name=' . $name,
+				));
+			} catch( Exception $e ) {
+				$this->template->assign_block_vars('disabled', array(
+					'EXT_NAME'		=> $this->user->lang('EXTENSION_INVALID_LIST', $name, $e),
+				));
+			}
 		}
 	}
 
@@ -178,19 +198,28 @@ class acp_extensions
 	 * @param  $template 					An instance of the template engine
 	 * @return null
 	 */
-	public function list_disabled_exts(phpbb_extension_manager $phpbb_extension_manager, phpbb_template $template)
+	public function list_disabled_exts(phpbb_extension_manager $phpbb_extension_manager)
 	{
 		foreach ($phpbb_extension_manager->all_disabled() as $name => $location)
 		{
-			$md_manager = $phpbb_extension_manager->get_extension_metadata_manager($name, $template);
+			$md_manager = $phpbb_extension_manager->get_extension_metadata_manager($name, $this->template);
 
-			$template->assign_block_vars('disabled', array(
+			try {
+				$this->template->assign_block_vars('disabled', array(
 					'EXT_NAME'		=> $md_manager->get_metadata('display-name'),
 
 					'U_DETAILS'		=> $this->u_action . '&action=details&ext_name=' . $name,
-				'U_PURGE'		=> $this->u_action . '&action=purge_pre&ext_name=' . $name,
-				'U_ENABLE'		=> $this->u_action . '&action=enable_pre&ext_name=' . $name,
 				));
+
+				$this->output_actions('disabled', array(
+					'ENABLE'		=> $this->u_action . '&action=enable_pre&ext_name=' . $name,
+					'PURGE'			=> $this->u_action . '&action=purge_pre&ext_name=' . $name,
+				));
+			} catch( Exception $e ) {
+				$this->template->assign_block_vars('disabled', array(
+					'EXT_NAME'		=> $this->user->lang('EXTENSION_INVALID_LIST', $name, $e),
+				));
+			}
 		}
 	}
 
@@ -201,19 +230,45 @@ class acp_extensions
 	 * @param  $template 					An instance of the template engine
 	 * @return null
 	 */
-	public function list_available_exts(phpbb_extension_manager $phpbb_extension_manager, phpbb_template $template)
+	public function list_available_exts(phpbb_extension_manager $phpbb_extension_manager)
 	{
 		$uninstalled = array_diff_key($phpbb_extension_manager->all_available(), $phpbb_extension_manager->all_configured());
 
 		foreach ($uninstalled as $name => $location)
 		{
-			$md_manager = $phpbb_extension_manager->get_extension_metadata_manager($name, $template);
+			$md_manager = $phpbb_extension_manager->get_extension_metadata_manager($name, $this->template);
 
-			$template->assign_block_vars('disabled', array(
+			try {
+				$this->template->assign_block_vars('disabled', array(
 					'EXT_NAME'		=> $md_manager->get_metadata('display-name'),
 
 					'U_DETAILS'		=> $this->u_action . '&action=details&ext_name=' . $name,
-				'U_ENABLE'		=> $this->u_action . '&action=enable_pre&ext_name=' . $name,
+				));
+
+				$this->output_actions('disabled', array(
+					'ENABLE'		=> $this->u_action . '&action=enable_pre&ext_name=' . $name,
+				));
+			} catch( Exception $e ) {
+				$this->template->assign_block_vars('disabled', array(
+					'EXT_NAME'		=> $this->user->lang('EXTENSION_INVALID_LIST', $name, $e),
+				));
+			}
+		}
+	}
+
+	/**
+	* Output actions to a block
+	*
+	* @param string $block
+	* @param array $actions
+	*/
+	private function output_actions($block, $actions)
+	{
+		foreach ($actions as $lang => $url)
+		{
+			$this->template->assign_block_vars($block . '.actions', array(
+				'L_ACTION'		=> $this->user->lang($lang),
+				'U_ACTION'		=> $url,
 			));
 		}
 	}

phpBB/includes/exception/metadata.php

@@ -0,0 +1,69 @@
+<?php
+/**
+*
+* @package extension
+* @copyright (c) 2012 phpBB Group
+* @license http://opensource.org/licenses/gpl-2.0.php GNU General Public License v2
+*
+*/
+
+/**
+* @ignore
+*/
+if (!defined('IN_PHPBB'))
+{
+	exit;
+}
+
+/**
+ * Exception class for metadata
+ */
+class phpbb_exception_metadata extends LogicException
+{
+	const NOT_SET = 10001;
+	const INVALID = 10002;
+	const FILE_GET_CONTENTS = 10003;
+	const JSON_DECODE = 10004;
+	const FILE_DOES_NOT_EXIST = 10005;
+
+	public function __construct($code, $field_name)
+	{
+		$this->code = $code;
+		$this->field_name = $field_name;
+	}
+
+	public function __toString()
+	{
+		return sprintf($this->getErrorMessage(), $this->field_name);
+	}
+
+	public function getErrorMessage()
+	{
+		switch ($this->code)
+		{
+			case self::NOT_SET:
+				return 'The "%s" meta field has not been set.';
+			break;
+
+			case self::INVALID:
+				return 'The "%s" meta field is not valid.';
+			break;
+
+			case self::FILE_GET_CONTENTS:
+				return 'file_get_contents failed on %s';
+			break;
+
+			case self::JSON_DECODE:
+				return 'json_decode failed on %s';
+			break;
+
+			case self::FILE_DOES_NOT_EXIST:
+				return 'Required file does not exist at %s';
+			break;
+
+			default:
+				return 'An unexpected error has occurred.';
+			break;
+		}
+	}
+}

phpBB/includes/extension/metadata_manager.php

@@ -56,27 +56,18 @@ class phpbb_extension_metadata_manager
 	 * Processes and gets the metadata requested
 	 *
 	 * @param  string $element			All for all metadata that it has and is valid, otherwise specify which section you want by its shorthand term.
-	 * @return bool|array				Contains all of the requested metadata or bool False if not valid
+	 * @return array					Contains all of the requested metadata, throws an exception on failure
 	 */
 	public function get_metadata($element = 'all')
 	{
 		// TODO: Check ext_name exists and is an extension that exists
-		if (!$this->set_metadata_file())
-		{
-			return false;
-		}
+		$this->set_metadata_file();
 
 		// Fetch the metadata
-		if (!$this->fetch_metadata())
-		{
-			return false;
-		}
+		$this->fetch_metadata();
 
 		// Clean the metadata
-		if (!$this->clean_metadata_array())
-		{
-			return false;
-		}
+		$this->clean_metadata_array();
 
 		switch ($element)
 		{
@@ -112,46 +103,42 @@ class phpbb_extension_metadata_manager
 	/**
 	 * Sets the filepath of the metadata file
 	 *
-	 * @return boolean  Set to true if it exists
+	 * @return boolean  Set to true if it exists, throws an exception on failure
 	 */
 	private function set_metadata_file()
 	{
 		$ext_filepath = $this->extension_manager->get_extension_path($this->ext_name);
-		$metadata_filepath = $this->phpbb_root_path . $ext_filepath . '/composer.json';
+		$metadata_filepath = $this->phpbb_root_path . $ext_filepath . 'composer.json';
 
 		$this->metadata_file = $metadata_filepath;
 
 		if (!file_exists($this->metadata_file))
 		{
-			return false;
-		}
-		else
-		{
-			return true;
+    		throw new phpbb_exception_metadata(phpbb_exception_metadata::FILE_DOES_NOT_EXIST, $this->metadata_file);
 		}
 	}
 
 	/**
 	 * Gets the contents of the composer.json file
 	 *
-	 * @return bool True of false (if loading succeeded or failed)
+	 * @return bool True if success, throws an exception on failure
 	 */
 	private function fetch_metadata()
 	{
 		if (!file_exists($this->metadata_file))
 		{
-			return false;
+			throw new phpbb_exception_metadata(phpbb_exception_metadata::FILE_DOES_NOT_EXIST, $this->metadata_file);
 		}
 		else
 		{
 			if (!($file_contents = file_get_contents($this->metadata_file)))
 			{
-				return false;
+    			throw new phpbb_exception_metadata(phpbb_exception_metadata::FILE_GET_CONTENTS, $this->metadata_file);
 			}
 
 			if (($metadata = json_decode($file_contents, true)) === NULL)
 			{
-				return false;
+    			throw new phpbb_exception_metadata(phpbb_exception_metadata::JSON_DECODE, $this->metadata_file);
 			}
 
 			$this->metadata = $metadata;
@@ -161,7 +148,7 @@ class phpbb_extension_metadata_manager
 	}
 
 	/**
-	 * This array handles the validation and cleaning of the array
+	 * This array handles the cleaning of the array
 	 *
 	 * @return array Contains the cleaned metadata array
 	 */
@@ -179,7 +166,7 @@ class phpbb_extension_metadata_manager
 	* @param string $name  ("all" for display and enable validation
 	* 						"display" for name, type, and authors
 	* 						"name", "type")
-	* @return Bool False if validation fails, true if valid
+	* @return Bool True if valid, throws an exception if invalid
 	*/
 	public function validate($name = 'display')
     {
@@ -193,21 +180,34 @@ class phpbb_extension_metadata_manager
 
     	if (isset($fields[$name]))
     	{
-    		return (isset($this->metadata[$name])) ? (bool) preg_match($this->validation[$name], $this->metadata[$name]) : false;
+    		if (!isset($this->metadata[$name]))
+    		{
+    			throw new phpbb_exception_metadata(phpbb_exception_metadata::NOT_SET, $name);
+			}
+
+			if (!preg_match($fields[$name], $this->metadata[$name]))
+			{
+    			throw new phpbb_exception_metadata(phpbb_exception_metadata::INVALID, $name);
+			}
 		}
 
     	// Validate all fields
     	if ($name == 'all')
     	{
-    		foreach ($fields as $field => $data)
-			{
-				if (!$this->validate($field))
-				{
-					return false;
-				}
+    		$this->validate('display');
+
+			$this->validate_enable();
 		}
 
-			return $this->validate_authors();
+    	// Validate display fields
+    	if ($name == 'display')
+    	{
+    		foreach ($fields as $field => $data)
+			{
+				$this->validate($field);
+			}
+
+			$this->validate_authors();
 		}
 
 		return true;
@@ -216,20 +216,20 @@ class phpbb_extension_metadata_manager
 	/**
 	 * Validates the contents of the authors field
 	 *
-	 * @return boolean True when passes validation
+	 * @return boolean True when passes validation, throws exception if invalid
 	 */
 	private function validate_authors()
 	{
 		if (empty($this->metadata['authors']))
 		{
-			return false;
+    		throw new phpbb_exception_metadata(phpbb_exception_metadata::NOT_SET, 'authors');
 		}
 
 		foreach ($this->metadata['authors'] as $author)
 		{
 			if (!isset($author['name']))
 			{
-				return false;
+    			throw new phpbb_exception_metadata(phpbb_exception_metadata::NOT_SET, 'author name');
 			}
 		}
 
@@ -244,7 +244,7 @@ class phpbb_extension_metadata_manager
 	public function validate_enable()
 	{
 		// Check for phpBB, PHP versions
-		if (!$this->validate_require_phpbb || !$this->validate_require_php)
+		if (!$this->validate_require_phpbb() || !$this->validate_require_php())
 		{
 			return false;
 		}

phpBB/language/en/acp/extensions.php

@@ -39,7 +39,7 @@ $lang = array_merge($lang, array(
 	'EXTENSIONS'				=> 'Extensions',
 	'EXTENSIONS_ADMIN'			=> 'Extensions Manager',
 	'EXTENSIONS_EXPLAIN'		=> 'The Extensions Manager is a tool in your phpBB Board which allows you to manage all of your extensions statuses and view information about them.',
-	'EXTENSION_INVALID'			=> 'The selected extension is not valid.',
+	'EXTENSION_INVALID_LIST'	=> 'The "%s" extension is not valid.<br /><p>%s</p>',
 	'EXTENSION_NOT_AVAILABLE'	=> 'The selected extension is not available for this board, please verify your phpBB and PHP versions are allowed (see the details page).',
 
 	'DETAILS'				=> 'Details',