-
Notifications
You must be signed in to change notification settings - Fork 52
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
PSR-4 autoloading only #29
base: master
Are you sure you want to change the base?
Conversation
Hi, Thanks a lot for providing an improvement to our PHP client. I merged this change into a local branch, and all the tests fail due to auto loading issues. https://github.com/rkoopmans/tinify-php/actions/runs/4222383881 Feel free to provide an updated version that works with the test suite. |
LOL! Completely forgot about this PR. I moved the curl_mock.php after the autoloader so it could find the exception. I noticed you are using travis CL. Most PHP projects have switched to GitHub Actions. I would be happy to port it for you. Let me know. Also you seem to be supported very old versions of PHP. I would recommend only supporting 7.1 and higher. Any older PHP versions would still be able to use an older version of the library (it does not seem to change much). 7.1 support adds parameter types, which is very useful in helping people catch bugs. Let me know. Happy to help any way I can. |
Currently the respository is on github actions, but I guess you will need to rebase on the master branch, then the PR should be run according to our test workflow in .github/workflows/ci-cd.yaml. We migrated it about a year ago from Travis to GH. Back then we still had customers on php 5, so we want to keep supporting it until the number is close to zero. |
I merged the latest release into this branch and fixed a few things.
This makes the package fully PSR-4 compliant and should still work if people are using the functions. I would also remove the functions as this is not best PHP practice, but not sure of who would be using them or why. Functions don't autoload correctly if you are using a pure PSR-4 path based autoloader and require the overhead of the composer autoloader on each request. See https://blog.phpfui.com/benchmarking-php-autoloaders |
Thanks for the update! Looks great, and that blog post is an interesting read. We"ll have to check WHO is using these functions and what it will impact :-) |
No big deal to leave it as is, since it should work fine with the composer autoloader which most people use. If you want to use only PSR-4, you can just port to the static class function, which is best practice anyway, since global functions require the file to be loaded on every request. The beauty of PSR-4 autoloading is you only load what you need, not the entire universe, which slows everything down. |
Since it looks like you are maintaining this package, I thought I would submit a PR to upgrade to full PSR-4 autoloading.
There is no need for a "files" section, PSR-4 gets it done when you do one class per file. This PR simply breaks up the Exceptions into individual files and removes the "files" section from the autoloader.
This allows for super fast autoloaders and is considered best practice in PHP.
Thanks!