search

picqer / moneybird-php-client

PHP Client for Moneybird V2
MIT License
82 stars 77 forks source link

Bump minimum required version #106

Closed holtkamp closed 7 years ago

holtkamp commented 7 years ago

Currently the minimum required version is 5.5.0, would it be an idea to bump this to either:

Note that some PHP 7.0 code is already used, for example the short array notation [] at: https://github.com/picqer/moneybird-php-client/blob/9b8761ba963e17f3217c640a921d75cede3de525/tests/ConnectionTest.php#L18

stephangroen commented 7 years ago

Short array syntax was added in 5.4.0. I'll investigate to bump the required version to 5.6.0, but 7.1.0 is probably a bit new for most people (unfortunately). We're running PHP 7.1 ourselves, but the library should be easy to use for most developers, also in shared hosting environments or older projects.

holtkamp commented 7 years ago

Short array syntax was added in 5.4.0.

aah, yeah? ok, sorry 😄

holtkamp commented 7 years ago

but 7.1.0 is probably a bit new for most people (unfortunately)

Yeah, I agree with the potential risks for other users. But consider the following arguments:

When people have properly configured their PHP version in their composer.json, it should not be a problem at al, since a new constraint for the PHP version of this library will not match their environment.

And suppose a new version is tagged as a new minor version0.15:

To also prevent the last mentioned problem, it could be considered to release a new major version1.0.0, but this seems overkill to me. I would say: people should take the responsibility to properly configure their dependencies...

Just my $0.02 😄

PS, also see http://jubianchi.github.io/semver-check/

casperbakker commented 7 years ago

Bumping the version is only useful if it gives access to new features that make the library better. I don't think there are any additions in PHP 7 that this library needs.

Return types from 5.6 could be handy. But upgrading just to be more modern is not practical for a small service library as this one.

yamitenshi commented 7 years ago

I'd actually argue that strict typing, scalar type hinting and/or return type hinting contribute to code readability and error prevention, and as such would contribute to the quality of this library.

However, as it stands, I'd argue against bumping the PHP version up, for a simple reason: the dependencies in composer.json are a list of required versions for the functioning of this library, not a list of recommendations. I'd heartily recommend anyone to update their production servers to at least 7.0, but as it stands this library is perfectly compatible with PHP 5.5, so until PHP 7 's features are actually used, I'd argue against bumping the version up.

Just my two cents.

holtkamp commented 7 years ago

@yamitenshi nice feedback.

so until PHP 7 's features are actually used, I'd argue against bumping the version up.

that is the "problem" / "challenge", when working on the code my hands are itching to use all these nice features that enforces clean coding and readability. But since we can not use it yet, nobody will actually add it...

yamitenshi commented 7 years ago

Honestly, there's nothing stopping you, as I see it - just be sure to bump up the minimum version if you do, since it then becomes a dependency.

No reason it should ever break installs, even without a version constraint - Composer accounts for PHP versions, so if the current version is the last one compatible with 5.x, Composer will never install the next one on a server with 5.x.

Of course, that sort of decision might not be one to make without the okay from other maintainers.

holtkamp commented 7 years ago

yeah, agree, that was kind of my first post 😄 https://github.com/picqer/moneybird-php-client/issues/106#issue-262702107, I guess we agree 😉

casperbakker commented 7 years ago

I am closing this now. It is fine to have different opinions about this, but lets put our time into building the best products for users instead of discussing these kinds of meta things.