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

Pluggable storage backend #6180

Merged
merged 44 commits into from
Dec 14, 2018
Merged

Conversation

fabrixxm
Copy link
Collaborator

@fabrixxm fabrixxm commented Nov 21, 2018

This pull request want to replace in-database data storage with a system of storage backends, selectable by the administrator.
The plan is to keep support for old database storage system, and being able to retrieve data from different backends when admin change the default backend.
A console command will be provided to move data between backends.
The work is focusing on photo handling and storage, but backends are generic and can be used to store attachments data as well.

my plan at the moment is

  • port mod/photo.php to \Friendica\Module\Photo

  • update \Friendica\Model\Photo to do CRUD on photo table and handle data via backends

  • add admin configurable photo storage backend (db, filesystem)

  • add a way to write storage backends as addons

  • update all code related to photo to use Model\Photo

    • file which still calls DBA with photo table (found with git grep -n "DBA::.*(.photo" ):
      • include/api.php
      • src/Worker/CronJobs.php
      • src/Model/Contact.php
      • src/Model/Item.php
      • src/Model/Mail.php
  • add a new console command to move data data between storages

Works to attachments will be in another branch

Set correct http response header, display error message using "404.tpl"
- add backend related columns in photo table
- add system resource storage class
- add code to load image data from backend class
- return "nosign" image as photo meta with SystemResource backend
storage classes should implement this interface
@fabrixxm
Copy link
Collaborator Author

A note: Commit 58bd75f is not directly related to the feature, but it's something I wanted to add for a while now.
Calls to module functions in App::runFrontend() are wrapped in a try...catch which catch HTTPExceptions and return correct http status code and use "404.tpl" template to display error message to the user.
Code can now throw any HTTPException subclass and user will get a nice page (and a valid http error code) instead of a WSOD or a cryptic message about an unhanded exception.
A Logger::log() call could be added to log the exception traceback

@annando
Copy link
Collaborator

annando commented Nov 21, 2018

Concerning the code: Please write boolean values always lowcaps and please always leave a blank between comparator and value. And I guess @MrPetovan will prefer having only ' or " in the code as string delimiters.

Concerning the "404.tpl" I would prefer having a single functionality for throwing HTTP errors. Have a look at System::httpExit(...);. When we unify this, we can - somewhere in the future - throw really nice messages.

src/Model/Storage/Filesystem.php Outdated Show resolved Hide resolved
.gitignore Outdated Show resolved Hide resolved
src/Model/Storage/Filesystem.php Outdated Show resolved Hide resolved
src/Model/Storage/IStorage.php Outdated Show resolved Hide resolved
src/Module/Photo.php Outdated Show resolved Hide resolved
src/Model/Storage/Filesystem.php Show resolved Hide resolved
src/Model/Storage/SystemResource.php Outdated Show resolved Hide resolved
src/Model/Storage/SystemResource.php Outdated Show resolved Hide resolved
src/Module/Photo.php Show resolved Hide resolved
src/Module/Photo.php Outdated Show resolved Hide resolved
@fabrixxm
Copy link
Collaborator Author

fabrixxm commented Nov 21, 2018

I would prefer having a single functionality for throwing HTTP errors
You have it: throw new WhatEverHTTPException("message to the user")

I could use System::httpExit(...); in catch() block but it then stop and just return the content of http_status.tpl which is a blank page. My code leave the user with an error message in current theme page. with navbar and everything.

Another reason is that module code don't need to think "I should die here, or I should call System::httpExit() or System::jsonExit() ?" It just throw and error and let the system decide.
That catch() block could check for "Accept" header and render the error message accordingly.

Plus, if you are reusing code from a module in, let's say, API calls, and the module stop with System::httpExit(), the api call will return an html page. If the module throws an HTTPException, the api code (which is already using a catch() block for HTTPException) will render the error according to requested api format

But if it's not ok, I'll use existing functions.

edit: and I don't have to remember http codes :-P

@MrPetovan
Copy link
Collaborator

@fabrixxm We're treading in uncharted territory here. We've never had something as "nice" as we do now, so we do have to iron out some standard behavior.

I like the idea of having request-agnostic outputs for modules, but the fact is that API modules and Web modules have two very different approaches to building this output, so I'm not sure we can do something truly universal.

@annando
Copy link
Collaborator

annando commented Nov 21, 2018

When not using System::httpExit(...); I would suggest some other function that is then feed with the HTTP code. This could then direct to some general stuff in the future. I don't prefer hard coded template files for some of the HTTP codes.

Copy link
Collaborator

@MrPetovan MrPetovan left a comment

Choose a reason for hiding this comment

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

I'm adding only a single code standard comment for each violation type per file to avoid spam. Please make sure that there aren't any further violations in the whole file when you fix one.

src/Model/Storage/IStorage.php Outdated Show resolved Hide resolved
src/Model/Photo.php Outdated Show resolved Hide resolved
src/Model/Photo.php Outdated Show resolved Hide resolved
src/Model/Photo.php Outdated Show resolved Hide resolved
src/Model/Photo.php Outdated Show resolved Hide resolved
src/Model/Storage/SystemResource.php Outdated Show resolved Hide resolved
src/Module/Photo.php Outdated Show resolved Hide resolved
@MrPetovan
Copy link
Collaborator

It's a little late but have considered https://github.com/thephpleague/glide ?

@fabrixxm
Copy link
Collaborator Author

Now I should write some documentation ...

src/Core/StorageManager.php Outdated Show resolved Hide resolved
@fabrixxm
Copy link
Collaborator Author

I've wrote some docs.
Someone should proof read it, really...

doc/AddonStorageBackend.md Show resolved Hide resolved
doc/AddonStorageBackend.md Outdated Show resolved Hide resolved
@MrPetovan
Copy link
Collaborator

Are you done with your changes? What happens if admins don't have any storage backend addon enabled yet? What happens when they do?

@fabrixxm
Copy link
Collaborator Author

fabrixxm commented Dec 13, 2018

Are you done with your changes?

I hope so. I should have changed all the important places about photo management.

What happens if admins don't have any storage backend addon enabled yet?

They will see only core storage backends: "Filesystem" and "Database"

What happens when they do?

The new storage backend will be shown as a select option in admin page

src/Model/Photo.php Outdated Show resolved Hide resolved
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants