Improvements

3 files changed, 47 insertions, 17 deletions

diff --git a/apps/files_versions/lib/expiration.php b/apps/files_versions/lib/expiration.php
index e1c7f50a20d..02885e823ca 100644
--- a/apps/files_versions/lib/expiration.php
+++ b/apps/files_versions/lib/expiration.php
@@ -27,7 +27,6 @@ use \OCP\AppFramework\Utility\ITimeFactory;
 class Expiration {
 
 	// how long do we keep files a version if no other value is defined in the config file (unit: days)
-	const DEFAULT_RETENTION_OBLIGATION = 30;
 	const NO_OBLIGATION = -1;
 
 	/** @var ITimeFactory */
@@ -116,22 +115,44 @@ class Expiration {
 	private function parseRetentionObligation(){
 		$splitValues = explode(',', $this->retentionObligation);
 		if (!isset($splitValues[0])) {
-			$minValue = self::DEFAULT_RETENTION_OBLIGATION;
+			$minValue = 'auto';
 		} else {
 			$minValue = trim($splitValues[0]);
 		}
 
-		if (!isset($splitValues[1]) && $minValue === 'auto') {
-			$maxValue = 'auto';
-		} elseif (!isset($splitValues[1])) {
-			$maxValue = self::DEFAULT_RETENTION_OBLIGATION;
+		if (!isset($splitValues[1])) {
+			$maxValue = self::NO_OBLIGATION;
 		} else {
 			$maxValue = trim($splitValues[1]);
 		}
 
+		$isValid = true;
+		// Validate
+		if (!ctype_digit($minValue) && $minValue !== 'auto') {
+			$isValid = false;
+			\OC::$server->getLogger()->warning(
+					$minValue . ' is not a valid value for minimal versions retention obligation. Check versions_retention_obligation in your config.php. Falling back to auto.',
+					['app'=>'files_versions']
+			);
+		}
+
+		if (!ctype_digit($maxValue) && $maxValue !== 'auto') {
+			$isValid = false;
+			\OC::$server->getLogger()->warning(
+					$maxValue . ' is not a valid value for maximal versions retention obligation. Check versions_retention_obligation in your config.php. Falling back to auto.',
+					['app'=>'files_versions']
+			);
+		}
+
+		if (!$isValid){
+			$minValue = 'auto';
+			$maxValue = 'auto';
+		}
+
+
 		if ($minValue === 'auto' && $maxValue === 'auto') {
-			// Default: Keep for 30 days but delete anytime if space needed
-			$this->minAge = self::DEFAULT_RETENTION_OBLIGATION;
+			// Default: Delete anytime if space needed
+			$this->minAge = self::NO_OBLIGATION;
 			$this->maxAge = self::NO_OBLIGATION;
 			$this->canPurgeToSaveSpace = true;
 		} elseif ($minValue !== 'auto' && $maxValue === 'auto') {
diff --git a/apps/files_versions/lib/storage.php b/apps/files_versions/lib/storage.php
index 269d43befb8..90fe308e970 100644
--- a/apps/files_versions/lib/storage.php
+++ b/apps/files_versions/lib/storage.php
@@ -480,11 +480,11 @@ class Storage {
 	 * get list of files we want to expire
 	 * @param array $versions list of versions
 	 * @param integer $time
+	 * @param bool $quotaExceeded is versions storage limit reached
 	 * @return array containing the list of to deleted versions and the size of them
 	 */
 	protected static function getExpireList($time, $versions, $quotaExceeded = false) {
-		$application = new Application();
-		$expiration = $application->getContainer()->query('Expiration');
+		$expiration = self::getExpiration();
 
 		if ($expiration->shouldAutoExpire()) {
 			list($toDelete, $size) = self::getAutoExpireList($time, $versions);
@@ -568,8 +568,7 @@ class Storage {
 	 */
 	private static function scheduleExpire($uid, $fileName, $versionsSize = null, $neededSpace = 0) {
 		// let the admin disable auto expire
-		$application = new Application();
-		$expiration = $application->getContainer()->query('Expiration');
+		$expiration = self::getExpiration();
 		if ($expiration->isEnabled()) {
 			$command = new Expire($uid, $fileName, $versionsSize, $neededSpace);
 			\OC::$server->getCommandBus()->push($command);
@@ -586,8 +585,7 @@ class Storage {
 	 */
 	public static function expire($filename, $versionsSize = null, $offset = 0) {
 		$config = \OC::$server->getConfig();
-		$application = new Application();
-		$expiration = $application->getContainer()->query('Expiration');
+		$expiration = self::getExpiration();
 		
 		if($config->getSystemValue('files_versions', Storage::DEFAULTENABLED)=='true' && $expiration->isEnabled()) {
 			list($uid, $filename) = self::getUidAndFilename($filename);
@@ -706,4 +704,13 @@ class Storage {
 		}
 	}
 
+	/**
+	 * Static workaround
+	 * @return Expiration
+	 */
+	protected static function getExpiration(){
+		$application = new Application();
+		return $application->getContainer()->query('Expiration');
+	}
+
 }
diff --git a/apps/files_versions/tests/expiration.php b/apps/files_versions/tests/expiration.php
index 44d911c74b4..fd5e022f689 100644
--- a/apps/files_versions/tests/expiration.php
+++ b/apps/files_versions/tests/expiration.php
@@ -111,13 +111,15 @@ class Expiration_Test extends \PHPUnit_Framework_TestCase {
 	public function configData(){
 		return [
 			[ 'disabled', null, null, null],
-			[ 'auto', Expiration::DEFAULT_RETENTION_OBLIGATION, Expiration::NO_OBLIGATION, true ],
-			[ 'auto,auto', Expiration::DEFAULT_RETENTION_OBLIGATION, Expiration::NO_OBLIGATION, true ],
-			[ 'auto, auto', Expiration::DEFAULT_RETENTION_OBLIGATION, Expiration::NO_OBLIGATION, true ],
+			[ 'auto', Expiration::NO_OBLIGATION, Expiration::NO_OBLIGATION, true ],
+			[ 'auto,auto', Expiration::NO_OBLIGATION, Expiration::NO_OBLIGATION, true ],
+			[ 'auto, auto', Expiration::NO_OBLIGATION, Expiration::NO_OBLIGATION, true ],
 			[ 'auto, 3', Expiration::NO_OBLIGATION, 3, true ],
 			[ '5, auto', 5, Expiration::NO_OBLIGATION, true ],
 			[ '3, 5', 3, 5, false ],
 			[ '10, 3', 10, 10, false ],
+			[ 'g,a,r,b,a,g,e',  Expiration::NO_OBLIGATION, Expiration::NO_OBLIGATION, true ],
+			[ '-3,8',  Expiration::NO_OBLIGATION, Expiration::NO_OBLIGATION, true ]
 		];
 	}