Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[ticket/15276] Use storage in avatars #4894

Merged
merged 34 commits into from Sep 8, 2017
Merged
Show file tree
Hide file tree
Changes from 33 commits
Commits
Show all changes
34 commits
Select commit Hold shift + click to select a range
4ed0de9
[ticket/15276] Use storage in avatars
rubencm Jul 19, 2017
0417482
[ticket/15276] Fix migration
rubencm Jul 20, 2017
ef00bb4
[ticket/15276] Resolve conflicts
rubencm Jul 20, 2017
41af01b
[ticket/15276] Rename exception
rubencm Jul 20, 2017
929e32e
[ticket/15276] Use storage in avatar test
rubencm Jul 21, 2017
ee094dd
[ticket/15276] Remove annotation
rubencm Jul 21, 2017
5a25a6b
[ticket/15276] Remove empty line
rubencm Jul 21, 2017
89f4e12
[ticket/15276] Add service to collection
rubencm Jul 24, 2017
286b1bb
[ticket/15276] Change adapter to provider in config_name
rubencm Jul 28, 2017
4c5114c
[ticket/15276] Use storage to download avatar
rubencm Jul 28, 2017
946f034
[ticket/15276] Add methods to get file info
rubencm Aug 7, 2017
9e018e7
[ticket/15276] Use streams
rubencm Aug 8, 2017
006990f
[ticket/15276] Fix file_info errors
rubencm Aug 8, 2017
8a47fd4
[ticket/15276] Remove old code
rubencm Aug 8, 2017
8d7336e
[ticket/15276] Fix code style
rubencm Aug 8, 2017
400e663
[ticket/15276] Update file_info
rubencm Aug 8, 2017
090ed9b
[ticket/15276] Use finfo to get mimetype
rubencm Aug 9, 2017
0ff80fe
[ticket/15276] Remove avatar_path from acp
rubencm Aug 9, 2017
2afada5
[ticket/15276] Remove avatar_path
rubencm Aug 9, 2017
3c60333
[ticket/15276] Update
rubencm Aug 9, 2017
09856ae
[ticket/15276] Update file_info to get size of images
rubencm Aug 9, 2017
c3e9aa1
[ticket/15276] Use mimetype guesser
rubencm Aug 10, 2017
87229e1
[ticket/15276] Fix typo
rubencm Aug 15, 2017
28060a8
[ticket/15276] Use stream_copy_to_stream
rubencm Aug 18, 2017
da3c9b3
[ticket/15276] Fix code and add phpdoc
rubencm Aug 28, 2017
fe20aa0
[ticket/15276] Fix comments
rubencm Sep 7, 2017
354dda5
[ticket/15276] Use InitGetWrapper
rubencm Sep 7, 2017
bb88666
[ticket/15276] Add missing properties
rubencm Sep 7, 2017
b255209
[ticket/15276] Remove unused dependency
rubencm Sep 7, 2017
5ff182c
[ticket/15276] Add missing dependency
rubencm Sep 7, 2017
9fcf30d
[ticket/15276] Use IniGetWrapper
rubencm Sep 7, 2017
ed28219
[ticket/15276] Revert some changes
rubencm Sep 7, 2017
a8ba4a9
[ticket/15276] Remove unused code
rubencm Sep 7, 2017
443c503
[ticket/15276] Changed annotation
rubencm Sep 7, 2017
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
4 changes: 2 additions & 2 deletions phpBB/config/default/container/services_avatar.yml
Expand Up @@ -61,11 +61,11 @@ services:
- '@config'
- '%core.root_path%'
- '%core.php_ext%'
- '@filesystem'
- '@storage.avatar'
- '@path_helper'
- '@dispatcher'
- '@files.factory'
- '@cache.driver'
- '@php_ini'
calls:
- [set_name, [avatar.driver.upload]]
tags:
Expand Down
12 changes: 12 additions & 0 deletions phpBB/config/default/container/services_storage.yml
@@ -1,4 +1,14 @@
services:

# Storages
storage.avatar:
class: phpbb\storage\storage
arguments:
- '@storage.adapter.factory'
- 'avatar'
tags:
- { name: storage }

# Factory
storage.adapter.factory:
class: phpbb\storage\adapter_factory
Expand Down Expand Up @@ -28,6 +38,8 @@ services:
shared: false
arguments:
- '@filesystem'
- '@upload_imagesize'
- '@mimetype.guesser'
- '%core.root_path%'
tags:
- { name: storage.adapter }
Expand Down
23 changes: 14 additions & 9 deletions phpBB/develop/adjust_avatars.php
Expand Up @@ -19,7 +19,7 @@
$user->setup();

$echos = 0;

if (!isset($config['avatar_salt']))
{
$cache->purge();
Expand All @@ -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');
Copy link
Member

Choose a reason for hiding this comment

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

We should display some translateable error message here i think.

Copy link
Member Author

@rubencm rubencm Aug 28, 2017

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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().

Copy link
Member Author

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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

}

// let's start with the users using a group_avatar.
$sql = 'SELECT group_id, group_avatar
FROM ' . GROUPS_TABLE . '
Expand All @@ -46,16 +51,16 @@
{
$new_avatar_name = adjust_avatar($row['group_avatar'], 'g' . $row['group_id']);
$group_avatars[] = $new_avatar_name;

// failure is probably due to the avatar name already being adjusted
if ($new_avatar_name !== false)
{
$sql = 'UPDATE ' . USERS_TABLE . "
SET user_avatar = '" . $db->sql_escape($new_avatar_name) . "'
WHERE user_avatar = '" . $db->sql_escape($row['group_avatar']) . "'
WHERE user_avatar = '" . $db->sql_escape($row['group_avatar']) . "'
AND user_avatar_type = " . AVATAR_UPLOAD;
$db->sql_query($sql);

$sql = 'UPDATE ' . GROUPS_TABLE . "
SET group_avatar = '" . $db->sql_escape($new_avatar_name) . "'
WHERE group_id = {$row['group_id']}";
Expand All @@ -80,8 +85,8 @@
$db->sql_freeresult($result);

$sql = 'SELECT user_id, username, user_avatar, user_avatar_type
FROM ' . USERS_TABLE . '
WHERE user_avatar_type = ' . AVATAR_UPLOAD . '
FROM ' . USERS_TABLE . '
WHERE user_avatar_type = ' . AVATAR_UPLOAD . '
AND ' . $db->sql_in_set('user_avatar', $group_avatars, true, true);
$result = $db->sql_query($sql);

Expand All @@ -108,7 +113,7 @@
$db->sql_query($sql);
echo '<br /> Failed updating user ' . $row['user_id'] . "\n";
}

if ($echos > 200)
{
echo '<br />' . "\n";
Expand All @@ -131,8 +136,8 @@
function adjust_avatar($old_name, $midfix)
{
global $config, $phpbb_root_path;
$avatar_path = $phpbb_root_path . $config['avatar_path'];

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

Choose a reason for hiding this comment

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

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.

Copy link
Member

@Nicofuma Nicofuma Sep 7, 2017

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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 )

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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

$extension = strtolower(substr(strrchr($old_name, '.'), 1));
$new_name = $config['avatar_salt'] . '_' . $midfix . '.' . $extension;

Expand Down
1 change: 0 additions & 1 deletion phpBB/docs/coding-guidelines.html
Expand Up @@ -264,7 +264,6 @@ <h4>PHPBB_USE_BOARD_URL_PATH</h4>
<li>{T_SUPER_TEMPLATE_PATH} - styles/xxx/template</li>
<li>{T_IMAGES_PATH} - images/</li>
<li>{T_SMILIES_PATH} - $config['smilies_path']/</li>
<li>{T_AVATAR_PATH} - $config['avatar_path']/</li>
<li>{T_AVATAR_GALLERY_PATH} - $config['avatar_gallery_path']/</li>
<li>{T_ICONS_PATH} - $config['icons_path']/</li>
<li>{T_RANKS_PATH} - $config['ranks_path']/</li>
Expand Down
22 changes: 2 additions & 20 deletions phpBB/includes/acp/acp_main.php
Expand Up @@ -502,26 +502,8 @@ function main($id, $mode)

$upload_dir_size = get_formatted_filesize($config['upload_dir_size']);

$avatar_dir_size = 0;

if ($avatar_dir = @opendir($phpbb_root_path . $config['avatar_path']))
{
while (($file = readdir($avatar_dir)) !== false)
{
if ($file[0] != '.' && $file != 'CVS' && strpos($file, 'index.') === false)
{
$avatar_dir_size += filesize($phpbb_root_path . $config['avatar_path'] . '/' . $file);
}
}
closedir($avatar_dir);

$avatar_dir_size = get_formatted_filesize($avatar_dir_size);
}
else
{
// Couldn't open Avatar dir.
$avatar_dir_size = $user->lang['NOT_AVAILABLE'];
}
// Couldn't open Avatar dir.
$avatar_dir_size = $user->lang['NOT_AVAILABLE'];
Copy link
Member Author

Choose a reason for hiding this comment

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

I need here another way to get the avatars total filesize

Copy link
Member

@Nicofuma Nicofuma Aug 15, 2017

Choose a reason for hiding this comment

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

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).

Copy link
Member

Choose a reason for hiding this comment

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

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]

Copy link
Member Author

@rubencm rubencm Aug 15, 2017

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

please create a dedicated ticket for this issue


if ($posts_per_day > $total_posts)
{
Expand Down
2 changes: 0 additions & 2 deletions phpBB/includes/functions.php
Expand Up @@ -4434,7 +4434,6 @@ function page_header($page_title = '', $display_online_list = false, $item_id =
'T_SUPER_TEMPLATE_PATH' => "{$web_path}styles/" . rawurlencode($user->style['style_path']) . '/template',
'T_IMAGES_PATH' => "{$web_path}images/",
'T_SMILIES_PATH' => "{$web_path}{$config['smilies_path']}/",
'T_AVATAR_PATH' => "{$web_path}{$config['avatar_path']}/",
'T_AVATAR_GALLERY_PATH' => "{$web_path}{$config['avatar_gallery_path']}/",
'T_ICONS_PATH' => "{$web_path}{$config['icons_path']}/",
'T_RANKS_PATH' => "{$web_path}{$config['ranks_path']}/",
Expand All @@ -4452,7 +4451,6 @@ function page_header($page_title = '', $display_online_list = false, $item_id =
'T_SUPER_TEMPLATE_NAME' => rawurlencode((isset($user->style['style_parent_tree']) && $user->style['style_parent_tree']) ? $user->style['style_parent_tree'] : $user->style['style_path']),
'T_IMAGES' => 'images',
'T_SMILIES' => $config['smilies_path'],
'T_AVATAR' => $config['avatar_path'],
'T_AVATAR_GALLERY' => $config['avatar_gallery_path'],
'T_ICONS' => $config['icons_path'],
'T_RANKS' => $config['ranks_path'],
Expand Down
1 change: 0 additions & 1 deletion phpBB/includes/functions_acp.php
Expand Up @@ -88,7 +88,6 @@ function adm_page_header($page_title)

'T_IMAGES_PATH' => "{$phpbb_root_path}images/",
'T_SMILIES_PATH' => "{$phpbb_root_path}{$config['smilies_path']}/",
'T_AVATAR_PATH' => "{$phpbb_root_path}{$config['avatar_path']}/",
'T_AVATAR_GALLERY_PATH' => "{$phpbb_root_path}{$config['avatar_gallery_path']}/",
'T_ICONS_PATH' => "{$phpbb_root_path}{$config['icons_path']}/",
'T_RANKS_PATH' => "{$phpbb_root_path}{$config['ranks_path']}/",
Expand Down
61 changes: 30 additions & 31 deletions phpBB/includes/functions_download.php
Expand Up @@ -25,30 +25,27 @@
*/
function send_avatar_to_browser($file, $browser)
{
global $config, $phpbb_root_path;
global $config, $phpbb_container;

$prefix = $config['avatar_salt'] . '_';
$image_dir = $config['avatar_path'];
$storage = $phpbb_container->get('storage.avatar');

// Adjust image_dir path (no trailing slash)
if (substr($image_dir, -1, 1) == '/' || substr($image_dir, -1, 1) == '\\')
{
$image_dir = substr($image_dir, 0, -1) . '/';
}
$image_dir = str_replace(array('../', '..\\', './', '.\\'), '', $image_dir);
$prefix = $config['avatar_salt'] . '_';
$file_path = $prefix . $file;

if ($image_dir && ($image_dir[0] == '/' || $image_dir[0] == '\\'))
if ($storage->exists($file_path) && !headers_sent())
{
$image_dir = '';
}
$file_path = $phpbb_root_path . $image_dir . '/' . $prefix . $file;
$file_info = $storage->file_info($file_path);

if ((@file_exists($file_path) && @is_readable($file_path)) && !headers_sent())
{
header('Cache-Control: public');

$image_data = @getimagesize($file_path);
header('Content-Type: ' . image_type_to_mime_type($image_data[2]));
try
{
header('Content-Type: ' . $file_info->mimetype);
}
catch (\phpbb\storage\exception\exception $e)
{
// Just don't send this header
}

if ((strpos(strtolower($browser), 'msie') !== false) && !phpbb_is_greater_ie_version($browser, 7))
{
Expand All @@ -69,24 +66,26 @@ function send_avatar_to_browser($file, $browser)
header('Expires: ' . gmdate('D, d M Y H:i:s', time() + 31536000) . ' GMT');
}

$size = @filesize($file_path);
if ($size)
try
{
header("Content-Length: $size");
header('Content-Length: ' . $file_info->size);
}

if (@readfile($file_path) == false)
catch (\phpbb\storage\exception\exception $e)
{
$fp = @fopen($file_path, 'rb');
// Just don't send this header
}

if ($fp !== false)
{
while (!feof($fp))
{
echo fread($fp, 8192);
}
fclose($fp);
}
try
{
$fp = $storage->read_stream($file_path);
$output = fopen('php://output', 'w+b');
stream_copy_to_stream($fp, $output);
fclose($fp);
fclose($output);
}
catch (\Exception $e)
{
// Send nothing
}

flush();
Expand Down
26 changes: 20 additions & 6 deletions phpBB/includes/functions_user.php
Expand Up @@ -2164,7 +2164,9 @@ function phpbb_style_is_active($style_id)
*/
function avatar_delete($mode, $row, $clean_db = false)
{
global $phpbb_root_path, $config;
global $config, $phpbb_container;

$storage = $phpbb_container->get('storage.avatar');

// Check if the users avatar is actually *not* a group avatar
if ($mode == 'user')
Expand All @@ -2181,11 +2183,16 @@ function avatar_delete($mode, $row, $clean_db = false)
}
$filename = get_avatar_filename($row[$mode . '_avatar']);

if (file_exists($phpbb_root_path . $config['avatar_path'] . '/' . $filename))
try
{
@unlink($phpbb_root_path . $config['avatar_path'] . '/' . $filename);
$storage->delete($filename);

return true;
}
catch (\phpbb\storage\exception\exception $e)
{
// Fail is covered by return statement below
}

return false;
}
Expand Down Expand Up @@ -2505,22 +2512,29 @@ function group_create(&$group_id, $type, $name, $desc, $group_attributes, $allow
*/
function group_correct_avatar($group_id, $old_entry)
{
global $config, $db, $phpbb_root_path;
global $config, $db, $phpbb_container;

$storage = $phpbb_container->get('storage.avatar');

$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
Copy link
Member

Choose a reason for hiding this comment

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

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).

Copy link
Member Author

@rubencm rubencm Aug 28, 2017

Choose a reason for hiding this comment

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

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

{
$this->storage->rename($old_filename, $new_filename);

$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)
{
// If rename fail, dont execute the query
}
}


Expand Down
2 changes: 1 addition & 1 deletion phpBB/install/convert/convertor.php
Expand Up @@ -281,7 +281,7 @@ function convert_data($converter)
$bad_folders = array();

$local_paths = array(
'avatar_path' => path($config['avatar_path']),
'avatar_path' => path($config['storage\\avatar\\config\\path']),
'avatar_gallery_path' => path($config['avatar_gallery_path']),
'icons_path' => path($config['icons_path']),
'ranks_path' => path($config['ranks_path']),
Expand Down
3 changes: 2 additions & 1 deletion phpBB/install/schemas/schema_data.sql
Expand Up @@ -55,7 +55,6 @@ INSERT INTO phpbb_config (config_name, config_value) VALUES ('avatar_max_height'
INSERT INTO phpbb_config (config_name, config_value) VALUES ('avatar_max_width', '90');
INSERT INTO phpbb_config (config_name, config_value) VALUES ('avatar_min_height', '20');
INSERT INTO phpbb_config (config_name, config_value) VALUES ('avatar_min_width', '20');
INSERT INTO phpbb_config (config_name, config_value) VALUES ('avatar_path', 'images/avatars/upload');
INSERT INTO phpbb_config (config_name, config_value) VALUES ('avatar_salt', 'phpbb_avatar');
INSERT INTO phpbb_config (config_name, config_value) VALUES ('board_contact', 'contact@yourdomain.tld');
INSERT INTO phpbb_config (config_name, config_value) VALUES ('board_contact_name', '');
Expand Down Expand Up @@ -288,6 +287,8 @@ INSERT INTO phpbb_config (config_name, config_value) VALUES ('exts_composer_json
INSERT INTO phpbb_config (config_name, config_value) VALUES ('exts_composer_vendor_dir', 'vendor-ext/');
INSERT INTO phpbb_config (config_name, config_value) VALUES ('exts_composer_enable_on_install', '0');
INSERT INTO phpbb_config (config_name, config_value) VALUES ('exts_composer_purge_on_remove', '1');
INSERT INTO phpbb_config (config_name, config_value) VALUES ('storage\avatar\provider', 'phpbb\storage\provider\local');
INSERT INTO phpbb_config (config_name, config_value) VALUES ('storage\avatar\config\path', 'images/avatars/upload');

INSERT INTO phpbb_config (config_name, config_value, is_dynamic) VALUES ('cache_last_gc', '0', 1);
INSERT INTO phpbb_config (config_name, config_value, is_dynamic) VALUES ('cron_lock', '0', 1);
Expand Down