Code review on com_content component. Fixes #10851

administrator/components/com_content/controller.php

@@ -17,8 +17,10 @@
 class ContentController extends JControllerLegacy
 {
 	/**
-	 * @var		string	The default view.
-	 * @since   1.6
+	 * The default view.
+	 *
+	 * @var    string
+	 * @since  1.6
 	 */
 	protected $default_view = 'articles';
 
@@ -28,7 +30,7 @@ class ContentController extends JControllerLegacy
 	 * @param   boolean  $cachable   If true, the view output will be cached
 	 * @param   array    $urlparams  An array of safe url parameters and their variable types, for valid values see {@link JFilterInput::clean()}.
 	 *
-	 * @return  JController		This object to support chaining.
+	 * @return  ContentController  This object to support chaining.
 	 *
 	 * @since   1.5
 	 */

administrator/components/com_content/controllers/article.php

@@ -14,9 +14,7 @@
 /**
  * The article controller
  *
- * @package     Joomla.Administrator
- * @subpackage  com_content
- * @since       1.6
+ * @since  1.6
  */
 class ContentControllerArticle extends JControllerForm
 {
@@ -65,10 +63,8 @@ protected function allowAdd($data = array())
 			// In the absense of better information, revert to the component permissions.
 			return parent::allowAdd();
 		}
-		else
-		{
-			return $allow;
-		}
+
+		return $allow;
 	}
 
 	/**
@@ -110,7 +106,7 @@ protected function allowEdit($data = array(), $key = 'id')
 			}
 
 			// Grant if current user is owner of the record
-			return $user->get('id') == $record->created_by;
+			return $user->id == $record->created_by;
 		}
 
 		return false;
@@ -130,27 +126,12 @@ public function batch($model = null)
 		JSession::checkToken() or jexit(JText::_('JINVALID_TOKEN'));
 
 		// Set the model
+		/** @var ContentModelArticle $model */
 		$model = $this->getModel('Article', '', array());
 
 		// Preset the redirect
 		$this->setRedirect(JRoute::_('index.php?option=com_content&view=articles' . $this->getRedirectToListAppend(), false));
 
 		return parent::batch($model);
 	}
-
-	/**
-	 * Function that allows child controller access to model data after the data has been saved.
-	 *
-	 * @param   JModelLegacy  $model      The data model object.
-	 * @param   array         $validData  The validated data.
-	 *
-	 * @return	void
-	 *
-	 * @since	3.1
-	 */
-	protected function postSaveHook(JModelLegacy $model, $validData = array())
-	{
-
-		return;
-	}
 }

administrator/components/com_content/controllers/articles.php

@@ -23,7 +23,7 @@ class ContentControllerArticles extends JControllerAdmin
 	 *
 	 * @param   array  $config  An optional associative array of configuration settings.
 	 *
-	 * @see     JController
+	 * @see     JControllerLegacy
 	 * @since   1.6
 	 */
 	public function __construct($config = array())
@@ -76,6 +76,7 @@ public function featured()
 		else
 		{
 			// Get the model.
+			/** @var ContentModelArticle $model */
 			$model = $this->getModel();
 
 			// Publish the items.
@@ -113,27 +114,12 @@ public function featured()
 	 * @param   string  $prefix  The class prefix. Optional.
 	 * @param   array   $config  The array of possible config values. Optional.
 	 *
-	 * @return  JModel
+	 * @return  JModelLegacy
 	 *
 	 * @since   1.6
 	 */
 	public function getModel($name = 'Article', $prefix = 'ContentModel', $config = array('ignore_request' => true))
 	{
 		return parent::getModel($name, $prefix, $config);
 	}
-
-	/**
-	 * Function that allows child controller access to model data
-	 * after the item has been deleted.
-	 *
-	 * @param   JModelLegacy  $model  The data model object.
-	 * @param   integer       $ids    The array of ids for items being deleted.
-	 *
-	 * @return  void
-	 *
-	 * @since   12.2
-	 */
-	protected function postDeleteHook(JModelLegacy $model, $ids = null)
-	{
-	}
 }

administrator/components/com_content/controllers/featured.php

@@ -51,6 +51,7 @@ public function delete()
 		else
 		{
 			// Get the model.
+			/** @var ContentModelFeature $model */
 			$model = $this->getModel();
 
 			// Remove the items.
@@ -84,7 +85,7 @@ public function publish()
 	 * @param   string  $prefix  The class prefix. Optional.
 	 * @param   array   $config  Configuration array for model. Optional.
 	 *
-	 * @return  object  The model.
+	 * @return  JModelLegacy  The model.
 	 *
 	 * @since   1.6
 	 */

administrator/components/com_content/helpers/html/contentadministrator.php

@@ -23,9 +23,11 @@ abstract class JHtmlContentAdministrator
 	/**
 	 * Render the list of associated items
 	 *
-	 * @param   int  $articleid  The article item id
+	 * @param   integer  $articleid  The article item id
 	 *
 	 * @return  string  The language HTML
+	 *
+	 * @throws  Exception
 	 */
 	public static function association($articleid)
 	{
@@ -60,7 +62,7 @@ public static function association($articleid)
 			}
 			catch (RuntimeException $e)
 			{
-				throw new Exception($e->getMessage(), 500);
+				throw new Exception($e->getMessage(), 500, $e);
 			}
 
 			if ($items)
@@ -70,7 +72,9 @@ public static function association($articleid)
 					$text = strtoupper($item->lang_sef);
 					$url = JRoute::_('index.php?option=com_content&task=article.edit&id=' . (int) $item->id);
 					$tooltipParts = array(
-						JHtml::_('image', 'mod_languages/' . $item->image . '.gif',
+						JHtml::_(
+							'image',
+							'mod_languages/' . $item->image . '.gif',
 							$item->language_title,
 							array('title' => $item->language_title),
 							true
@@ -101,8 +105,8 @@ public static function association($articleid)
 	/**
 	 * Show the feature/unfeature links
 	 *
-	 * @param   int      $value      The state value
-	 * @param   int      $i          Row number
+	 * @param   integer  $value      The state value
+	 * @param   integer  $i          Row number
 	 * @param   boolean  $canChange  Is user allowed to change?
 	 *
 	 * @return  string       HTML code

administrator/components/com_content/models/article.php

@@ -22,24 +22,26 @@
 class ContentModelArticle extends JModelAdmin
 {
 	/**
-	 * @var        string    The prefix to use with controller messages.
-	 * @since   1.6
+	 * The prefix to use with controller messages.
+	 *
+	 * @var    string
+	 * @since  1.6
 	 */
 	protected $text_prefix = 'COM_CONTENT';
 
 	/**
 	 * The type alias for this content type (for example, 'com_content.article').
 	 *
-	 * @var      string
-	 * @since    3.2
+	 * @var    string
+	 * @since  3.2
 	 */
 	public $typeAlias = 'com_content.article';
 
 	/**
 	 * The context used for the associations table
 	 *
-	 * @var      string
-	 * @since    3.4.4
+	 * @var    string
+	 * @since  3.4.4
 	 */
 	protected $associationsContext = 'com_content.item';
 
@@ -60,7 +62,7 @@ protected function batchCopy($value, $pks, $contexts)
 
 		$newIds = array();
 
-		if (!parent::checkCategoryId($categoryId))
+		if (!$this->checkCategoryId($categoryId))
 		{
 			return false;
 		}
@@ -122,7 +124,7 @@ protected function batchCopy($value, $pks, $contexts)
 				return false;
 			}
 
-			parent::createTagsHelper($this->tagsObserver, $this->type, $pk, $this->typeAlias, $this->table);
+			$this->createTagsHelper($this->tagsObserver, $this->type, $pk, $this->typeAlias, $this->table);
 
 			// Store the row.
 			if (!$this->table->store())
@@ -174,9 +176,7 @@ protected function canDelete($record)
 				return false;
 			}
 
-			$user = JFactory::getUser();
-
-			return $user->authorise('core.delete', 'com_content.article.' . (int) $record->id);
+			return JFactory::getUser()->authorise('core.delete', 'com_content.article.' . (int) $record->id);
 		}
 
 		return false;
@@ -200,16 +200,15 @@ protected function canEditState($record)
 		{
 			return $user->authorise('core.edit.state', 'com_content.article.' . (int) $record->id);
 		}
+
 		// New article, so check against the category.
-		elseif (!empty($record->catid))
+		if (!empty($record->catid))
 		{
 			return $user->authorise('core.edit.state', 'com_content.category.' . (int) $record->catid);
 		}
+
 		// Default to component settings if neither article nor category known.
-		else
-		{
-			return parent::canEditState('com_content');
-		}
+		return parent::canEditState();
 	}
 
 	/**
@@ -224,16 +223,14 @@ protected function canEditState($record)
 	protected function prepareTable($table)
 	{
 		// Set the publish date to now
-		$db = $this->getDbo();
-
 		if ($table->state == 1 && (int) $table->publish_up == 0)
 		{
 			$table->publish_up = JFactory::getDate()->toSql();
 		}
 
 		if ($table->state == 1 && intval($table->publish_down) == 0)
 		{
-			$table->publish_down = $db->getNullDate();
+			$table->publish_down = $this->getDbo()->getNullDate();
 		}
 
 		// Increment the content version number.
@@ -301,7 +298,6 @@ public function getItem($pk = null)
 		}
 
 		// Load associated content items
-		$app = JFactory::getApplication();
 		$assoc = JLanguageAssociations::isEnabled();
 
 		if ($assoc)
@@ -328,7 +324,7 @@ public function getItem($pk = null)
 	 * @param   array    $data      Data for the form.
 	 * @param   boolean  $loadData  True if the form is to load its own data (default case), false if not.
 	 *
-	 * @return  mixed  A JForm object on success, false on failure
+	 * @return  JForm|boolean  A JForm object on success, false on failure
 	 *
 	 * @since   1.6
 	 */
@@ -344,16 +340,11 @@ public function getForm($data = array(), $loadData = true)
 
 		$jinput = JFactory::getApplication()->input;
 
-		// The front end calls this model and uses a_id to avoid id clashes so we need to check for that first.
-		if ($jinput->get('a_id'))
-		{
-			$id = $jinput->get('a_id', 0);
-		}
-		// The back end uses id so we use that the rest of the time and set it to 0 by default.
-		else
-		{
-			$id = $jinput->get('id', 0);
-		}
+		/*
+		 * The front end calls this model and uses a_id to avoid id clashes so we need to check for that first.
+		 * The back end uses id so we use that the rest of the time and set it to 0 by default.
+		 */
+		$id = $jinput->get('a_id', $jinput->get('id', 0));
 
 		// Determine correct permissions to check.
 		if ($this->getState('article.id'))
@@ -602,7 +593,6 @@ public function save($data)
 
 		if (parent::save($data))
 		{
-
 			if (isset($data['featured']))
 			{
 				$this->featured($this->getState($this->getName() . '.id'), $data['featured']);
@@ -641,9 +631,9 @@ public function featured($pks, $value = 0)
 		{
 			$db = $this->getDbo();
 			$query = $db->getQuery(true)
-						->update($db->quoteName('#__content'))
-						->set('featured = ' . (int) $value)
-						->where('id IN (' . implode(',', $pks) . ')');
+				->update($db->quoteName('#__content'))
+				->set('featured = ' . (int) $value)
+				->where('id IN (' . implode(',', $pks) . ')');
 			$db->setQuery($query);
 			$db->execute();
 
@@ -652,8 +642,8 @@ public function featured($pks, $value = 0)
 				// Adjust the mapping table.
 				// Clear the existing features settings.
 				$query = $db->getQuery(true)
-							->delete($db->quoteName('#__content_frontpage'))
-							->where('content_id IN (' . implode(',', $pks) . ')');
+					->delete($db->quoteName('#__content_frontpage'))
+					->where('content_id IN (' . implode(',', $pks) . ')');
 				$db->setQuery($query);
 				$db->execute();
 			}
@@ -681,7 +671,6 @@ public function featured($pks, $value = 0)
 
 				if (count($tuples))
 				{
-					$db = $this->getDbo();
 					$columns = array('content_id', 'ordering');
 					$query = $db->getQuery(true)
 						->insert($db->quoteName('#__content_frontpage'))
@@ -717,10 +706,7 @@ public function featured($pks, $value = 0)
 	 */
 	protected function getReorderConditions($table)
 	{
-		$condition = array();
-		$condition[] = 'catid = ' . (int) $table->catid;
-
-		return $condition;
+		return array('catid = ' . (int) $table->catid);
 	}
 
 	/**
@@ -732,7 +718,7 @@ protected function getReorderConditions($table)
 	 *
 	 * @return  void
 	 *
-	 * @since    3.0
+	 * @since   3.0
 	 */
 	protected function preprocessForm(JForm $form, $data, $group = 'content')
 	{