[ticket/15276] Use storage in avatars #4894

rubencm · 2017-08-08T10:47:40Z

Checklist:

Correct branch: master for new features; 3.2.x, 3.1.x for fixes
Tests pass
Code follows coding guidelines: master / 3.2.x, 3.1.x
Commit follows commit message format

This depends on #4893

Tracker ticket (set the ticket ID to your ticket ID):

https://tracker.phpbb.com/browse/PHPBB3-15276

rubencm · 2017-08-08T12:41:13Z

phpBB/phpbb/storage/adapter/local.php

+
+	public function file_mimetype($path)
+	{
+		return mime_content_type($this->root_path . $path);


I am looking that this function is obsolete, but i dont find much information about Fileinfo

rubencm · 2017-08-09T11:44:19Z

phpBB/includes/acp/acp_main.php

-			$avatar_dir_size = $user->lang['NOT_AVAILABLE'];
-		}
+		// Couldn't open Avatar dir.
+		$avatar_dir_size = $user->lang['NOT_AVAILABLE'];


I need here another way to get the avatars total filesize

I think this is just not possible with most cloud providers. We can check if the local adapter is in use, but otherwise I think you may need to use your cloud provider web interface (and even if it is possible we can't display the avatars usage but only the global one, if you use the same S3 bucket for many things per example).

Actually it may be possible for some providers and I think it can still be useful (even if it displays the global usage).
You could add a get_total_usage or something like that in the provider which will return an array containing the current total usage and the maximum allowed usage (if it exists). If one information is missing its value being replaced by false.

Example: (all numbers are in bytes)
return ['usage' => 9999, 'max' => 10000]
return ['usage' => 9999, 'max' => false]
return ['usage' => false, 'max' => false]

Yes, but i cannot get the usage easily.
I think i need to create a provider for the storages, with methods to get the list of files, or the total size.
And in the adapter a method to get the total available space.
Or something like that, i dont know

well, it depends what we want to do.
If we want to track all files then we need a new table yes (we can have a singe table for all storages) but if we just want the current usage of the cloud storage then we can rely on the adapter directly.

I dont know, i still think that is better to do a class for each storage with methods to get the files or somehing like that

I'm sorry but I don't understand what you have in mind.

ok, im going to do a concept, but i have to finish other things first

please create a dedicated ticket for this issue

rubencm · 2017-08-09T11:45:00Z

Im pretty sure that i have broken the conversion from phpbb2

rubencm · 2017-08-09T13:00:08Z

phpBB/includes/functions_download.php

-					echo fread($fp, 8192);
-				}
-				fclose($fp);
+				echo fread($fp, 8192);


Maybe is better to do fwrite('php://output', fread($fp, 8192)); ?

actually as we have a stream we can use stream_copy_to_stream($fp, 'php://output') http://php.net/manual/fr/function.stream-copy-to-stream.php

Nicofuma · 2017-08-15T08:35:08Z

phpBB/includes/functions_download.php

-		{
-			header("Content-Length: $size");
+		try {
+			header('Content-Type: ' . $file_info->size);


Content-Length

Nicofuma · 2017-08-15T08:37:58Z

phpBB/phpbb/storage/adapter/local.php

@@ -234,4 +234,19 @@ public function write_stream($path, $resource)
 			throw new exception('STORAGE_CANNOT_COPY_RESOURCE');
 		}
 	}
+
+	public function file_properties($path)


this method should be in the interface

This if from an old commit, i removed this

Nicofuma · 2017-08-15T08:46:04Z

phpBB/phpbb/storage/adapter/local.php

+	 */
+	public function file_image_width($path)
+	{
+		return $this->image_dimensions($path);


return $this->image_dimensions($path)['image_width'];

No, this is right, but i think i will remove file_info, i dont like how it works

But now this won't return the actual width but rather just the output of image_dimensions()?

PHPBB3-15276

Nicofuma · 2017-08-25T13:35:23Z

better, thanks

Nicofuma · 2017-08-25T13:36:15Z

@phpbb/development-team it's ready for review and without commits from another PR now

marc1706 · 2017-08-27T08:41:10Z

phpBB/includes/functions_download.php

 		header('Cache-Control: public');

-		$image_data = @getimagesize($file_path);
-		header('Content-Type: ' . image_type_to_mime_type($image_data[2]));
+		try {


Braces on new line please (applies to other parts of the code as well).

marc1706 · 2017-08-27T08:54:16Z

phpBB/phpbb/storage/file_info.php

+	protected $path;
+
+	/**
+	 * @var array


Maybe an explanation of what properties these are?

marc1706 · 2017-08-27T08:54:26Z

phpBB/phpbb/storage/file_info.php

+	 */
+	protected $properties;
+
+	public function __construct($adapter, $path)


Docblock pls

CHItA · 2017-08-28T15:29:14Z

phpBB/develop/adjust_avatars.php

@@ -30,6 +30,11 @@
 	die('database not up to date');
 }

+if (!isset($config['storage\\avatar\\config\\path']) || $config['storage\\avatar\\config\\path'] != 'phpbb\\storage\\provider\\local')


Use !== if possible.

CHItA · 2017-08-28T15:29:53Z

phpBB/develop/adjust_avatars.php

@@ -30,6 +30,11 @@
 	die('database not up to date');
 }

+if (!isset($config['storage\\avatar\\config\\path']) || $config['storage\\avatar\\config\\path'] != 'phpbb\\storage\\provider\\local')
+{
+	die('use local provider');


We should display some translateable error message here i think.

I dont know what is the purpose of this folder, but none of the strings in this file are translatable

CHItA · 2017-08-28T15:35:55Z

phpBB/includes/functions_user.php


 	$group_id		= (int) $group_id;
 	$ext 			= substr(strrchr($old_entry, '.'), 1);
 	$old_filename 	= get_avatar_filename($old_entry);
 	$new_filename 	= $config['avatar_salt'] . "_g$group_id.$ext";
 	$new_entry 		= 'g' . $group_id . '_' . substr(time(), -5) . ".$ext";

-	$avatar_path = $phpbb_root_path . $config['avatar_path'];
-	if (@rename($avatar_path . '/'. $old_filename, $avatar_path . '/' . $new_filename))
+	try


This modification is not exactly the same as what the code did before which is fine for now, but it may cause problems if this needs to be extended in the future. (meaning error handling could get difficult if there are multiple statements that could throw exceptions).

Uhm, i think it is.
The update query is only executed if rename dont throws an exception

CHItA · 2017-08-28T15:38:45Z

phpBB/phpbb/files/types/remote_storage.php

+			return $this->factory->get('filespec')->set_error($this->language->lang($this->upload->error_prefix . 'WRONG_FILESIZE', $max_filesize['value'], $max_filesize['unit']));
+		}
+
+		if ($content_length == 0)


=== instead of double

PHPBB3-15276

Nicofuma · 2017-09-06T17:15:08Z

@phpbb/development-team please review it a last time :)

marc1706 · 2017-09-06T17:21:22Z

phpBB/develop/adjust_avatars.php

@@ -30,6 +30,11 @@
 	die('database not up to date');
 }

+if (!isset($config['storage\\avatar\\config\\path']) || $config['storage\\avatar\\config\\path'] !== 'phpbb\\storage\\provider\\local')
+{
+	die('use local provider');


I think this message should be improved as for someone not knowing the exact purpose of this might not be able to understand why it's causing a die().

I think i can rewrite this to use storage, but i dont know under which circunstances this script is used, so i just prevent to use it after changing storage

Actually I think we can leave it like that for now. We should start working on the dev doc soon and include a link here

marc1706 · 2017-09-06T17:25:14Z

phpBB/includes/functions_download.php

+		}
+		catch (\phpbb\storage\exception\exception $e)
+		{
+			// Just dont send this header


// Just don't send this header

marc1706 · 2017-09-06T17:25:34Z

phpBB/includes/functions_download.php

 		{
-			$fp = @fopen($file_path, 'rb');
+			// Just dont send this header


marc1706 · 2017-09-06T17:27:07Z

phpBB/includes/functions_user.php

 		$sql = 'UPDATE ' . GROUPS_TABLE . '
 			SET group_avatar = \'' . $db->sql_escape($new_entry) . "'
 			WHERE group_id = $group_id";
 		$db->sql_query($sql);
 	}
+	catch (\phpbb\storage\exception\exception $e)
+	{
+


Please add a comment here

marc1706 · 2017-09-06T17:30:32Z

phpBB/phpbb/avatar/driver/upload.php

 	*
 	* @return bool True if user can upload, false if not
 	*/
 	protected function can_upload()
 	{
-		return ($this->filesystem->exists($this->phpbb_root_path . $this->config['avatar_path']) && $this->filesystem->is_writable($this->phpbb_root_path . $this->config['avatar_path']) && (@ini_get('file_uploads') || strtolower(@ini_get('file_uploads')) == 'on'));
+		return (@ini_get('file_uploads') || strtolower(@ini_get('file_uploads')) == 'on');


Not sure if we should be using the bantu's ini class? @Nicofuma

we could use it yes

marc1706 · 2017-09-06T17:36:57Z

phpBB/phpbb/files/types/remote_storage.php

+	 *
+	 * @param string $upload_url URL pointing to file to upload, for example http://www.foobar.com/example.gif
+	 * @return filespec $file Object "filespec" is returned, all further operations can be done with this object
+	 * @access public


It's both wrong and no longer needed IMHO

I don't understand

just trash the @access line

PHPBB3-15276

Elsensee · 2017-09-07T11:00:14Z

phpBB/develop/adjust_avatars.php

-	
-	$avatar_path = $phpbb_root_path . $config['avatar_path'];
+
+	$avatar_path = $phpbb_root_path . $config['storage\\avatar\\config\\path'];


I know, there never was a rule about not having (back-)slashes in config names, but without underscores this somehow looks weird to me and breaking the status quo of having configuration names.

Actually I asked him to do that on purpose. For me as there is a status quo of not using \ we can reserved it for internal usage like that and have a guarantee of 0 conflict ever.

Internal usage means that you're not having any intention to make the path customizable? Then why not mark it as a parameter in a yaml file?

Otherwise I don't understand the term "internal usage" or have a different opinion.

the path is customizable from a core page in the acp
but we have a lot of config values related to storage with dynamic names.
Here per example, avatar and path are dynamics the rest is internal. The full name is build internally (including storage, config and )

As personally don't care for this. It's a nice way to prevent conflicts for sure. As long as it doesn't mess up with some dbal

Yeah, okay.. It just looks weird to me but other than that it doesn't do anything harmful, I guess.

PHPBB3-15276

marc1706 · 2017-09-07T14:10:01Z

phpBB/phpbb/files/types/remote_storage.php

+	 */
+	protected function get_max_file_size()
+	{
+		$max_file_size = $this->upload->max_filesize;


I'm wondering if it would have been better to just simply add this as a method to the upload class but oh well

CHItA · 2017-09-07T14:09:31Z

phpBB/phpbb/files/types/remote_storage.php

+	public function upload()
+	{
+		$args = func_get_args();
+		return $this->remote_upload($args[0]);


I don't really understand why this was done this way. What were the original arguments here?

CHItA · 2017-09-07T14:12:01Z

Otherwise it is looking good to me 👍

PHPBB3-15276

[ticket/15276] Use storage in avatars * github.com:phpbb/phpbb: (34 commits) [ticket/15276] Changed annotation [ticket/15276] Remove unused code [ticket/15276] Revert some changes [ticket/15276] Use IniGetWrapper [ticket/15276] Add missing dependency [ticket/15276] Remove unused dependency [ticket/15276] Add missing properties [ticket/15276] Use InitGetWrapper [ticket/15276] Fix comments [ticket/15276] Fix code and add phpdoc [ticket/15276] Use stream_copy_to_stream [ticket/15276] Fix typo [ticket/15276] Use mimetype guesser [ticket/15276] Update file_info to get size of images [ticket/15276] Update [ticket/15276] Remove avatar_path [ticket/15276] Remove avatar_path from acp [ticket/15276] Use finfo to get mimetype [ticket/15276] Update file_info [ticket/15276] Fix code style ...

rubencm mentioned this pull request Aug 8, 2017

[ticket/15289] Allow to configure storage from acp #4895

Merged

4 tasks

rubencm commented Aug 8, 2017

View reviewed changes

rubencm force-pushed the ticket/15276 branch from 00f0beb to de37630 Compare August 9, 2017 08:43

phpbb-user added the WIP 🚧 label Aug 9, 2017

rubencm commented Aug 9, 2017

View reviewed changes

rubencm force-pushed the ticket/15276 branch 4 times, most recently from 934f8dd to b9b40af Compare August 9, 2017 21:02

phpbb-user removed the WIP 🚧 label Aug 11, 2017

Nicofuma reviewed Aug 15, 2017

View reviewed changes

Nicofuma added the 4.0 (Triton) 🔮 label Aug 15, 2017

Nicofuma modified the milestone: 3.3.0-a1 Aug 15, 2017

rubencm force-pushed the ticket/15276 branch from 20b5d83 to e20cf5f Compare August 23, 2017 15:20

rubencm added 11 commits August 25, 2017 15:31

[ticket/15276] Use storage in avatars

4ed0de9

PHPBB3-15276

[ticket/15276] Fix migration

0417482

PHPBB3-15276

[ticket/15276] Resolve conflicts

ef00bb4

PHPBB3-15276

[ticket/15276] Rename exception

41af01b

PHPBB3-15276

[ticket/15276] Use storage in avatar test

929e32e

PHPBB3-15276

[ticket/15276] Remove annotation

ee094dd

PHPBB3-15276

[ticket/15276] Remove empty line

5a25a6b

PHPBB3-15276

[ticket/15276] Add service to collection

89f4e12

PHPBB3-15276

[ticket/15276] Change adapter to provider in config_name

286b1bb

PHPBB3-15276

[ticket/15276] Use storage to download avatar

4c5114c

PHPBB3-15276

[ticket/15276] Add methods to get file info

946f034

PHPBB3-15276

marc1706 reviewed Aug 27, 2017

View reviewed changes

phpBB/phpbb/storage/file_info.php Outdated

protected $path;

/**

* @var array

Copy link

Member

marc1706 Aug 27, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe an explanation of what properties these are?

marc1706 reviewed Aug 27, 2017

View reviewed changes

phpBB/phpbb/storage/file_info.php Outdated

*/

protected $properties;

public function __construct($adapter, $path)

Copy link

Member

marc1706 Aug 27, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Docblock pls

rubencm force-pushed the ticket/15276 branch from 6b24db4 to f159948 Compare August 28, 2017 13:12

CHItA requested changes Aug 28, 2017

View reviewed changes

[ticket/15276] Fix code and add phpdoc

da3c9b3

PHPBB3-15276

rubencm force-pushed the ticket/15276 branch from f159948 to da3c9b3 Compare August 28, 2017 15:55

marc1706 requested changes Sep 6, 2017

View reviewed changes

[ticket/15276] Fix comments

fe20aa0

PHPBB3-15276

Elsensee reviewed Sep 7, 2017

View reviewed changes

rubencm added 4 commits September 7, 2017 13:29

[ticket/15276] Use InitGetWrapper

354dda5

PHPBB3-15276

[ticket/15276] Add missing properties

bb88666

PHPBB3-15276

[ticket/15276] Remove unused dependency

b255209

PHPBB3-15276

[ticket/15276] Add missing dependency

5ff182c

PHPBB3-15276

rubencm mentioned this pull request Sep 7, 2017

[ticket/15286] Use storage in attachments #4899

Merged

4 tasks

rubencm added 3 commits September 7, 2017 14:50

[ticket/15276] Use IniGetWrapper

9fcf30d

PHPBB3-15276

[ticket/15276] Revert some changes

ed28219

PHPBB3-15276

[ticket/15276] Remove unused code

a8ba4a9

PHPBB3-15276

marc1706 reviewed Sep 7, 2017

View reviewed changes

CHItA reviewed Sep 7, 2017

View reviewed changes

CHItA approved these changes Sep 7, 2017

View reviewed changes

[ticket/15276] Changed annotation

443c503

PHPBB3-15276

rubencm force-pushed the ticket/15276 branch from c7ad6bd to 443c503 Compare September 7, 2017 14:44

marc1706 approved these changes Sep 8, 2017

View reviewed changes

Nicofuma merged commit 443c503 into phpbb:master Sep 8, 2017


		$avatar_path = $phpbb_root_path . $config['avatar_path'];

		$avatar_path = $phpbb_root_path . $config['storage\\avatar\\config\\path'];

[ticket/15276] Use storage in avatars #4894

[ticket/15276] Use storage in avatars #4894

Conversation

rubencm commented Aug 8, 2017 • edited by marc1706

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Nicofuma Aug 15, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rubencm Aug 15, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rubencm commented Aug 9, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Nicofuma commented Aug 25, 2017

Nicofuma commented Aug 25, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rubencm Aug 28, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rubencm Aug 28, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Nicofuma commented Sep 6, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Nicofuma Sep 7, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

CHItA commented Sep 7, 2017

rubencm commented Aug 8, 2017 •

edited by marc1706

Nicofuma Aug 15, 2017 •

edited

rubencm Aug 15, 2017 •

edited

rubencm Aug 28, 2017 •

edited

rubencm Aug 28, 2017 •

edited

Nicofuma Sep 7, 2017 •

edited