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

PSR-4 autoloading only #29

Open
wants to merge 7 commits into
base: master
Choose a base branch
from
Open

PSR-4 autoloading only #29

wants to merge 7 commits into from

Conversation

phpfui
Copy link

@phpfui phpfui commented Oct 1, 2021

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!

@rkoopmans
Copy link
Contributor

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.

@phpfui
Copy link
Author

phpfui commented Feb 20, 2023

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.

@rkoopmans
Copy link
Contributor

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.

@phpfui
Copy link
Author

phpfui commented Mar 2, 2023

I merged the latest release into this branch and fixed a few things.

  • Typos
  • PHP 8.2 depreciation in tests
  • Separated out functions for file loading for those who use the functions instead of the class.

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

@rkoopmans
Copy link
Contributor

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 :-)

@phpfui
Copy link
Author

phpfui commented Mar 6, 2023

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.

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