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

Drop unmaintained PHP, publish a release #215

Closed
tacman opened this issue Feb 6, 2024 · 14 comments · Fixed by #226
Closed

Drop unmaintained PHP, publish a release #215

tacman opened this issue Feb 6, 2024 · 14 comments · Fixed by #226

Comments

@tacman
Copy link
Contributor

tacman commented Feb 6, 2024

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
Copy link
Owner

meyfa commented Feb 6, 2024

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
Copy link
Contributor Author

tacman commented Feb 6, 2024

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
Copy link
Contributor

Niellles commented Feb 8, 2024

Kindly refer to #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
Copy link
Contributor

Niellles commented Feb 9, 2024

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
Copy link
Contributor

Niellles commented Feb 12, 2024

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

  • 25-30% of users running PHP 7.2. *notes
  • ~0.5% of users running PHP 7.3
  • ~6% of users running PHP 7.4
  • ~8% of users running PHP 8.0
  • ~55% of users running >=PHP 8.1

afbeelding

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

  • No installs running PHP 7.3
  • 0.1% users running PHP 7.4
  • 99.9% users running PHP 8.x).

As for 0.14.x:

  • 0-0.2% users running PHP 7.3
  • ~5% users running PHP 7.4

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
Copy link
Owner

meyfa commented Feb 12, 2024

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

  • union types - this would be nice
  • saner string to number comparisons (0 == 'foobar' is no longer true) - we had issues with this in the past, and it would be awesome to fix this once and for all.

PHP 8.1 would give us (among others):

  • enums
  • readonly properties
  • final class constants

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:

  • I would like us to make a conscious choice around attribute getters/setters for known properties, like SVGRect::getRX() or SVGPath::getDescription().
    • Do we want these at all? From a certain viewpoint, they are entirely redundant, and sometimes you have to use getAttribute()/setAttribute() anyway.
    • If we don't want them, we shouldn't have any whatsoever. If we want them, we should have all of them (i.e., for every known property on every element).
    • The names should be consistent. Notice how SVGRect::getRX() uses the short-form RX for the rx attribute while SVGPath::getDescription() uses a long-form (the attribute is called d), and SVGCircle::getCenterX() uses a long form again.
  • It's currently impossible to mix text nodes and tags arbitrarily. For example, the MDN example <text>You are <tspan>not</tspan> a banana!</text> cannot be represented in PHP-SVG. I think fixing this could be quite a significant breaking change, but very important to do.
  • As said above, if we want to support different rendering backends such as Imagick/Gmagick, this redesign should happen before v1.
  • We would need to figure out how we want to handle stylesheets. The current solution, especially the getIdAndClassPattern API and surrounding logic, feels badly designed. We wouldn't need to support any particular amount of CSS syntax, but the architectural framework should be in place (and getIdAndClassPattern removed).
  • Honorable mention: The font system was redesigned in between Dropping support for PHP versions < 7.3 #180 and now, which was a major pain-point at that time, but is not a blocker any more.

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
Copy link
Contributor Author

tacman commented Feb 12, 2024

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
Copy link
Contributor

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
Copy link
Contributor Author

tacman commented Feb 14, 2024

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 added a commit that referenced this issue Feb 14, 2024
See #215 for discussion.
@meyfa
Copy link
Owner

meyfa commented Feb 14, 2024

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

Niellles added a commit to Niellles/php-svg that referenced this issue Feb 14, 2024
As discussed/requested in meyfa#215, PHP 7.3 is dropped,  and typed properties shoud be added where possible.
@tacman
Copy link
Contributor Author

tacman commented Feb 14, 2024

I can give it a first pass.

@tacman
Copy link
Contributor Author

tacman commented Mar 17, 2024

Congrats!

@meyfa
Copy link
Owner

meyfa commented Mar 17, 2024

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

@tacman
Copy link
Contributor Author

tacman commented Mar 17, 2024

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

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 a pull request may close this issue.

3 participants