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

Update this library to a supported PHP version #32

Merged
merged 6 commits into from
Feb 21, 2022

Conversation

ricardoboss
Copy link
Collaborator

Great lib!

In this PR, I added a lot of type-hints and updated the minimum PHP version to 7.4.
I also removed all deprecated usages of old PHP versions and implemented some best-practices. Most changes are return type-hints, parameter type-hints and casts.

One heads up: the addPath method can now return null. In previous versions it would have thrown an exception, since it tried to call a method on null.

Let me know what you think! If these changes are too much at once, I am happy to re-submit finer-grained PRs.

@kartsims
Copy link
Owner

Man this is amazing, thank you so much for this PR

To be honest I haven't even looked at PHP code for a while, so I am not aware what the state of the art is, and which version should be supported or not.

In general I think it's quite convenient to have back support for older versions, and if not possible, maybe include two versions with a tag for "legacy support". Because you may know how people just can't upgrade the PHP engine sometimes.

Is there a way we can offer that? Or is PHP 7.4 a given for 99% of the projects that may use/need that lib?

The code linting is also a nice touch, I am guessing you are using standard lint settings? If so, could you specify them either by including a settings file such as an editorfile, or in the README?

Thanks again, I am looking forward to merge that :)

If you want to become a maintainer there is also a spot for you as you may have guessed I am quite out of the loop on the PHP front (I am more of a JS guy now)

@ricardoboss
Copy link
Collaborator Author

Hi!

Personally, I'd rather not support anything below PHP 7.1. PHP 7.4 was my choice for minimum version, since it introduced property type hints and if you're able to update to PHP 7.1, you might as well go to 7.4.

A quick look at the usage statistics shows that 20% of installs use anything less PHP 7.4. And that's okay. Here's why:
In this PR, I didn't add new functionality. Users can still use version 1.0 on older PHP version without any problems. The issues start to come up when using the lib in newer versions, since there have been a lot of deprecations.

I just wanted to fix those.

About the code linting: I use PhpStorm as an IDE to develop in PHP. It has great built-in tools and analyzers. For distributed linting, I'd recommend PHP-CS-Fixer and a .editorconfig. If you want me to, I can set it up and provide a config.

Maintaining a project can be a lot of work. However, I'd be happy to review pull requests and respond to issues from time to time.

@ricardoboss
Copy link
Collaborator Author

@kartsims
Copy link
Owner

Awesome thanks

Then I'll branch out to legacy-php to allow support for older versions

Yes well there isn't so much usage for this lib actually (otherwise I would have run away / given it away a long time ago)

Responding from time to time is basically what I do, so I'll add you as maintainer, it won't take you much time and it's always a nice touch on your github page should you ever look for a job ;) (not that we developers look for jobs too much...)

If you can include the editorconfig file that'd be neat yes please, just in case someone is using a different IDE that usually helps keep PRs nicely formatted

@JellyBellyDev
Copy link

Good Job @ricardoboss!
@kartsims bump to new major? :)

@ricardoboss
Copy link
Collaborator Author

@kartsims you can add me as contributor on GitHub and packagist. I would be happy to tag a new version.

@ricardoboss
Copy link
Collaborator Author

@kartsims in case you want to release a new version, please tag it as 2.4. I had to release a few patches of my fork, which is why there needs to be a version greater than 2.3 of this lib.

@kartsims
Copy link
Owner

Hi @ricardoboss thank you for the messages and sorry about the delay in my response, I'll include you as contributor

@kartsims
Copy link
Owner

Regarding Packagist, I signed in there (I had credentials) but no package on my profile. It seems that this person "owns" the package.

@ricardoboss
Copy link
Collaborator Author

Thanks @kartsims!

It may be a wild guess, but could @nvashenko be their username on GitHub as well?

@nvashenko
Copy link

hi @kartsims , @ricardoboss
I used this lib for one of my projects, don't know for what reason I created a package on packagist but if you guys want I can try to add both of you as maintainers because i don't know how to transfer ownership there

@ricardoboss
Copy link
Collaborator Author

Hi @nvashenko! Thanks for replying so fast. Your approach seems to be right (composer/packagist#961). My packagist username is also ricardoboss (https://packagist.org/users/ricardoboss/)

@nvashenko
Copy link

@ricardoboss added you.
@kartsims could you please post your packagist username so i can add you to maintainers

@ricardoboss
Copy link
Collaborator Author

@kartsims I recommend you set up auto-updating on packagist. That way I don't even need to be maintainer over there.

@kartsims
Copy link
Owner

Thanks @nvashenko my username is kartsims there as well

Regarding auto updating I am not sure how it works... is it something like that?

@ricardoboss
Copy link
Collaborator Author

I think you can follow this guide: https://packagist.org/about#how-to-update-packages

@kartsims
Copy link
Owner

OK it seems that it is now automagically auto updated...

Since you added me there, and I connected my Github account, it is now working.

Thanks guys 🙌

@ricardoboss I'll let you follow up on the PRs and issues, you should be able to do them now otherwise lmk

@ricardoboss ricardoboss merged commit c105955 into kartsims:master Feb 21, 2022
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