Refactor entryController

- Coding style
- Refactoring
- Comments (set of TODO)

See https://github.com/marienfressinaud/FreshRSS/issues/655

app/Controllers/entryController.php

@@ -1,6 +1,14 @@
 <?php
 
+/**
+ * Controller to handle every entry actions.
+ */
 class FreshRSS_entry_Controller extends Minz_ActionController {
+	/**
+	 * This action is called before every other action in that class. It is
+	 * the common boiler plate for every action. It is triggered by the
+	 * underlying framework.
+	 */
 	public function firstAction() {
 		if (!$this->view->loginOk) {
 			Minz_Error::error(
@@ -9,119 +17,153 @@ class FreshRSS_entry_Controller extends Minz_ActionController {
 			);
 		}
 
+		// Keep parameter information (output) to do a correct redirection at
+		// the end.
 		$this->params = array();
 		$output = Minz_Request::param('output', '');
-		if (($output != '') && ($this->view->conf->view_mode !== $output)) {
+		if ($output != '' && $this->view->conf->view_mode !== $output) {
 			$this->params['output'] = $output;
 		}
 
-		$this->redirect = false;
-		$ajax = Minz_Request::param('ajax');
-		if ($ajax) {
+		// If ajax request, we do not print layout
+		$this->ajax = Minz_Request::param('ajax');
+		if ($this->ajax) {
 			$this->view->_useLayout(false);
-		}
-	}
-
-	public function lastAction() {
-		$ajax = Minz_Request::param('ajax');
-		if (!$ajax && $this->redirect) {
-			Minz_Request::forward(array(
-				'c' => 'index',
-				'a' => 'index',
-				'params' => $this->params
-			), true);
-		} else {
 			Minz_Request::_param('ajax');
 		}
 	}
 
+	/**
+	 * Mark one or several entries as read (or not!).
+	 *
+	 * If request concerns several entries, it MUST be a POST request.
+	 * If request concerns several entries, only mark them as read is available.
+	 *
+	 * Parameters are:
+	 *   - id (default: false)
+	 *   - get (default: false) /(c_\d+|f_\d+|s|a)/
+	 *   - nextGet (default: $get)
+	 *   - idMax (default: 0)
+	 *   - is_read (default: true)
+	 *
+	 * @todo nextGet system should not be present here... or should be?
+	 */
 	public function readAction() {
-		$this->redirect = true;
-
 		$id = Minz_Request::param('id');
 		$get = Minz_Request::param('get');
-		$nextGet = Minz_Request::param('nextGet', $get);
-		$idMax = Minz_Request::param('idMax', 0);
+		$next_get = Minz_Request::param('nextGet', $get);
+		$id_max = Minz_Request::param('idMax', 0);
 
 		$entryDAO = FreshRSS_Factory::createEntryDao();
-		if ($id == false) {
+		if ($id === false) {
+			// id is false? It MUST be a POST request!
 			if (!Minz_Request::isPost()) {
 				return;
 			}
 
 			if (!$get) {
-				$entryDAO->markReadEntries($idMax);
+				// No get? Mark all entries as read (from $id_max)
+				$entryDAO->markReadEntries($id_max);
 			} else {
-				$typeGet = $get[0];
+				$type_get = $get[0];
 				$get = substr($get, 2);
-				switch($typeGet) {
+				switch($type_get) {
 				case 'c':
-					$entryDAO->markReadCat($get, $idMax);
+					$entryDAO->markReadCat($get, $id_max);
 					break;
 				case 'f':
-					$entryDAO->markReadFeed($get, $idMax);
+					$entryDAO->markReadFeed($get, $id_max);
 					break;
 				case 's':
-					$entryDAO->markReadEntries($idMax, true);
+					$entryDAO->markReadEntries($id_max, true);
 					break;
 				case 'a':
-					$entryDAO->markReadEntries($idMax);
+					$entryDAO->markReadEntries($id_max);
 					break;
 				}
-				if ($nextGet !== 'a') {
-					$this->params['get'] = $nextGet;
+
+				if ($next_get !== 'a') {
+					// Redirect to the correct page (category, feed or starred)
+					// Not "a" because it is the default value if nothing is
+					// given.
+					$this->params['get'] = $next_get;
 				}
 			}
-
-			$notif = array(
-				'type' => 'good',
-				'content' => _t('feeds_marked_read')
-			);
-			Minz_Session::_param('notification', $notif);
 		} else {
 			$is_read = (bool)(Minz_Request::param('is_read', true));
 			$entryDAO->markRead($id, $is_read);
 		}
+
+		if (!$this->ajax) {
+			Minz_Request::good(_t('feeds_marked_read'), array(
+				'c' => 'index',
+				'a' => 'index',
+				'params' => $this->params,
+			), true);
+		}
 	}
 
+	/**
+	 * This action marks an entry as favourite (bookmark) or not.
+	 *
+	 * Parameter is:
+	 *   - id (default: false)
+	 *   - is_favorite (default: true)
+	 * If id is false, nothing happened.
+	 */
 	public function bookmarkAction() {
-		$this->redirect = true;
-
 		$id = Minz_Request::param('id');
-		if ($id) {
+		$is_favourite = (bool)Minz_Request::param('is_favorite', true);
+		if ($id !== false) {
 			$entryDAO = FreshRSS_Factory::createEntryDao();
-			$entryDAO->markFavorite($id, (bool)(Minz_Request::param('is_favorite', true)));
+			$entryDAO->markFavorite($id, $is_favourite);
+		}
+
+		if (!$this->ajax) {
+			Minz_Request::forward(array(
+				'c' => 'index',
+				'a' => 'index',
+				'params' => $this->params,
+			), true);
 		}
 	}
 
+	/**
+	 * This action optimizes database to reduce its size.
+	 *
+	 * This action shouldbe reached by a POST request.
+	 *
+	 * @todo move this action in configure controller.
+	 * @todo call this action through web-cron when available
+	 */
 	public function optimizeAction() {
-		if (Minz_Request::isPost()) {
-			@set_time_limit(300);
+		$url_redirect = array(
+			'c' => 'configure',
+			'a' => 'archiving',
+		);
 
-			// La table des entrées a tendance à grossir énormément
-			// Cette action permet d'optimiser cette table permettant de grapiller un peu de place
-			// Cette fonctionnalité n'est à appeler qu'occasionnellement
-			$entryDAO = FreshRSS_Factory::createEntryDao();
-			$entryDAO->optimizeTable();
+		if (!Minz_Request::isPost()) {
+			Minz_Request::forward($url_redirect, true);
+		}
 
-			$feedDAO = FreshRSS_Factory::createFeedDao();
-			$feedDAO->updateCachedValues();
+		@set_time_limit(300);
 
-			invalidateHttpCache();
+		$entryDAO = FreshRSS_Factory::createEntryDao();
+		$entryDAO->optimizeTable();
 
-			$notif = array(
-				'type' => 'good',
-				'content' => _t('optimization_complete')
-			);
-			Minz_Session::_param('notification', $notif);
-		}
+		$feedDAO = FreshRSS_Factory::createFeedDao();
+		$feedDAO->updateCachedValues();
 
-		Minz_Request::forward(array(
-			'c' => 'configure',
-			'a' => 'archiving'
-		), true);
+		invalidateHttpCache();
+		Minz_Request::good(_t('optimization_complete'), $url_redirect);
 	}
 
+	/**
+	 * This action purges old entries from feeds.
+	 *
+	 * @todo should be a POST request
+	 * @todo should be in feedController
+	 */
 	public function purgeAction() {
 		@set_time_limit(300);
 
@@ -130,21 +172,23 @@ class FreshRSS_entry_Controller extends Minz_ActionController {
 
 		$feedDAO = FreshRSS_Factory::createFeedDao();
 		$feeds = $feedDAO->listFeeds();
-		$nbTotal = 0;
+		$nb_total = 0;
 
 		invalidateHttpCache();
 
 		foreach ($feeds as $feed) {
-			$feedHistory = $feed->keepHistory();
-			if ($feedHistory == -2) {	//default
-				$feedHistory = $this->view->conf->keep_history_default;
+			$feed_history = $feed->keepHistory();
+			if ($feed_history == -2) {
+				// TODO: -2 must be a constant!
+				// -2 means we take the default value from configuration
+				$feed_history = $this->view->conf->keep_history_default;
 			}
-			if ($feedHistory >= 0) {
-				$nb = $feedDAO->cleanOldEntries($feed->id(), $date_min, $feedHistory);
+
+			if ($feed_history >= 0) {
+				$nb = $feedDAO->cleanOldEntries($feed->id(), $date_min, $feed_history);
 				if ($nb > 0) {
-					$nbTotal += $nb;
+					$nb_total += $nb;
 					Minz_Log::debug($nb . ' old entries cleaned in feed [' . $feed->url() . ']');
-					//$feedDAO->updateLastUpdate($feed->id());
 				}
 			}
 		}
@@ -152,16 +196,9 @@ class FreshRSS_entry_Controller extends Minz_ActionController {
 		$feedDAO->updateCachedValues();
 
 		invalidateHttpCache();
-
-		$notif = array(
-			'type' => 'good',
-			'content' => _t('purge_completed', $nbTotal)
-		);
-		Minz_Session::_param('notification', $notif);
-
-		Minz_Request::forward(array(
+		Minz_Request::good(_t('purge_completed', $nb_total), array(
 			'c' => 'configure',
 			'a' => 'archiving'
-		), true);
+		));
 	}
 }

app/Controllers/feedController.php

@@ -332,7 +332,7 @@ class FreshRSS_feed_Controller extends Minz_ActionController {
 			}
 
 			if ($feed_history >= 0 && rand(0, 30) === 1) {
-				// TODO: move this function in web cron when available
+				// TODO: move this function in web cron when available (see entry::purge)
 				// Remove old entries once in 30.
 				if (!$feedDAO->hasTransaction()) {
 					$feedDAO->beginTransaction();