Skip to content

Conversation

@oleibman
Copy link

@oleibman oleibman commented Dec 4, 2025

Use tmpfile for cache files. No need to specify directory.

Use PhpSpreadsheet to format benchmark output.

Use `tmpfile` for cache files. No need to specify directory.

Use PhpSpreadsheet to format benchmark output.
@oleibman
Copy link
Author

oleibman commented Dec 4, 2025

You mentioned this project in a discussion in PhpSpreadsheet, and you and I discussed it briefly there. Time to move the discussion here.

My major problem with what you are doing is that it doesn't work on Windows. The unlink and rmdir calls were often failing, and mkdir is poorly implemented on Windows. A side effect of investigating these problems was that I think that none of these functions are truly required. I think that using Php's tmpfile is just as effective, and files created that way clean up after themselves - they go away when closed or when the script ends. It also has the additional benefit of greatly simplifying your code, because no cleanup code is required.

With this change, the constructor for SpreadsheetSmartCache no longer needs a $localDir parameter, nor $autoClear. I have chosen for now to keep both, although neither will be used for anything, for compatibility purposes. It might make more sense to just delete them, even though that would be a compatibility break.

I admit that I do not know much about caching, and have, in consequence, not been able to come up with usable documentation for PhpSpreadsheet users on how it might be used. Setting up Redis or Apcu for myself is just too difficult. Yours comes without their additional setup needs, so it is attractive to me for documentation purposes if we can get it working in Windows. I hope I didn't break anything; I rely on your unit tests and benchmark script to tell me whether it works, and both run successfully for me.

We don't need to report microseconds - seconds is sufficient.

Right-align numbers when that feature is available in PhpSpreadsheet [TextGrid](PHPOffice/PhpSpreadsheet#4735).
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.

1 participant