Merge pull request #6067 from rxu/ticket/16604-master

[ticket/16604] Display warning messages on storage path setting issues

phpBB/adm/style/acp_storage.html

@@ -27,6 +27,13 @@
 	</tbody>
 </table>
 
+{% if S_ERROR %}
+	<div class="errorbox">
+		<h3>{{ lang('WARNING') }}</h3>
+		<p>{{ ERROR_MSG }}</p>
+	</div>
+{% endif %}
+
 <form id="acp_storage" method="post" action="{{ U_ACTION }}">
 
 	{% for storage in STORAGES %}

phpBB/includes/acp/acp_storage.php

@@ -42,9 +42,15 @@ class acp_storage
 	/** @var \phpbb\di\service_collection */
 	protected $storage_collection;
 
+	/** @var \phpbb\filesystem\filesystem */
+	protected $filesystem;
+
 	/** @var string */
 	public $page_title;
 
+	/** @var string */
+	public $phpbb_root_path;
+
 	/** @var string */
 	public $tpl_name;
 
@@ -57,15 +63,17 @@ class acp_storage
 	 */
 	public function main($id, $mode)
 	{
-		global $phpbb_container, $phpbb_dispatcher;
+		global $phpbb_container, $phpbb_dispatcher, $phpbb_root_path;
 
 		$this->config = $phpbb_container->get('config');
+		$this->filesystem = $phpbb_container->get('filesystem');
 		$this->lang = $phpbb_container->get('language');
 		$this->request = $phpbb_container->get('request');
 		$this->template = $phpbb_container->get('template');
 		$this->user = $phpbb_container->get('user');
 		$this->provider_collection = $phpbb_container->get('storage.provider_collection');
 		$this->storage_collection = $phpbb_container->get('storage.storage_collection');
+		$this->phpbb_root_path = $phpbb_root_path;
 
 		// Add necesary language files
 		$this->lang->add_lang(['acp/storage']);
@@ -96,10 +104,10 @@ class acp_storage
 		// Set page title
 		$this->page_title = 'STORAGE_TITLE';
 
+		$messages = [];
 		if ($this->request->is_set_post('submit'))
 		{
 			$modified_storages = [];
-			$messages = [];
 
 			if (!check_form_key($form_key))
 			{
@@ -112,6 +120,8 @@ class acp_storage
 
 				$options = $this->get_provider_options($this->get_current_provider($storage_name));
 
+				$this->validate_path($storage_name, $options, $messages);
+
 				$modified = false;
 
 				// Check if provider have been modified
@@ -165,6 +175,11 @@ class acp_storage
 		$storage_stats = [];
 		foreach ($this->storage_collection as $storage)
 		{
+			$storage_name = $storage->get_name();
+			$options = $this->get_provider_options($this->get_current_provider($storage_name));
+
+			$this->validate_path($storage_name, $options, $messages);
+
 			try
 			{
 				$free_space = get_formatted_filesize($storage->free_space());
@@ -182,11 +197,14 @@ class acp_storage
 			];
 		}
 
-		$this->template->assign_vars(array(
+		$this->template->assign_vars([
 			'STORAGES' => $this->storage_collection,
 			'STORAGE_STATS' => $storage_stats,
 			'PROVIDERS' => $this->provider_collection,
-		));
+
+			'ERROR_MSG'	=> implode('<br />', $messages),
+			'S_ERROR'	=> !empty($messages),
+		]);
 	}
 
 	/**
@@ -336,4 +354,29 @@ class acp_storage
 			$this->config->set('storage\\' . $storage_name . '\\config\\' . $definition, $this->get_new_definition($storage_name, $definition));
 		}
 	}
+
+	/**
+	 * Validates path
+	 *
+	 * @param string $storage_name Storage name
+	 * @param array $options Storage provider configuration keys
+	 * @param array $messages Error messages array
+	 * @return array $messages Reference to messages array
+	 */
+	protected function validate_path($storage_name, $options, &$messages)
+	{
+		if ($this->provider_collection->get_by_class($this->get_current_provider($storage_name))->get_name() == 'local' && isset($options['path']))
+		{
+			$path = $this->request->is_set_post('submit') ? $this->get_new_definition($storage_name, 'path') : $this->get_current_definition($storage_name, 'path');
+
+			if (empty($path))
+			{
+				$messages[] = $this->lang->lang('STORAGE_PATH_NOT_SET', $this->lang->lang('STORAGE_' . strtoupper($storage_name) . '_TITLE'));
+			}
+			else if (!$this->filesystem->is_writable($this->phpbb_root_path . $path) || !$this->filesystem->exists($this->phpbb_root_path . $path))
+			{
+				$messages[] = $this->lang->lang('STORAGE_PATH_NOT_EXISTS', $this->lang->lang('STORAGE_' . strtoupper($storage_name) . '_TITLE'));
+			}
+		}
+	}
 }

phpBB/language/en/acp/storage.php

@@ -68,4 +68,7 @@ $lang = array_merge($lang, array(
 	'STORAGE_FORM_TYPE_EMAIL_INCORRECT_FORMAT'	=>	'Incorrect email for %s of %s.',
 	'STORAGE_FORM_TYPE_TEXT_TOO_LONG'			=>	'Text is too long for %s of %s.',
 	'STORAGE_FORM_TYPE_SELECT_NOT_AVAILABLE'	=>	'Selected value is not available for %s of %s.',
+
+	'STORAGE_PATH_NOT_EXISTS'		=> '“%1$s” path does not exist or is not writable.',
+	'STORAGE_PATH_NOT_SET'			=> '“%1$s” path is not set.',
 ));

phpBB/language/en/common.php

@@ -110,8 +110,6 @@ $lang = array_merge($lang, array(
 	'AVATAR_NOT_UPLOADED'			=> 'Avatar could not be uploaded.',
 	'AVATAR_NO_TEMP_DIR'			=> 'Temporary folder could not be found or is not writable.',
 	'AVATAR_NO_SIZE'				=> 'The width or height of the linked avatar could not be determined. Please enter them manually.',
-	'AVATAR_NO_UPLOAD_DIR'			=> 'Avatar storage path does not exist or is not writable.',
-	'AVATAR_NO_UPLOAD_PATH'			=> 'Avatar uploading is enabled but avatar storage path is not set.',
 	'AVATAR_PARTIAL_UPLOAD'			=> 'The specified file was only partially uploaded.',
 	'AVATAR_PHP_SIZE_NA'			=> 'The avatar’s filesize is too large.<br />The maximum allowed filesize set in php.ini could not be determined.',
 	'AVATAR_PHP_SIZE_OVERRUN'		=> 'The avatar’s filesize is too large. The maximum allowed upload size is %1$d %2$s.<br />Please note this is set in php.ini and cannot be overridden.',

tests/functional/acp_avatar_settings_test.php

@@ -1,45 +0,0 @@
-<?php
-/**
-*
-* This file is part of the phpBB Forum Software package.
-*
-* @copyright (c) phpBB Limited <https://www.phpbb.com>
-* @license GNU General Public License, version 2 (GPL-2.0)
-*
-* For full copyright and license information, please see
-* the docs/CREDITS.txt file.
-*
-*/
-
-/**
-* @group functional
-*/
-class phpbb_functional_acp_avatar_settings_test extends phpbb_functional_test_case
-{
-	public function test_avatar_upload_settings()
-	{
-		$this->add_lang(['acp/common', 'acp/board']);
-		$this->login();
-		$this->admin_login();
-
-		$crawler = self::request('GET', 'adm/index.php?i=acp_board&mode=avatar&sid=' . $this->sid);
-		$this->assertContainsLang('ACP_AVATAR_SETTINGS', $this->get_content());
-		$this->assertContainsLang('ACP_AVATAR_SETTINGS_EXPLAIN', $this->get_content());
-
-		// Test disabling avatar uploading - valid
-		$form = $crawler->selectButton($this->lang('SUBMIT'))->form([
-			'config[allow_avatar_upload]' => '0'
-		]);
-		$crawler = self::submit($form);
-		$this->assertContainsLang('CONFIG_UPDATED', $crawler->filter('div[class="successbox"] > p')->text());
-
-		$crawler = self::request('GET', 'adm/index.php?i=acp_board&mode=avatar&sid=' . $this->sid);
-
-		// Test enabling avatar uploading - valid
-		$form = $crawler->selectButton($this->lang('SUBMIT'))->form([
-			'config[allow_avatar_upload]' => '1'
-		]);
-		$crawler = self::submit($form);
-		$this->assertContainsLang('CONFIG_UPDATED', $crawler->filter('div[class="successbox"] > p')->text());
-	}
-}

tests/functional/acp_storage_settings_test.php

@@ -0,0 +1,93 @@
+<?php
+/**
+*
+* This file is part of the phpBB Forum Software package.
+*
+* @copyright (c) phpBB Limited <https://www.phpbb.com>
+* @license GNU General Public License, version 2 (GPL-2.0)
+*
+* For full copyright and license information, please see
+* the docs/CREDITS.txt file.
+*
+*/
+
+/**
+* @group functional
+*/
+class phpbb_functional_acp_storage_settings_test extends phpbb_functional_test_case
+{
+	public function test_storage_settings()
+	{
+		$this->add_lang(['common', 'acp/storage']);
+		$this->login();
+		$this->admin_login();
+
+		$crawler = self::request('GET', 'adm/index.php?i=acp_storage&mode=settings&sid=' . $this->sid);
+		$this->assertContainsLang('STORAGE_TITLE', $this->get_content());
+		$this->assertContainsLang('STORAGE_TITLE_EXPLAIN', $this->get_content());
+
+		$form = $crawler->selectButton($this->lang('SUBMIT'))->form();
+		$crawler = self::submit($form);
+		$this->assertContainsLang('INFORMATION', $crawler->filter('div[class="errorbox"] > h3')->text());
+		$this->assertContainsLang('STORAGE_NO_CHANGES', $crawler->filter('div[class="errorbox"] > p')->text());
+
+		// Test empty storage paths - invalid
+		$crawler = self::request('GET', 'adm/index.php?i=acp_storage&mode=settings&sid=' . $this->sid);
+		$form = $crawler->selectButton($this->lang('SUBMIT'))->form([
+			'attachment[path]' => '',
+			'avatar[path]' => '',
+			'backup[path]' => '',
+		]);
+		$crawler = self::submit($form);
+		$this->assertContainsLang('INFORMATION', $crawler->filter('div[class="errorbox"] > h3')->text());
+		$this->assertStringContainsString($this->lang('STORAGE_PATH_NOT_SET', $this->lang('STORAGE_ATTACHMENT_TITLE')), $crawler->filter('div[class="errorbox"] > p')->text());
+		$this->assertStringContainsString($this->lang('STORAGE_PATH_NOT_SET', $this->lang('STORAGE_AVATAR_TITLE')), $crawler->filter('div[class="errorbox"] > p')->text());
+		$this->assertStringContainsString($this->lang('STORAGE_PATH_NOT_SET', $this->lang('STORAGE_BACKUP_TITLE')), $crawler->filter('div[class="errorbox"] > p')->text());
+
+		// Test storage paths became not writable on the server afterwards
+		// Unix tests only
+		if (!defined('PHP_WINDOWS_VERSION_MAJOR'))
+		{
+			$crawler = self::request('GET', 'adm/index.php?i=acp_storage&mode=settings&sid=' . $this->sid);
+			$form = $crawler->selectButton($this->lang('SUBMIT'))->form();
+			$values = $form->getValues();
+
+			$attachments_storage_path = $values['attachment[path]'];
+			$avatar_upload_path = $values['avatar[path]'];
+			$backup_storage_path = $values['backup[path]'];
+
+			$filesystem = new \phpbb\filesystem\filesystem;
+
+			// Make the directory not writable
+			global $phpbb_root_path;
+			$filesystem->chmod($phpbb_root_path . $attachments_storage_path, 444);
+			$filesystem->chmod($phpbb_root_path . $avatar_upload_path, 444);
+			$filesystem->chmod($phpbb_root_path . $backup_storage_path, 444);
+			$this->assertFalse($filesystem->is_writable($phpbb_root_path . $avatar_upload_path));
+
+			// Visit ACP Storage settings again - warning should be displayed
+			$crawler = self::request('GET', 'adm/index.php?i=acp_storage&mode=settings&sid=' . $this->sid);
+			$this->assertContainsLang('WARNING', $crawler->filter('div[class="errorbox"] > h3')->text());
+			$this->assertStringContainsString($this->lang('STORAGE_PATH_NOT_EXISTS', $this->lang('STORAGE_ATTACHMENT_TITLE')), $crawler->filter('div[class="errorbox"] > p')->text());
+			$this->assertStringContainsString($this->lang('STORAGE_PATH_NOT_EXISTS', $this->lang('STORAGE_AVATAR_TITLE')), $crawler->filter('div[class="errorbox"] > p')->text());
+			$this->assertStringContainsString($this->lang('STORAGE_PATH_NOT_EXISTS', $this->lang('STORAGE_BACKUP_TITLE')), $crawler->filter('div[class="errorbox"] > p')->text());
+
+			// Restore default state
+			$filesystem->chmod($phpbb_root_path . $attachments_storage_path, 777);
+			$filesystem->chmod($phpbb_root_path . $avatar_upload_path, 777);
+			$filesystem->chmod($phpbb_root_path . $backup_storage_path, 777);
+			$this->assertTrue($filesystem->is_writable($phpbb_root_path . $attachments_storage_path));
+			$this->assertTrue($filesystem->is_writable($phpbb_root_path . $avatar_upload_path));
+			$this->assertTrue($filesystem->is_writable($phpbb_root_path . $backup_storage_path));
+
+			$crawler = self::request('GET', 'adm/index.php?i=acp_storage&mode=settings&sid=' . $this->sid);
+			$this->assertStringNotContainsString($this->lang('STORAGE_PATH_NOT_SET', $this->lang('STORAGE_ATTACHMENT_TITLE')), $this->get_content());
+			$this->assertStringNotContainsString($this->lang('STORAGE_PATH_NOT_SET', $this->lang('STORAGE_AVATAR_TITLE')), $this->get_content());
+			$this->assertStringNotContainsString($this->lang('STORAGE_PATH_NOT_SET', $this->lang('STORAGE_BACKUP_TITLE')), $this->get_content());
+
+			$this->assertStringNotContainsString($this->lang('STORAGE_PATH_NOT_EXISTS', $this->lang('STORAGE_ATTACHMENT_TITLE')), $this->get_content());
+			$this->assertStringNotContainsString($this->lang('STORAGE_PATH_NOT_EXISTS', $this->lang('STORAGE_AVATAR_TITLE')), $this->get_content());
+			$this->assertStringNotContainsString($this->lang('STORAGE_PATH_NOT_EXISTS', $this->lang('STORAGE_BACKUP_TITLE')), $this->get_content());
+		}
+	}
+}