meyfa / php-svg

Vector graphics (SVG) library for PHP
https://packagist.org/packages/meyfa/php-svg
MIT License
509 stars 91 forks source link

Drop unmaintained PHP, publish a release #215

Closed tacman closed 7 months ago

tacman commented 8 months ago

Great library! It deserves to be more than a version 0 though.

Idea: publish a version 1 as a snapshot, with the current PHP requirement.

Then drop PHP 7 (which has be EOL for a while) and support only the maintained version of PHP . Since PHP 8.1 is security fixes only, and version 1 of this library would still support it, you could even bump the requirements to PHP 8.2.

That way, PRs like #212 could be easily accepted. In fact, we could run rector and make the code base easier to test, auto-complete, etc.

In short, supporting old versions of PHP complicates things, publish a version 1 for those that need it, and then bump the requirements to take advantage of all the goodness from more recent versions.

meyfa commented 8 months ago

Thanks, but I personally wouldn't consider PHP-SVG production-ready and don't want to give false impressions by making a 1.x release.

Can you elaborate on how dropping PHP 7 would affect #212? As far as I can see, that PR could be merged as-is. The only reason it hasn't is that I'm waiting for feedback from the author.

The intent was never to be cutting-edge, in fact, PHP-SVG was started out of necessity to work with PHP 5.3.3 and only once that became impossible to maintain, the choice was made to require PHP 7.3 or later.

I find it hard to believe that supporting PHP 7 prevents people from contributing, but please correct me if I'm wrong :) In general the more likely reason is that the SVG spec is quite involved and few people feel like working out all the weird edge cases. I spent hundreds of hours in the past working on the algorithms and it's barely enough to get something usable. Unfortunately I don't use PHP regularly any more, and nobody is sponsoring this work, so I haven't been able to keep investing as much time.

tacman commented 8 months ago

It probably don't affect #212 -- but I couldn't remember when that ::class syntax was introduced.

As far as being production-ready, how about simply saying "implements a common subset of the spec"?

While you're right that PHP 7 doesn't keep anyone from contributing, it does mean that any contributions have to run in older versions of PHP. I love typing-hinting and property promotion and all that, and (mostly) getting rid of php docblocks. But none of that is essential to the core.

Niellles commented 8 months ago

Kindly refer to https://github.com/meyfa/php-svg/issues/180 for previous discussion when <7.3 was dropped.

I think PHP version stats: January, 2024 is an interesting read. A staggering 13% still on PHP 7.4. That being said.. how many of those installs would be regularly updating their dependencies?

Personally I would like to drop <8.1, just because it's the right thing to do. It would also allow us to clean up the project a bit and use some of the goodies mentioned by @tacman. (I have some spare time now, happy to pitch in).

It shouldn't be too much of a problem for people using php-svg as, they'll only be missing out on new features.. and honestly I think their time would be better spend on updating their PHP version than writing new code that uses those new php-svg features.

Niellles commented 8 months ago

After giving this some more thought (apparently I was talking about it in my sleep last night) I think we should first fix #98.

For that implementation we probably have to introduce some breaking changes that would warrant a major version bump anyway. Additionally I think once that's done I'd consider this ready for a 1.x release. Users on 7.x would still be possible to profit from that feature.

I do also think that @tacman makes a good point, this project may not be perfect, but AFAIK it's the best thing out there for rendering SVG with vanilla PHP. Also it implements a broad enough subset of the spec to be production ready (even more so with additional rasterizers).

Niellles commented 8 months ago

Analysis

I discovered that you can browse package installs by PHP versions on packagist! Which provides us some data to analyse this dilemma further.

As you can see, when looking at all versions in february, we have

afbeelding

However, if we take a look at 0.13.x, which I think was very short lived:

As for 0.14.x:

afbeelding

Proposal

Let's drop PHP 7.3 going forward; the numbers of installs is negligible. As of 7.4 we have:

Roughly 5% of users on PHP 7.4 isn't that much either, but I think it's enough to warrant continued support. At least we should try to bring them additional rasterizers (e.g. Imagick) before we drop support.

Notes

Not super relevant to the discussion at hand, but I thought the following was interesting. Even though we barely see any recents installs from PHP 7.2 for the last three versions that supported it (i.e. 0.12, 0.11 and 0.10). And then, all of a sudden 0.9 gets about 60-70% of its installs from PHP 7.2: afbeelding

My assumption/educated guess is that these are comming from lasserafn/php-initial-avatar-generator (about 100,000 monthly installs, mostly on PHP 7.2) which uses meyfa/php-svg: ^0.9.0 as a dependency. afbeelding Normally I'd expect those to install the latest version of 0.x as ^ means anything before the next major. However, composer version caret version range works somewhat differently for pre-1.0 versions:

The ^ operator behaves very similarly, but it sticks closer to semantic versioning, and will always allow non-breaking updates. For example ^1.2.3 is equivalent to >=1.2.3 <2.0.0 as none of the releases until 2.0 should break backwards compatibility. For pre-1.0 versions it also acts with safety in mind and treats ^0.3 as >=0.3.0 <0.4.0 and ^0.0.3 as >=0.0.3 <0.0.4.

Also see daily installs per version: afbeelding

meyfa commented 8 months ago

Let's try to unwrap this. There are two requests here: drop support for old PHP versions, and make a v1 release. Let's discuss these individually (summary at the end).


Regarding the first request - I've thought about this a bit and also looked at the PHP changelogs. Some very good points were brought up by both @tacman and @Niellles. Especially thanks to @Niellles for pointing out the statistics page, which is fantastic, and for providing an analysis.

I agree with @Niellles that the syntax changes of 7.4 are a significant improvement and worth dropping 7.3 support for. PHP 8.0 would additionally give us (among other things):

PHP 8.1 would give us (among others):

I think PHP 8.2 and 8.3 have comparatively less to offer.

Overall, the improvements offered by PHP 8.0 and 8.1 seem almost equally as important as the ones in 7.4, however, breaking support for 5% of the userbase is a significant downside. Yes, those users are on EOL versions - but I don't agree with the stance that we have to align with PHP's support policy, necessarily.


Regarding the v1 release - In my opinion, the version number matters little in practice, so I don't understand why this keeps being raised. Obviously we should release a v1 at some point, but why is this important to do soon? As mentioned in #180, I feel like some aspects of PHP-SVG would need a redesign, still. Let me expand on what I mean:

Note that any breaking changes (such as dropping a PHP version) do not necessitate a major version bump while PHP-SVG is on v0.x. As pointed out above, v0.x has special versioning rules. This is well-supported by Composer.


In summary, if a PR was raised to require PHP 7.4 or later, I will accept it. It would be awesome if this was followed up with one or more PRs adding the type system syntax supported by PHP 7.4. (I might have a go at this myself, but it could be a while until I find enough free time to do so.)

Based on the usage data, I could be convinced to go directly to 8.0, however, only if someone actually wants to dedicate time to make use of its features and raise PRs for that. I don't think we should drop 8.0 yet given the circumstances.

Finally, the v1 release is blocked on the points mentioned above, and shouldn't happen yet IMO.

tacman commented 8 months ago

It would be awesome if this was followed up with one or more PRs adding the type system syntax supported by PHP 7.4.

Rector is awesome for this.

Although I feel like a bit of a snob saying this, but do people who won't upgrade from an EOL version of the language merit the extra work to provide them new features? This ties into the 1.0 question -- although imperfect, the version people are using now is being used in production.

So one idea is to snapshot the 1.0 release as is, with PHP 7.0 support and whatnot. Then the main (formerly master) branch could be an alias for 2.0, with PHP 8.1+. Keeping support for 7.4 for new features seems overly generous. If the 5% of the user base discovers that they're missing out on a new feature from this library, it should inspire them to upgrade.

Be careful though -- once you start using the new features of 8.1, you'll never want to go back to using PHP 7!

Niellles commented 8 months ago

Dropping PHP 7

I think dropping PHP 7.3, so that we can use some of the goodies 7.4 brings, is a no brainer given the previously shared analysis (i.e. usage on 7.3 negligible).

I really don't see a reason for dropping 7.4 (and 5% of users) on the other hand. Yes, both 8.0 as well as 8.1 offer some very nice features — that I do take for granted in my day to day work. However I do very much like the idea of supporting any PHP version for as long as possible. PHP 5 was only dropped when we absolutely had to. If I recall correctly the test suite could still be polyfilled for 8.0, but 8.1 was impossible (or at least very challenging). At this point there's no hard requirement to drop PHP 7, just some (really) nice to haves IMHO.

As an additional note: I highly doubt whether people that are a) knowledgeable on the SVG spec and b) willing to "donate" their free time to this project, are not doing so because they cannot use enums, readonly/promoted properties or union types.

1.0 release

I do not really have a strong opinion on this topic, other than a few notes.

While I understand the wishlist (and the potential breaking changes they'd bring) might cause some reluctancy to do a 1.0 release. I.e. I suppose it would have to be a LTS-release and then you cannot make breaking changes. Bringing out a new major twice a year sounds less than desirable.

The only downside I see when staying on 0.x is that there's no distinction between a backwards compatible feature release and a breaking change, which would usually be minor and major releases respectively.

tacman commented 8 months ago

I highly doubt whether people that are knowledgeable on the SVG spec

Absolutely true! While I've contributed my opinion here about things related to PHP and to the library structure, the core code is something I have no knowledge of at all. Intuitively, I think it would be easier for the core developers to work with PHP 8, which is why I was advocating for it.

Where I could contribute is things like rector and phpstan and (maybe) helping set up github actions, but I don't even have PHP 7 on my computer. Requiring developers to also support PHP 7 may hinder recruiting them, but as you say, it's very hard to find anyone with knowledge of the SVG spec to contribute.

meyfa commented 8 months ago

I appreciate everybody's input! We should keep an eye on the usage numbers going forward - I was indeed convinced that there are large benefits waiting with PHP 8 and beyond, however, I would still favor compatibility in this situation.

I've opened #217 for dropping PHP 7.3, and am planning on merging it tomorrow. Following that, I expect someone to raise a PR adding PHP 7.4 syntax. I might be able to do this over the weekend but can't make any promises. If someone else is interested in volunteering, that'd be even better :)

tacman commented 8 months ago

I can give it a first pass.

tacman commented 7 months ago

Congrats!

meyfa commented 7 months ago

I've decided to publish v0.15.0 with the PHP 7.4 requirement.

tacman commented 7 months ago

Great! Because of the way github handles releases < 1.0, I hope you'll be able to mark a 1.0 (stable) release soon.