Skip to content

Update storage and file tracking #32

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.

Sign up for GitHub

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

Merged
iMattPro merged 10 commits into from
Feb 22, 2025
Merged

Update storage and file tracking #32

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
fbc0517
Make tests on rubencm/ticket/17361
iMattPro Jan 18, 2025
248eff9
Update file tracking stuff
iMattPro Jan 18, 2025
6ab47d2
extend file tracker instead of storage
iMattPro Jan 19, 2025
90de924
Extend track_files
iMattPro Jan 19, 2025
c9fab13
Fixes
iMattPro Jan 19, 2025
84c574e
Remove extended track files method
iMattPro Jan 19, 2025
7f82b40
Revert "Make tests on rubencm/ticket/17361"
iMattPro Feb 22, 2025
7279366
Update tests
iMattPro Feb 22, 2025
4263fdb
Fix quotes
iMattPro Feb 22, 2025
01552c8
Merge branch 'main' into experiemental
iMattPro Feb 22, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion adm/style/acp_pwakit.html
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -84,7 +84,7 @@ <h1>{{ lang('ACP_PWA_KIT_APP_ICONS') }}</h1>
{% set iconName = icon.src|split('/')|last %}
<p style="position: relative;">
<span class="pwa-icon-container">
<img src="{{ icon.src }}" alt="{{ iconName|e("html") }}">
<img src="{{ icon.src }}" alt="{{ iconName|e('html') }}">
<button class="delete-btn" name="delete" value="{{ icon.src }}" title="{{ lang('ACP_PWA_IMG_DELETE') }}">
<span class="fa-stack fa-2x">
{{ Icon('font', 'circle', '', true, 'fa-stack-2x fa-inverse') }}
Expand Down
11 changes: 7 additions & 4 deletions config/services.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ services:
- '@ext.manager'
- '@upload_imagesize'
- '@phpbb.pwakit.storage'
- '@phpbb.pwakit.file_tracker'
- '@storage.helper'
- '%core.root_path%'

Expand All @@ -25,16 +26,18 @@ services:
- '@phpbb.pwakit.storage'

phpbb.pwakit.storage:
class: phpbb\pwakit\storage\storage
class: phpbb\storage\storage
arguments:
- '@dbal.conn'
- '@cache.driver'
- '@storage.adapter.factory'
- '@phpbb.pwakit.file_tracker'
- 'phpbb_pwakit'
- '%tables.storage%'
tags:
- { name: storage }

phpbb.pwakit.file_tracker:
class: phpbb\pwakit\storage\file_tracker
parent: storage.file_tracker

phpbb.pwakit.admin.controller:
class: phpbb\pwakit\controller\admin_controller
arguments:
Expand Down
53 changes: 31 additions & 22 deletions helper/helper.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,8 @@
use FastImageSize\FastImageSize;
use phpbb\exception\runtime_exception;
use phpbb\extension\manager as ext_manager;
use phpbb\pwakit\storage\storage;
use phpbb\pwakit\storage\file_tracker;
use phpbb\storage\storage;
use phpbb\storage\exception\storage_exception;
use phpbb\storage\helper as storage_helper;

Expand All @@ -28,6 +29,9 @@ class helper
/** @var storage */
protected storage $storage;

/** @var file_tracker $file_tracker */
protected file_tracker $file_tracker;

/** @var storage_helper */
protected storage_helper $storage_helper;

Expand All @@ -40,14 +44,16 @@ class helper
* @param ext_manager $extension_manager
* @param FastImageSize $imagesize
* @param storage $storage
* @param file_tracker $file_tracker
* @param storage_helper $storage_helper
* @param string $root_path
*/
public function __construct(ext_manager $extension_manager, FastImageSize $imagesize, storage $storage, storage_helper $storage_helper, string $root_path)
public function __construct(ext_manager $extension_manager, FastImageSize $imagesize, storage $storage, file_tracker $file_tracker, storage_helper $storage_helper, string $root_path)
{
$this->extension_manager = $extension_manager;
$this->imagesize = $imagesize;
$this->storage = $storage;
$this->file_tracker = $file_tracker;
$this->storage_helper = $storage_helper;
$this->root_path = $root_path;
}
Expand Down Expand Up @@ -82,36 +88,39 @@ public function get_icons(string $use_path = ''): array
public function resync_icons(): void
{
$path = $this->get_storage_path() . '/';
$full_base_path = $this->root_path . $path;

// Get both arrays at once and pre-process paths
$untracked_files = array_map(static function($file) use ($path) {
return str_replace($path, '', $file);
}, $this->get_images());
// Create a single reusable callback function
$remove_path = static fn($file) => str_replace($path, '', $file);

$tracked_files = array_map(static function($file) use ($path) {
return str_replace($path, '', $file);
}, $this->get_stored_images());
// Get and process both arrays using the same callback
$untracked_files = array_map($remove_path, $this->get_images());
$tracked_files = array_map($remove_path, $this->get_stored_images());

// Process tracking changes
$files_to_track = array_diff($untracked_files, $tracked_files);
$files_to_untrack = array_diff($tracked_files, $untracked_files);

// Batch process tracking operations
foreach ($files_to_track as $file)
// Prepare batch tracking array with array_map instead of foreach
$files = !empty($files_to_track) ? array_map(
static fn($file) => [
'file_path' => $file,
'filesize' => filesize($full_base_path . $file)
Copy link
Copy Markdown

@rubencm rubencm Jan 19, 2025

Choose a reason for hiding this comment

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

This won't work if the adapter used is not local

Copy link
Copy Markdown
Contributor Author

@iMattPro iMattPro Jan 19, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Local is the only option I have. But also this is installed as local in migrations and the files being handled here must be treated as normal file system files. This is just a way for admins to add/remove favicons without needing to use FTP.

Copy link
Copy Markdown
Contributor Author

@iMattPro iMattPro Jan 19, 2025

Choose a reason for hiding this comment

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

Or I guess I can do this in this function:

$provider = $this->storage_helper->get_current_provider($this->storage->get_name());
if (str_ends_with($provider, 'local') === false)
{
    throw new runtime_exception('Web app icons storage must be local in storage settings.');
}

Copy link
Copy Markdown
Contributor Author

@iMattPro iMattPro Jan 19, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

I don't really get what the other adapters do, but I'll just explain what's going on here and why I think in this use case, I just have to enforce local adapter usage, any others won't be permitted for this storage type.

This method is addressing a problem that will occur if an admin decides to add or remove images (that are their site's favicons/touch icons) via FTP. If they do that, what's on disk won't match what's in the storage table. So the two need to be "re-synchronized"

These images must be kept on disk, aka local, as the browsers expect to be directly accessible from a physical location, for example:

<link rel="apple-touch-icon" href="./phpBB/images/site_icons/touch-icon.png">

I assume the other adapters can do something like, encode an image or file into binary data and store that in a DB instead of on disk. The image files that this extension is letting admin's manage via the ACP shouldn't be stored in those ways though.

],
$files_to_track
) : [];

if ($files)
{
try
{
$this->storage->track_file($file);
}
catch (storage_exception)
{
// If file doesn't exist, don't track it
}
$this->file_tracker->track_files(file_tracker::STORAGE_NAME, $files);
}

foreach ($files_to_untrack as $file)
if ($files_to_untrack)
{
$this->storage->untrack_file($file);
foreach ($files_to_untrack as $file)
{
$this->file_tracker->untrack_file(file_tracker::STORAGE_NAME, $file);
}
}
}

Expand Down Expand Up @@ -167,7 +176,7 @@ public function delete_icon(string $path): string
protected function get_stored_images(): array
{
$path = $this->get_storage_path();
$images = $this->storage->get_tracked_files();
$images = $this->file_tracker->get_tracked_files();

$result = [];
foreach ($images as $image)
Expand Down
41 changes: 9 additions & 32 deletions migrations/m3_storage.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,14 +10,10 @@

namespace phpbb\pwakit\migrations;

use phpbb\cache\driver\driver_interface;
use phpbb\db\migration\container_aware_migration;
use phpbb\extension\manager;
use phpbb\pwakit\ext;
use phpbb\pwakit\storage\storage;
use phpbb\storage\adapter\adapter_interface;
use phpbb\storage\adapter_factory;
use phpbb\storage\exception\storage_exception;
use phpbb\storage\file_tracker;
use phpbb\storage\provider\local;

class m3_storage extends container_aware_migration
Expand Down Expand Up @@ -61,19 +57,8 @@ public function add_tracked_files(): void
/** @var manager $extension_manager */
$extension_manager = $this->container->get('ext.manager');

/** @var driver_interface $cache */
$cache = $this->container->get('cache.driver');

/** @var adapter_interface|adapter_factory $factory */
$factory = $this->container->get('storage.adapter.factory');

$storage = new storage(
$this->db,
$cache,
$factory,
'phpbb_pwakit',
$this->tables['storage']
);
/** @var file_tracker $file_tracker */
$file_tracker = $this->container->get('storage.file_tracker');

$storage_path = ext::PWA_ICON_DIR . '/';

Expand All @@ -87,22 +72,14 @@ public function add_tracked_files(): void
// Extract just the file paths relative to the storage dir
$files = array_map(static function($image) use ($storage_path) {
$pos = strpos($image, $storage_path);
return $pos !== false ? substr($image, $pos + strlen($storage_path)) : $image;
return [
'file_path' => $pos !== false ? substr($image, $pos + strlen($storage_path)) : $image,
'filesize' => filesize($image)
];
}, $files);

// Track each file
foreach ($files as $file)
{
try
{
$storage->track_file($file);
}
catch (storage_exception)
{
// If file doesn't exist or other error, continue with next file
continue;
}
}
// Track files
$file_tracker->track_files('phpbb_pwakit', $files);
}

/**
Expand Down
6 changes: 4 additions & 2 deletions storage/storage.php → storage/file_tracker.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,8 +10,10 @@

namespace phpbb\pwakit\storage;

class storage extends \phpbb\storage\storage
class file_tracker extends \phpbb\storage\file_tracker
{
public const STORAGE_NAME = 'phpbb_pwakit';

/**
* Gets tracked files in the storage table
*
Expand All @@ -20,7 +22,7 @@ class storage extends \phpbb\storage\storage
public function get_tracked_files(): array
{
$sql = 'SELECT file_path FROM ' . $this->storage_table . "
WHERE storage = '" . $this->db->sql_escape($this->get_name()) . "'
WHERE storage = '" . self::STORAGE_NAME . "'
ORDER BY file_path";
$result = $this->db->sql_query($sql);
$files = $this->db->sql_fetchrowset($result);
Expand Down
7 changes: 3 additions & 4 deletions tests/unit/acp_module_test.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -77,9 +77,8 @@ public function test_module_info()
public function module_auth_test_data(): array
{
return [
// module_auth, expected result
['ext_foo/bar', false],
['ext_phpbb/pwakit', true],
'invalid auth' => ['ext_foo/bar', false],
'valid auth' => ['ext_phpbb/pwakit', true],
];
}

Expand All @@ -94,7 +93,7 @@ public function test_module_auth($module_auth, $expected)
public function main_module_test_data(): array
{
return [
['settings'],
'valid mode' => ['settings'],
];
}

Expand Down
48 changes: 30 additions & 18 deletions tests/unit/admin_controller_test.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -115,8 +115,8 @@ protected function setUp(): void
public function module_access_test_data(): array
{
return [
['settings', true],
['foobar', false],
'correct mode' => ['settings', true],
'incorrect mode' => ['foobar', false],
];
}

Expand All @@ -138,9 +138,9 @@ public function test_module_access($mode, $expected)
public function form_checks_data(): array
{
return [
['submit'],
['upload'],
['resync'],
'submit test' => ['submit'],
'upload test' => ['upload'],
'resync test' => ['resync'],
];
}

Expand All @@ -162,7 +162,7 @@ public function test_form_checks($action)
public function display_settings_test_data(): array
{
return [
[
'site name and short name' => [
[
'sitename' => 'phpBB',
'sitename_short' => 'phpBB',
Expand All @@ -172,7 +172,7 @@ public function display_settings_test_data(): array
'sitename_short' => 'phpBB',
],
],
[
'site name only' => [
[
'sitename' => 'phpBB',
'sitename_short' => '',
Expand All @@ -182,7 +182,7 @@ public function display_settings_test_data(): array
'sitename_short' => 'phpBB',
],
],
[
'long site name only' => [
[
'sitename' => 'phpBB Long Site Name',
'sitename_short' => '',
Expand All @@ -192,7 +192,7 @@ public function display_settings_test_data(): array
'sitename_short' => 'phpBB Long S',
],
],
[
'long mb site name only' => [
[
'sitename' => utf8_encode_ucr('phpBB™ 😂😂😂😂😂😂😂😂'),
'sitename_short' => '',
Expand Down Expand Up @@ -263,7 +263,7 @@ public function test_display_settings($configs, $expected)
public function submit_test_data(): array
{
return [
[ // all good inputs
'all good inputs' => [
[
'pwa_bg_color_1' => '#000000',
'pwa_theme_color_1' => '#ffffff',
Expand All @@ -276,7 +276,7 @@ public function submit_test_data(): array
],
'CONFIG_UPDATED',
],
[ // one style with good inputs
'one style with good inputs #1' => [
[
'pwa_bg_color_1' => '#000000',
'pwa_theme_color_1' => '#ffffff',
Expand All @@ -289,7 +289,7 @@ public function submit_test_data(): array
],
'CONFIG_UPDATED',
],
[ // one style with good inputs
'one style with good inputs #2' => [
[
'pwa_bg_color_1' => '#000000',
'pwa_theme_color_1' => '',
Expand All @@ -302,7 +302,7 @@ public function submit_test_data(): array
],
'CONFIG_UPDATED',
],
[ // one bad input
'one bad input' => [
[
'pwa_bg_color_1' => '#000000',
'pwa_theme_color_1' => 'fff',
Expand All @@ -315,7 +315,7 @@ public function submit_test_data(): array
],
'ACP_PWA_INVALID_COLOR',
],
[ // all bad inputs
'all bad inputs' => [
[
'pwa_bg_color_1' => 'foo',
'pwa_theme_color_1' => 'bar',
Expand All @@ -328,7 +328,7 @@ public function submit_test_data(): array
],
'ACP_PWA_INVALID_COLOR<br>ACP_PWA_INVALID_COLOR<br>ACP_PWA_INVALID_COLOR<br>ACP_PWA_INVALID_COLOR',
],
[ // all empty inputs
'all empty inputs' => [
[
'pwa_bg_color_1' => '',
'pwa_theme_color_1' => '',
Expand Down Expand Up @@ -446,9 +446,21 @@ public function test_resync()
public function delete_test_data(): array
{
return [
['foo.png', false, false], // not confirmed yet
['foo.png', true, false], // confirmed and valid data, no errors
['', true, true], // confirmed with invalid data, errors
'not confirmed' => [
'foo.png',
false,
false
],
'confirmed valid data' => [
'foo.png',
true,
false
],
'confirmed invalid data' => [
'',
true,
true
],
];
}

Expand Down
Loading