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

Enh: Introduce state and other fields to file table and implement interfaces #6523

Open
wants to merge 8 commits into
base: statable
Choose a base branch
from

Conversation

@martin-rueegg
Copy link
Contributor Author

martin-rueegg commented Aug 17, 2023

@martin-rueegg
Copy link
Contributor Author

martin-rueegg commented Aug 25, 2023

@luke- Question: for version 1.15, would you consider introducing a breakdown-version of this Interface and StateService just for the files table, as discussed in the forum?

In concrete, this would mean:

  • Introducing the following fields:
    • sort_order
    • category
    • metadata
    • state
  • Introducing the StatableInterface and related interfaces
  • StateService related interfaceses following the implementation proposed in this PR but
    • without the FindInstance implementation
    • caching
    • any change to any other table?

Because that is what I actually only urgently need.

@luke-
Copy link
Contributor

luke- commented Aug 26, 2023

@martin-rueegg

@luke- Question: for version 15, would you consider introducing a breakdown-version of this Interface and StateService just for the files table, as discussed in the forum?

With stable "v1.15" due out soon, I would really want to avoid (larger) changes & features.

Basically I am open for new fields like category, sort_order or metadata.

However, it would be important to me that introduced code (APIs/Attributes) are actively used in the core (or official modules).

e.g. (just quick thoughts)

  • category - could replace the field show_in_stream with a new default category FILE_LIST_STREAM.
  • sort_order - could provide possibility to re-order attached files to a content like Post.
  • metadata - could store custom information e.g. onlyoffice_key or file details. Maybe a generic approach would be useful. We need to also consider file_history here.

Idk if I want quickly introduce these fields that probably won't be used in Core/Modules for a while.
But if urgently required, you can prepare a PR to introduce these fields in 1.15. But expect changes here later.


state with StateService+Interfaces is more complex and currently I am still thinking about use cases (e.g. IS_PROCESSING here. I see that if wanted in 1.16 or later.

@martin-rueegg
Copy link
Contributor Author

@luke- thanks for your comments!

Basically I am open for new fields like category, sort_order or metadata.

However, it would be important to me that introduced code (APIs/Attributes) are actively used in the core (or official modules).

Yes, of course.


e.g. (just quick thoughts)

  • category - could replace the field show_in_stream with a new default category FILE_LIST_STREAM.

Make profile image handling generic and re-usable for any content #6408 would be using the category field for profile images and banners. The above PR would also allow to have multiple profile images. So a further PR could provide the code to actually ...

  • manage multiple images (add, update, sort, delete)
  • show a gallery when clicking on the image (where the image is not used as a link to the user/space)

What would FILE_LIST_STREAM stand for?


  • sort_order - could provide possibility to re-order attached files to a content like Post.

Yes, that's what we use it for. This code could be provided once we've implemented the sorting. Could also be used to order profile images (see above).

On this note: when you previously agreed that "the file module provide a common core API for standard file use cases like title and even sorting," did you mean just the API (code) or also it's active use in the core?

  • metadata - could store custom information e.g. onlyoffice_key or file details. Maybe a generic approach would be useful. We need to also consider file_history here.

Exactly. PR #6408 stores the following information (although as serialized data, not JSON):

{
	"data": {
		"file._upload.mimetype": "image/jpeg",
		"file._upload.hash": "2ef4bfec23ee811fa402de7a7c4d6c48a701439d",
		"file._upload.size": 19577,
		"file._original.mimetype": "image/jpeg",
		"file._original.hash": "ef25b44c8cd3fd88823c5e711fb844195eccb05b",
		"file._original.size": 244106,
	},
	"config": {
		"default": null,
	}
}

Idk if I want quickly introduce these fields that probably won't be used in Core/Modules for a while. But if urgently required, you can prepare a PR to introduce these fields in 1.15. But expect changes here later.

As mentioned above, the code to use those fields is already waiting in PR #6408. It's just that there are some intermediate steps that need to be approved, as we are already in progress of doing.

state with StateService+Interfaces is more complex and currently I am still thinking about use cases (e.g. IS_PROCESSING here. I see that if wanted in 1.16 or later.

When you say you're thinking about StateService+Interfaces, do you refer to the general implementation as proposed in PR #6430, or more specific in the sense of applying it to the files table?

#6408 uses the state field to implement soft deletes as well as a state draft, which is used during upload, while adjusting the image, but before saving the edit of the post.

(The mentioned PR also allows then to upload a new draft, which will overwrite the main image upon save. Until then, the old original is used. But while this can be done with the file variant, the previous example specifically requires the draft state.)

It will - at least in our module - also be used to report, review, ban an image. This could also be used in the core. Also, a user might to make a file temporarily invisible.

@luke-
Copy link
Contributor

luke- commented Sep 14, 2023

@luke- thanks for your comments!

Basically I am open for new fields like category, sort_order or metadata.
However, it would be important to me that introduced code (APIs/Attributes) are actively used in the core (or official modules).

Yes, of course.

e.g. (just quick thoughts)

  • category - could replace the field show_in_stream with a new default category FILE_LIST_STREAM.

Make profile image handling generic and re-usable for any content #6408 would be using the category field for profile images and banners. The above PR would also allow to have multiple profile images. So a further PR could provide the code to actually ...

Definitely interesting. Haven't had time to take a closer look yet.

  • manage multiple images (add, update, sort, delete)

Do you mean images added to a post?

  • show a gallery when clicking on the image (where the image is not used as a link to the user/space)

So one user should have multiple profile pictures?
I would rather prefer to revise the Gallery module. Maybe it could be integrated at this point.

What would FILE_LIST_STREAM stand for?

Just an idea to remove this flag. (Which indiciates a file is shown in the file list or not)
https://github.com/humhub/humhub/blob/master/protected/humhub/modules/file/migrations/m170211_105743_show_in_stream.php#L9

  • sort_order - could provide possibility to re-order attached files to a content like Post.

Yes, that's what we use it for. This code could be provided once we've implemented the sorting. Could also be used to order profile images (see above).

Please also see above.

On this note: when you previously agreed that "the file module provide a common core API for standard file use cases like title and even sorting," did you mean just the API (code) or also it's active use in the core?

I've we simply add this column it would be fine for me.

  • metadata - could store custom information e.g. onlyoffice_key or file details. Maybe a generic approach would be useful. We need to also consider file_history here.

Exactly. PR #6408 stores the following information (although as serialized data, not JSON):

I would be interested in possibly using a MySQL JSON column for this. do you have any experience with this?

Idk if I want quickly introduce these fields that probably won't be used in Core/Modules for a while. But if urgently required, you can prepare a PR to introduce these fields in 1.15. But expect changes here later.

As mentioned above, the code to use those fields is already waiting in PR #6408. It's just that there are some intermediate steps that need to be approved, as we are already in progress of doing.

state with StateService+Interfaces is more complex and currently I am still thinking about use cases (e.g. IS_PROCESSING here. I see that if wanted in 1.16 or later.

When you say you're thinking about StateService+Interfaces, do you refer to the general implementation as proposed in PR #6430, or more specific in the sense of applying it to the files table?

Both, I'll have to study StateService PR and #6408 more closely for this. Unfortunately, I currently lack the time here.

@martin-rueegg
Copy link
Contributor Author

@luke- Thanks for your comments!

I think we don't need to over-complicate things. Maybe on a general note I'd like to mention that sometimes I receive contradictory requirements for a change to be accepted:

  • On one side, every change to the core must be used in the core.
  • On the other side, changes should not be too complex at once.

To illustrate this, I'd like ta add my thoughts hen I read your comment above:


With stable "v1.15" due out soon, I would really want to avoid (larger) changes & features.

-> ok, that means, small changes to me.

Basically I am open for new fields like category, sort_order or metadata.

-> Seem to be straight-forward...

However, it would be important to me that introduced code (APIs/Attributes) are actively used in the core (or official modules).

-> uhm, ok, so not just the fields, but also API and also used in the core (or official modules). So, not so small then?

Idk if I want quickly introduce these fields that probably won't be used in Core/Modules for a while.

-> So do you now want to introduce these fields or not? On the forum it gave me the impression that you would want to provide "a common core API for standard file use cases like title and even sorting." Hence the API should be in the core, whether or not it is used in the core ...

But if urgently required, you can prepare a PR to introduce these fields in 1.15. But expect changes here later.

-> ... well, I just did what you asked me to do: "So you’re welcome to create PR for this." (same post)


So what I've tried to do in my reply to your comment was to indicate that indeed there are use-cases for all the fields I've mentioned and implementation for the core is already underway/waiting for review ...

I appreciate that my change requests are complex and far- and deep-reaching. I do my best to present them in a manageable way. Even if sometimes it might not look that way! 🤣

Now, as far as the new fields for file are concerned, I think we need to eventually have four layers:

  1. the fields in the database with their appropriate data types
  2. the $model-based API to manage those fields, including
  • definition of field content, e.g. status codes, category values, metadata-structure
  • methods to edit the fields (validation, serialization, etc)
  • auxiliary methods to manage a set of records, eg. sorting functions like e.g. $file->sortPosition(int $steps), $file->sortFirst() etc.
  1. front-end elements (e.g. widgets) to
  • display on the wall a set of images/documents attached to a ContentActiveRecord
  • manage a set of documents/images (add, edit, delete, re-order)
  1. actual usage of the above in production code

My understanding is, that right now we're only talking about Step 1 and maybe partially Step 2.

Maybe we should further discuss scope and details in

  • metadata - could store custom information e.g. onlyoffice_key or file details. Maybe a generic approach would be useful. We need to also consider file_history here.

Exactly. PR #6408 stores the following information (although as serialized data, not JSON):

I would be interested in possibly using a MySQL JSON column for this. do you have any experience with this?

Yes, I do have experience with MySQL JSON column, although not used through the Yii DB API.

Main points to consider:

  • Portability with other DBMS: do you only want to support MySQL and compatible? Does PDO have a JSON type?
  • PHP serialization is by far faster, specifically when de-serializing. It also allows to store objects. And objects can implement the Serializable interface or the __serialize()/__sleep()/__wakeup() magic functions
  • The current Metadata class is array-accessible, flexible, but still an object that does not copy when passed to and from functions. It is also quite important that it is one instance of the data, whoever adds or changes data in it. Not that in the end some data gets lost on the way.

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.

None yet

2 participants