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

Adds FileAdder@setFileSize() #3357

Merged
merged 3 commits into from
Sep 4, 2023
Merged

Conversation

cosmastech
Copy link
Contributor

This PR makes it possible to set the filesize via FileAdder. My thinking is that we may already have this information (such as from the headers of a request) and having to either call filesize() or Storage::size() can be avoided.

Also, I refactored it a bit so file size is only fetched one time.

@cosmastech
Copy link
Contributor Author

After diving deeper into this, I don't think we need the setFileSize method.

However, I do think the refactor to only determine the filesize once is beneficial, as it cuts down on a repeated call to S3.

@freekmurze freekmurze merged commit 0e97c0f into spatie:main Sep 4, 2023
@freekmurze
Copy link
Member

Thanks!

@francoism90
Copy link
Contributor

Why would you want to do this? What if the file size differs when the upload has been finished?

Doing the extra call, would help debugging these issues.

@rodrigopedra
Copy link

I am sorry if this looks like a rant by any means, I value all the hard work put into it, and into the other many Spatie packages, as open source for the community.

And am very grateful for all your hard work, and how it impacted on my family's life.

I just would like to point out, somehow this PR was missed on the changelog, and one of my apps had a subclass with a private $fileSize, so it started failing for one of my clients.

I know I shouldn't be subclassing, and should have more tests, this is a feature that is rarely used on a rather stable project.

Likewise, I could easily fix the issue upon a client's report, and no one was upset, but I thought on letting you know.

ping @freekmurze (just for awareness, thank you for all the hard work)

@rodrigopedra
Copy link

I actually wanted to mean release notes instead of the changelog.

Yet, everyone is happy, and no puppies were harmed during the fix 😃

thanks again 🙏

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