Skip to content

Require using local storage provider#33

Draft
iMattPro wants to merge 5 commits intophpbb-extensions:mainfrom
iMattPro:local-storage-stuff
Draft

Require using local storage provider#33
iMattPro wants to merge 5 commits intophpbb-extensions:mainfrom
iMattPro:local-storage-stuff

Conversation

@iMattPro
Copy link
Contributor

@iMattPro iMattPro commented Jan 19, 2025

@rubencm Trying this approach here instead. Only allow icon management if the storage provider for Web App Icons is set to Local.

So with these settings:
Screenshot 2025-01-19 at 2 58 17 PM

You can do:
Screenshot 2025-01-19 at 2 57 11 PM

Otherwise, we say no and why not:
Screenshot 2025-01-19 at 2 57 32 PM

@iMattPro iMattPro force-pushed the local-storage-stuff branch from e9a8594 to 3d43791 Compare January 19, 2025 22:54
@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 (2e9540c) to head (62b0f2b).

Additional details and impacted files
@@             Coverage Diff             @@
##                main       #33   +/-   ##
===========================================
  Coverage     100.00%   100.00%           
- Complexity        84        87    +3     
===========================================
  Files              8         8           
  Lines            305       314    +9     
===========================================
+ Hits             305       314    +9     

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

@iMattPro iMattPro force-pushed the local-storage-stuff branch from 3d43791 to e3e4255 Compare January 19, 2025 23:18
@iMattPro iMattPro requested a review from rxu January 20, 2025 15:09
@rubencm
Copy link

rubencm commented Jan 20, 2025

👍

I think it should be only for resync, the upload is ok

@iMattPro
Copy link
Contributor Author

iMattPro commented Jan 20, 2025

👍

I think it should be only for resync, the upload is ok

I'm not sure if I want these files being served from anything other than local disk. I tried to use your DB adapter extension. It was broken, so I fixed the issues but still can't use it. Something about the system still wants to validate paths even though the DB doesn't have a path field. Otherwise I would have tested if files from another provider would even work for my purposes here. Because if they could work, then yeah this would only need to apply to the resync method.

@rubencm
Copy link

rubencm commented Jan 20, 2025

If you want to use files only in local, then maybe should not use storage, and the filespec classes should not be deprecated, not sure
But i think you can and should use it better

@iMattPro
Copy link
Contributor Author

I've put too much work and time into switching away from the deprecated class to go back. 🤭

@rubencm
Copy link

rubencm commented Jan 20, 2025

You can use storage, but for loading the icons you will need to use the storage controller

@iMattPro
Copy link
Contributor Author

iMattPro commented Jan 20, 2025

I will implement the storage controller after the core implements stateless routes as seen here phpbb/phpbb@master...marc1706:phpbb:feature/stateless_sessions

Also the storage system will need to implement stateless routing as well by changing, for example:

phpbb_storage_avatar:
    path:             /avatar/{file}
    defaults:
        _controller:  storage.controller.avatar:handle

to

phpbb_storage_avatar:
    path:             /avatar/{file}
    defaults:
        _controller:  storage.controller.avatar:handle
    stateless: true

@iMattPro iMattPro force-pushed the local-storage-stuff branch 2 times, most recently from 16da4a9 to 0cf4638 Compare January 21, 2025 01:27
@iMattPro iMattPro force-pushed the local-storage-stuff branch from 0cf4638 to eb068eb Compare January 21, 2025 01:32
@rxu
Copy link

rxu commented Jan 21, 2025

There should be a way to restrict adapter options to only local one and to prevent changing adapter for the pwakit.storage in the backend. Core events could be added if needed.

@iMattPro
Copy link
Contributor Author

iMattPro commented Jan 21, 2025

There should be a way to restrict adapter options to only local one and to prevent changing adapter for the pwakit.storage in the backend. Core events could be added if needed.

I was going to say the same thing. There should be a method in the main storage class perhaps, to have something like allowed providers? So we can do stuff like:

set_allowed_providers(['local']);

@rubencm
Copy link

rubencm commented Jan 21, 2025

I was thinking in something like this if is possible to disable configure it from acp

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

@iMattPro
Copy link
Contributor Author

iMattPro commented Jan 21, 2025

That would be cool too, but only if it prevents editing of the adapter/provider thingy. being able to change the path is still very useful to let admins do.

@rubencm
Copy link

rubencm commented Jan 21, 2025

That would be cool too, but only if it prevents editing of the adapter/provider thingy. being able to change the path is still very useful to let admins do.

One option to allow to update the adapter/provider and other to update the options or something like that

@iMattPro
Copy link
Contributor Author

iMattPro commented Jan 21, 2025 via email

@iMattPro
Copy link
Contributor Author

iMattPro commented Feb 6, 2025

I was thinking in something like this if is possible to disable configure it from acp

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

An option might be to use method calls:

    phpbb.pwakit.storage:
        class: phpbb\pwakit\storage\storage
        arguments:
            - '@dbal.conn'
            - '@cache.driver'
            - '@storage.adapter.factory'
            - 'phpbb_pwakit'
            - '%tables.storage%'
        tags:
            - { name: storage }
        calls:
            - [allowed_providers, ['@storage.provider.local']]
            - [set_options, []]

But then of course in the storage classes you'd need allowed_providers() and set_options() methods.

@iMattPro iMattPro marked this pull request as draft February 7, 2025 00:45
# Conflicts:
#	adm/style/acp_pwakit.html
#	helper/helper.php
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.

4 participants