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

Initial WIP on improving the File model to use more native disk methods #141

Draft
wants to merge 1 commit into
base: develop
Choose a base branch
from

Conversation

LukeTowers
Copy link
Member

WIP

@what-the-diff
Copy link

what-the-diff bot commented Feb 21, 2023

  • Added a constant DISK to the class.
  • Changed fromPost and fromFile methods return type hinting static instead of void, also added nullable types for parameters in both functions.
  • Removed unused setDataAttribute method as it is not used anywhere else in the codebase or called by any other classes/methods etc...
    5-6: Fixed typos within comments above each function where 'return' was spelt incorrectly as 'retunr'. This has since been fixed correctly now though :)

*/
public function getWidthAttribute()
public function getWidthAttribute(): string|int|null
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you consider changing the width and height attribute to be optional when using an s3 driver in some form. These 2 attributes make any remote storage take more than 7 seconds because it has to download the file locally.

Copy link
Contributor

Choose a reason for hiding this comment

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

actually nvm, I can just override it

Copy link
Member Author

Choose a reason for hiding this comment

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

We could store that data on upload theoretically and retrieve it from metadata instead of recalculating

Copy link
Contributor

Choose a reason for hiding this comment

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

hmm, if that's the case, wouldn't it also be beneficial to have a has_thumb meta parameter to indicate if the image has a thumbnail or not.

Currently the way I have it is that I have a has_thumb property on the file model. so rather than using hasFile which checks the remote file location it checks if that parameter is set, then I offload the thumbnail generation to a queue (this is more of a custom solution for me that im utilizing, but using hasFile slows down the process a lot.)

Copy link
Member Author

Choose a reason for hiding this comment

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

We should probably add a metadata column in the form of a JSON blob to store this sort of information

@LukeTowers LukeTowers added this to the 1.3.0 milestone Apr 25, 2024
Copy link

This pull request will be closed and archived in 3 days, as there has been no activity in the last 60 days.
If this is still being worked on, please respond and we will re-open this pull request.
If this pull request is critical to your business, consider joining the Premium Support Program where a Service Level Agreement is offered.

Copy link

This pull request will be closed and archived in 3 days, as there has been no activity in the last 60 days.
If this is still being worked on, please respond and we will re-open this pull request.
If this pull request is critical to your business, consider joining the Premium Support Program where a Service Level Agreement is offered.

Copy link

This pull request will be closed and archived in 3 days, as there has been no activity in the last 60 days.
If this is still being worked on, please respond and we will re-open this pull request.
If this pull request is critical to your business, consider joining the Premium Support Program where a Service Level Agreement is offered.

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.

2 participants