Skip to content

Update storage and file tracking#32

Merged
iMattPro merged 10 commits intophpbb-extensions:mainfrom
iMattPro:experiemental
Feb 22, 2025
Merged

Update storage and file tracking#32
iMattPro merged 10 commits intophpbb-extensions:mainfrom
iMattPro:experiemental

Conversation

@iMattPro
Copy link
Contributor

@iMattPro iMattPro commented Jan 18, 2025

This is for phpbb/phpbb#6753

@iMattPro iMattPro marked this pull request as draft January 18, 2025 22:56
@iMattPro iMattPro force-pushed the experiemental branch 3 times, most recently from 3f8ffae to 8bff3da Compare January 18, 2025 23:17
@iMattPro iMattPro force-pushed the experiemental branch 3 times, most recently from 0b74679 to df1da08 Compare January 19, 2025 00:42
@codecov-commenter
Copy link

codecov-commenter commented Jan 19, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 100.00%. Comparing base (097e235) to head (01552c8).

Additional details and impacted files
@@              Coverage Diff              @@
##               main       #32      +/-   ##
=============================================
+ Coverage     99.66%   100.00%   +0.33%     
- Complexity       83        84       +1     
=============================================
  Files             8         8              
  Lines           299       305       +6     
=============================================
+ Hits            298       305       +7     
+ Misses            1         0       -1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@iMattPro iMattPro force-pushed the experiemental branch 2 times, most recently from d7f84d2 to 9a43e36 Compare January 19, 2025 04:42
$files = !empty($files_to_track) ? array_map(
static fn($file) => [
'file_path' => $file,
'filesize' => filesize($full_base_path . $file)
Copy link

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

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
Contributor Author

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

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.

@iMattPro iMattPro marked this pull request as ready for review February 22, 2025 16:02
@iMattPro iMattPro merged commit 2e9540c into phpbb-extensions:main Feb 22, 2025
32 checks passed
@iMattPro iMattPro deleted the experiemental branch February 22, 2025 16:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants