[ticket/10631] Fix some issues as noted in github comments, significantly simplified validation

PHPBB3-10631

phpBB/adm/style/acp_ext_details.html

@@ -16,36 +16,35 @@
 			<dt><label for="md_name">{L_CLEAN_NAME}:</label></dt>
 			<dd><strong id="md_name">{MD_NAME}</strong></dd>
 		</dl>
-		<dl>
+		<!-- IF MD_DESCRIPTION -->
-			<dt><label for="md_type">{L_TYPE}:</label></dt>
-			<dd><p id="md_type">{MD_TYPE}</p></dd>
-		</dl>
 		<dl>
 			<dt><label for="md_description">{L_DESCRIPTION}:</label></dt>
 			<dd><p id="md_description">{MD_DESCRIPTION}</p></dd>
 		</dl>
+		<!-- ENDIF -->
 		<dl>
 			<dt><label for="md_version">{L_VERSION}:</label></dt>
 			<dd><p id="md_version">{MD_VERSION}</p></dd>
 		</dl>
+		<!-- IF MD_HOMEPAGE -->
 		<dl>
 			<dt><label for="md_homepage">{L_HOMEPAGE}:</label></dt>
 			<dd><p id="md_homepage">{MD_HOMEPAGE}</p></dd>
 		</dl>
-<!-- 		<dl>
+		<!-- ENDIF -->
-			<dt><label for="path">{L_PATH}:</label></dt>
+		<!-- IF MD_TIME -->
-			<dd><p id="path">{PATH}</p></dd>
-		</dl> -->
 		<dl>
 			<dt><label for="md_time">{L_TIME}:</label></dt>
 			<dd><p id="md_time">{MD_TIME}</p></dd>
 		</dl>
+		<!-- ENDIF -->
 		<dl>
 			<dt><label for="md_license">{L_LICENCE}:</label></dt>
 			<dd><p id="md_license">{MD_LICENCE}</p></dd>
 		</dl>
 	</fieldset>
 
+	<!-- IF MD_REQUIRE_PHPBB || MD_REQUIRE_PHP -->
 	<fieldset>
 		<legend>{L_REQUIREMENTS}</legend>
 		<!-- IF MD_REQUIRE_PHPBB -->
@@ -61,10 +60,12 @@
 		</dl>
 		<!-- ENDIF -->
 	</fieldset>
+	<!-- ENDIF -->
 
 	<fieldset>
 		<legend>{L_AUTHOR_INFORMATION}</legend>
 		<!-- BEGIN md_authors -->
+		<fieldset>
 			<dl>
 				<dt><label for="md_author_name">{L_AUTHOR_NAME}:</label></dt>
 				<dd><strong id="md_author_name">{md_authors.AUTHOR_NAME}</strong></dd>
@@ -87,8 +88,7 @@
 				<dd><strong id="md_author_role">{md_authors.AUTHOR_ROLE}</strong></dd>
 			</dl>
 			<!-- ENDIF -->
-
+		</fieldset>
-		<br /><br />
 		<!-- END md_authors -->
 	</fieldset>
 

phpBB/includes/acp/acp_extensions.php

@@ -155,11 +155,11 @@ class acp_extensions
 	 * @param  $template 					An instance of the template engine
 	 * @return null
 	 */
-	private function list_enabled_exts(phpbb_extension_manager $phpbb_extension_manager, phpbb_template $template)
+	public function list_enabled_exts(phpbb_extension_manager $phpbb_extension_manager, phpbb_template $template)
 	{
 		foreach ($phpbb_extension_manager->all_enabled() as $name => $location)
 		{
-			$md_manager = $phpbb_extension_manager->get_extension_metadata($name, $template);
+			$md_manager = $phpbb_extension_manager->get_extension_metadata_manager($name, $template);
 
 			$template->assign_block_vars('enabled', array(
 				'EXT_NAME'		=> $md_manager->get_metadata('display-name'),
@@ -178,11 +178,11 @@ class acp_extensions
 	 * @param  $template 					An instance of the template engine
 	 * @return null
 	 */
-	private function list_disabled_exts(phpbb_extension_manager $phpbb_extension_manager, phpbb_template $template)
+	public function list_disabled_exts(phpbb_extension_manager $phpbb_extension_manager, phpbb_template $template)
 	{
 		foreach ($phpbb_extension_manager->all_disabled() as $name => $location)
 		{
-			$md_manager = $phpbb_extension_manager->get_extension_metadata($name, $template);
+			$md_manager = $phpbb_extension_manager->get_extension_metadata_manager($name, $template);
 
 			$template->assign_block_vars('disabled', array(
 				'EXT_NAME'		=> $md_manager->get_metadata('display-name'),
@@ -201,13 +201,13 @@ class acp_extensions
 	 * @param  $template 					An instance of the template engine
 	 * @return null
 	 */
-	function list_available_exts(phpbb_extension_manager $phpbb_extension_manager, phpbb_template $template)
+	public function list_available_exts(phpbb_extension_manager $phpbb_extension_manager, phpbb_template $template)
 	{
 		$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($name, $template);
+			$md_manager = $phpbb_extension_manager->get_extension_metadata_manager($name, $template);
 
 			$template->assign_block_vars('disabled', array(
 				'EXT_NAME'		=> $md_manager->get_metadata('display-name'),

phpBB/includes/extension/manager.php

@@ -131,7 +131,7 @@ class phpbb_extension_manager
 	* @param string $template The template manager
 	* @return phpbb_extension_metadata_manager Instance of the metadata manager
 	*/
-	public function get_extension_metadata($name, phpbb_template $template)
+	public function get_extension_metadata_manager($name, phpbb_template $template)
 	{
 		return new phpbb_extension_metadata_manager($name, $this->db, $this, $this->phpbb_root_path, $this->php_ext, $template, $this->config);
 	}

phpBB/includes/extension/metadata_manager.php

@@ -28,62 +28,9 @@ class phpbb_extension_metadata_manager
 	protected $phpbb_root_path;
 	protected $template;
 	protected $ext_name;
-	public $metadata;
+	protected $metadata;
 	protected $metadata_file;
 
-	/**
-	* Array of validation regular expressions, see __call()
-	*
-	* @var mixed
-	*/
-	protected $validation = array(
-		'name'					=> '#^[a-zA-Z0-9_\x7f-\xff]{2,}/[a-zA-Z0-9_\x7f-\xff]{2,}$#',
-		'type'					=> '#^phpbb3-extension$#',
-		'description'			=> '#.*#',
-		'version'				=> '#.+#',
-		'licence'				=> '#.+#',
-		//'homepage'				=> '#([\d\w-.]+?\.(a[cdefgilmnoqrstuwz]|b[abdefghijmnorstvwyz]|c[acdfghiklmnoruvxyz]|d[ejkmnoz]|e[ceghrst]|f[ijkmnor]|g[abdefghilmnpqrstuwy]|h[kmnrtu]|i[delmnoqrst]|j[emop]|k[eghimnprwyz]|l[abcikrstuvy]|m[acdghklmnopqrstuvwxyz]|n[acefgilopruz]|om|p[aefghklmnrstwy]|qa|r[eouw]|s[abcdeghijklmnortuvyz]|t[cdfghjkmnoprtvwz]|u[augkmsyz]|v[aceginu]|w[fs]|y[etu]|z[amw]|aero|arpa|biz|com|coop|edu|info|int|gov|mil|museum|name|net|org|pro)(\b|\W(?<!&|=)(?!\.\s|\.{3}).*?))(\s|$)#',
-		'extra'					=> array(
-			'display-name'			=> '#.*#',
-		),
-	);
-
-	/**
-	* Magic method to catch validation calls
-	*
-	* @param string $name
-	* @param mixed $arguments
-	* @return int
-	*/
-	public function __call($name, $arguments)
-    {
-    	// Validation Magic methods
-        if (strpos($name, 'validate_') === 0)
-        {
-        	// Remove validate_
-        	$name = substr($name, 9);
-
-        	// Replace underscores with dashes (underscores are not used)
-        	$name = str_replace('_', '-', $name);
-
-        	if (strpos($name, 'extra-') === 0)
-        	{
-        		// Remove extra_
-        		$name = substr($name, 6);
-
-        		if (isset($this->validation['extra'][$name]))
-        		{
-        			// Extra means it's optional, so return true if it does not exist
-        			return (isset($this->metadata['extra'][$name])) ? preg_match($this->validation['extra'][$name], $this->metadata['extra'][$name]) : true;
-				}
-			}
-			else if (isset($this->validation[$name]) && isset($this->metadata[$name]))
-	        {
-        		return preg_match($this->validation[$name], $this->metadata[$name]);
-			}
-		}
-    }
-
 	/**
 	* Creates the metadata manager
 	*
@@ -136,7 +83,7 @@ class phpbb_extension_metadata_manager
 			case 'all':
 			default:
 				// Validate the metadata
-				if (!$this->validate_metadata_array())
+				if (!$this->validate())
 				{
 					return false;
 				}
@@ -145,17 +92,17 @@ class phpbb_extension_metadata_manager
 			break;
 
 			case 'name':
-				return ($this->validate_name()) ? $this->metadata['name'] : false;
+				return ($this->validate('name')) ? $this->metadata['name'] : false;
 			break;
 
 			case 'display-name':
-				if (isset($this->metadata['extra']['display-name']) && $this->validate_extra_display_name())
+				if (isset($this->metadata['extra']['display-name']))
 				{
 					return $this->metadata['extra']['display-name'];
 				}
 				else
 				{
-					return ($this->validate_name()) ? $this->metadata['name'] : false;
+					return ($this->validate('name')) ? $this->metadata['name'] : false;
 				}
 			break;
 			// TODO: Add remaining cases as needed
@@ -216,7 +163,7 @@ class phpbb_extension_metadata_manager
 	/**
 	 * This array handles the validation and cleaning of the array
 	 *
-	 * @return array Contains the cleaned and validated metadata array
+	 * @return array Contains the cleaned metadata array
 	 */
 	private function clean_metadata_array()
 	{
@@ -227,41 +174,45 @@ class phpbb_extension_metadata_manager
 	}
 
 	/**
-	 * This array handles the validation of strings
+	* Validate fields
 	*
-	 * @return bool True if validation succeeded, False if failed
+	* @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
 	*/
-	public function validate_metadata_array()
+	public function validate($name = 'display')
     {
-		foreach ($this->validation as $name => $regex)
+    	// Basic fields
-		{
+    	$fields = array(
-			if (is_array($regex))
+    		'name'		=> '#^[a-zA-Z0-9_\x7f-\xff]{2,}/[a-zA-Z0-9_\x7f-\xff]{2,}$#',
-			{
+    		'type'		=> '#^phpbb3-extension$#',
-				foreach ($regex as $extra_name => $extra_regex)
+    		'licence'	=> '#.+#',
-				{
+    		'version'	=> '#.+#',
-					$type = 'validate_' . $name . '_' . $extra_name;
+    	);
 
-					if (!$this->$type())
+    	if (isset($fields[$name]))
+    	{
+    		return (isset($this->metadata[$name])) ? (bool) preg_match($this->validation[$name], $this->metadata[$name]) : false;
+		}
+
+    	// Validate all fields
+    	if ($name == 'all')
+    	{
+    		foreach ($fields as $field => $data)
+			{
+				if (!$this->validate($field))
 				{
 					return false;
 				}
 			}
-			}
-			else
-			{
-
-				$type = 'validate_' . $name;
-
-				if (!$this->$type())
-				{
-					return false;
-				}
-			}
-		}
 
 			return $this->validate_authors();
 		}
 
+		return true;
+    }
+
 	/**
 	 * Validates the contents of the authors field
 	 *
@@ -292,20 +243,11 @@ class phpbb_extension_metadata_manager
 	 */
 	public function validate_enable()
 	{
-		$validate = array(
+		// Check for phpBB, PHP versions
-			'require_phpbb',
+		if (!$this->validate_require_phpbb || !$this->validate_require_php)
-			'require_php',
-		);
-
-		foreach ($validate as $type)
-		{
-			$type = 'validate_' . $type;
-
-			if (!$this->$type())
 		{
 			return false;
 		}
-		}
 
 		return true;
 	}
@@ -372,10 +314,10 @@ class phpbb_extension_metadata_manager
 		$this->template->assign_vars(array(
 			'MD_NAME'			=> htmlspecialchars($this->metadata['name']),
 			'MD_TYPE'			=> htmlspecialchars($this->metadata['type']),
-			'MD_DESCRIPTION'	=> htmlspecialchars($this->metadata['description']),
+			'MD_DESCRIPTION'	=> (isset($this->metadata['description'])) ? htmlspecialchars($this->metadata['description']) : '',
 			'MD_HOMEPAGE'		=> (isset($this->metadata['homepage'])) ? $this->metadata['homepage'] : '',
-			'MD_VERSION'		=> htmlspecialchars($this->metadata['version']),
+			'MD_VERSION'		=> (isset($this->metadata['version'])) ? htmlspecialchars($this->metadata['version']) : '',
-			'MD_TIME'			=> htmlspecialchars($this->metadata['time']),
+			'MD_TIME'			=> (isset($this->metadata['time'])) ? htmlspecialchars($this->metadata['time']) : '',
 			'MD_LICENCE'		=> htmlspecialchars($this->metadata['licence']),
 			'MD_REQUIRE_PHP'	=> (isset($this->metadata['require']['php'])) ? htmlspecialchars($this->metadata['require']['php']) : '',
 			'MD_REQUIRE_PHPBB'	=> (isset($this->metadata['require']['phpbb'])) ? htmlspecialchars($this->metadata['require']['phpbb']) : '',
@@ -386,7 +328,7 @@ class phpbb_extension_metadata_manager
 		{
 			$this->template->assign_block_vars('md_authors', array(
 				'AUTHOR_NAME'		=> htmlspecialchars($author['name']),
-				'AUTHOR_EMAIL'		=> $author['email'],
+				'AUTHOR_EMAIL'		=> (isset($author['email'])) ? $author['email'] : '',
 				'AUTHOR_HOMEPAGE'	=> (isset($author['homepage'])) ? $author['homepage'] : '',
 				'AUTHOR_ROLE'		=> (isset($author['role'])) ? htmlspecialchars($author['role']) : '',
 			));