endclothing / prometheus_client_php

Prometheus instrumentation library for PHP applications
https://prometheus.io/docs/concepts/metric_types/
Apache License 2.0
150 stars 77 forks source link

Please support older versions of PHP #6

Open bvisness opened 4 years ago

bvisness commented 4 years ago

My company is currently using the jimdo version of this SDK and found out today that the package has been archived and you have taken over maintenance. We still have projects on 7.1, but it looks like you recently changed this library to only support PHP 7.3 and up.

Since jimdo was the best-supported PHP SDK for Prometheus (and the one linked from the official site), and since jimdo supported PHP 5.6 and up, changing the minimum version like this is pretty frustrating and is preventing us from using this library.

Now that you are maintaining this library for more than just your employer, please consider making it your policy to support all versions of PHP that are still officially supported. As of now, this would mean supporting PHP 7.1 and up. Anything else will prevent otherwise law-abiding people from using this library.

NoelDavies commented 4 years ago

Hi @bvisness, newer versions of PHP are implemented from v1.0.0 of the package, and as-per semver, is marked as a breaking change due to method signatures.

PRs will still be accepted for security patches and small features for older versions of PHP (and those PRs will be released with <1.0.0 tags.)

Please ensure you're not affected by the breaking version change by appropriately using constraints in your dependencies https://getcomposer.org/doc/articles/versions.md#stability-constraints.

bvisness commented 4 years ago

Since PHP 5 is completely end-of-life, I think it makes sense to release a new major version requiring PHP 7. But locking the version to 7.3 and up is completely gratuitous. (And furthermore, will require another major version to be released, since you seem to already have released 1.0 with a dependency on 7.1.)

As a library author you are shooting yourself in the foot by requiring people to use the absolute latest version of PHP. I like type hints as much as the next guy, but the reality of the situation is that not everyone can update right away. Nor should they, if their version is still officially supported.

Are a couple fancy language features worth losing a large chunk of users? If so, I wouldn’t be surprised if my company ends up making a fork just to remove the type hints and make it compatible with 7.1 again.

NoelDavies commented 4 years ago

@bvisness I completely understand your issue, and we should have perhaps done a slow roll-out as older versions were no longer supported. Nothing stops users of the repository locking themselves to versions <1.0.0.

(Sidenote: the 7.1 release was accidental, and it should have actually been tagged 7.3.)

I'm more than happy to work on a pre 1.0.0 release with you to add some upgrades without forcing the use of PHP 7.3.

As the now official fork, we'd like to support all users where possible, whilst maintaining the ability to adding new features, reducing technical debt and keeping security at the front of our approach to development.

If you'd like to submit a PR for these changes, we can merge that and release that with a few other changes, as a release under 0.10.0.

piotrooo commented 4 years ago

@bvisness first question - who will be maintaining that old code? Who will be making backports to the old version for unsupported PHP version? When you contribute code to the 7.3 version there are BC changes, that can't be just merged, needs a manual intervention (e.g.: method signatures).

IMO if someone can't be upgraded to 7.3 they should use a <1.0.0 - as a @NoelDavies said.

Also, I think there should be created branch 1.0.x from which will be created a new tags for version 1.0.1 ... 1.0.n. (trunk-based development). How this should works? https://paulhammant.com/2013/04/05/what-is-trunk-based-development/ That's only an idea, so we can disscus about it.

bvisness commented 4 years ago

My suggestion was not to maintain patches for unsupported versions of PHP. My suggestion was for this library to support all supported versions of PHP instead of just the latest.

You say “there will be BC breaks when contributing to the 7.3 version”, but my point is that there should not be a “7.3 version”. There should be a 7.1+ version, until 7.1 is no longer supported, at which point you can move to a 7.2+ version.

You only get BC breaks if you use the absolute latest and greatest features, which in this case are not actually necessary for the actual function of the library.

Basically I think you have a tradeoff between two options:

As the pretty-much-official Prometheus library for PHP, I think the second choice is clearly the correct one.

NoelDavies commented 4 years ago

Due to PHP 7.1 being FULLY EOL in 11 days (Dec 1 2019) we will not be adding PHP 7.1 support. But I think we can add PHP 7.2 support, but this needs to be a community effort.

piotrooo commented 4 years ago

@NoelDavies agree with that. But firstly we must think how to organize code. Master should be 7.2 compatible? We need another branch. This is most important decision - IMHO.

bvisness commented 4 years ago

I think master should be 7.2 compatible. The differences between 7.2 and 7.3 should be minor enough that it will run fine on both (especially for a library that previously ran fine on 5.6), and maintaining two separate branches would be a ton of work.

NoelDavies commented 4 years ago

Agreed, We need a 7.2 addition to the CI pipeline, and go from there. Does anybody wanna start this? I'd be happy to contribute too.

bvisness commented 4 years ago

I created #35 to add PHP 7.2 to the CI config, and it seems to be "working".