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

Always determine reader type based on mime type #181

Closed
wants to merge 3 commits into from

Conversation

willrowe
Copy link

@willrowe willrowe commented Jan 7, 2025

This PR does the following:

  1. Removes the deprecated createFromType method and corresponding type property from SimpleExcelReader.
  2. Replaces calls to createFromFile with the createFromFileByMimeType method exclusively.

The reason for replacing calls to createFromFile is to allow any file to be processed correctly despite the file name. This is especially important when the original file has been renamed and no longer has a file extension. When working with uploaded files, the file name is always renamed to a generated string without the original extension. Always using createFromFileByMimeType should simplify the reader type detection and make it more consistent, since it is not dependent on the file extension.

This is a breaking change, so it will need to be released in the next major version.

@freekmurze
Copy link
Member

It seems the tests are failing could you take a look at that?

When the tests are passing, I'll release this as a new major version. Feel free to drop support for older PHP versions and use newish PHP features when needed 👍

@willrowe
Copy link
Author

After looking into this further, I don't know if this is the right direction to go anymore.

There are some edge cases with detecting the type that would make this package act in an unexpected way. For example, the text/csv mime can only be detected on CSV files with at least a couple of columns and rows. Empty CSV files or ones with only the headers would not be able to work.

Removing the ability to manually set the type would also remove support for reading from php://input and writing to php://output.

I'm not sure what direction you want to go with this package, but it seems like it would actually make things more predictable if specifying the type when both reading and writing was required and auto-detection based on file name and mime type were deprecated and removed instead. That way the user can use whatever method they deem reliable to determine what type they are reading from and/or writing to.

@willrowe willrowe closed this Jan 13, 2025
@willrowe willrowe deleted the patch-1 branch January 13, 2025 16:15
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.

2 participants