Skip to content

[ticket/17361] Improve storage#6753

Merged
marc1706 merged 14 commits intophpbb:masterfrom
rubencm:ticket/17361
Feb 22, 2025
Merged

[ticket/17361] Improve storage#6753
marc1706 merged 14 commits intophpbb:masterfrom
rubencm:ticket/17361

Conversation

@rubencm
Copy link
Copy Markdown
Member

@rubencm rubencm commented Nov 23, 2024 •
edited
Loading

PHPBB-17361

Checklist:

  • Correct branch: master for new features; 3.3.x for fixes
  • Tests pass
  • Code follows coding guidelines: master and 3.3.x
  • Commit follows commit message format

Tracker ticket:

https://tracker.phpbb.com/browse/PHPBB-17361

It can be tested with this test extensions:

todo:

  • s3 extension with hotlinks
  • check that files are migrated correctly when updating from 3.3.x
  • tests
  • language keys in storage/adapter
  • after write, fclose is not needed
  • make sure stream is always closed after read
  • event in controller for hotlink?
  • Use storage in phpbb/console/command/thumbnail /generate.php or in create_thumbnail?, and phpBB/phpbb/console/command/thumbnail/delete.php
  • Maybe, try somehow to return a BinaryFileResponse when storage is local to restore partial downloads if there is a decent way
  • Fix attachments controller
  • Check that validate_data and validate_path in acp_storage work as expected
  • custom storage extension
  • Untrack files on migrations revert
  • Track files should receive an array to support more than 1 file
  • Add method in storage to allow to call custom adapter methods
  • Configuration to disable editing Require using local storage provider phpbb-extensions/pwakit#33 (comment)

@rubencm rubencm force-pushed the ticket/17361 branch 11 times, most recently from e804d3d to 3c02781 Compare November 30, 2024 02:26
@rubencm rubencm changed the title [ticket/17361] Rewrite storage [ticket/17361] Improve storage Nov 30, 2024
@rubencm rubencm removed the WIP 🚧 label Nov 30, 2024
@rubencm rubencm mentioned this pull request Dec 23, 2024
Merged
4 tasks
marc1706
marc1706 requested changes Dec 24, 2024
View reviewed changes
@rubencm rubencm force-pushed the ticket/17361 branch 4 times, most recently from 12e8e02 to ad135f3 Compare December 24, 2024 21:16
@rubencm rubencm mentioned this pull request Dec 25, 2024
Open
@iMattPro
Copy link
Copy Markdown
Member

iMattPro commented Jan 2, 2025

@rubencm I noticed a little issue in filespec_storage - not this version but the currently merged version - where failed uploaded files are not being removed. For example, I took a text file but gave it an image extension, so it looks like a PNG, but it's not really a PNG. So I try to upload it and get an error message that it was not a real image.

However the logic in the move_file() method is such that, because there is an error message, we return out of the move_file method before setting file_moved to true, so in the end, the file never actually gets deleted. It stays on the filesystem and in the storage DB.

Seems like when we get to the remove() method, since file_moved is still false even though the file was in fact moved, all that runs is the unlink but that is being run using what appears to be a temporary PHP stream of the file and not the destination file itself.

So in the end, the failed uploaded file still ends up being uploaded and being tracked in the DB.

iMattPro
iMattPro reviewed Jan 3, 2025
View reviewed changes
marc1706
marc1706 reviewed Jan 4, 2025
View reviewed changes
@marc1706 marc1706 added this to the 4.0.0-a1 milestone Jan 4, 2025
@iMattPro iMattPro mentioned this pull request Jan 19, 2025
Merged
@marc1706 marc1706 merged commit 3584ef7 into phpbb:master Feb 22, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@marc1706 marc1706 marc1706 approved these changes

+1 more reviewer

@iMattPro iMattPro iMattPro left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

4.0 (Triton) 🔮

Projects

None yet

Milestone

4.0.0-a1

Development

Successfully merging this pull request may close these issues.

3 participants

@rubencm @iMattPro @marc1706