Targeting PHP ^8.0

[oojacoboo]

trafficstars

This post is for planning purposes. Sometime in the next year or so, PHP 7 will be unsupported, no longer receiving security updates. It's reasonable that we drop support for PHP 7, only targeting PHP ^8.0, sometime leading up to that date. PHP 7.4 is the only version that's still receiving security updates, at the moment. Users that wish to continue using PHP 7.4, can always target a previous version.

The main motivation and reasoning for this, would be to remove support for annotations, entirely. However, there may be some other refactoring and cleanup that can be done.

• Remove all annotation processing and handling logic
• Remove annotation cache support, as it's no longer needed
• Consider removal of support for MyCLabs\Enum in favor of native Enums.

[l-you]

Maybe it's time to support php >=8.1 only? Symfony 6.1 is out, and it has no support for php 8.0 .

[oojacoboo]

@bladl why not continue to support PHP 8.0? We can still support Symfony 6.1 components with PHP 8.0 support as well. It just means if your target PHP version is PHP 8.0, you won't get Symfony 6.1, but a lower version that we also support. I'm not aware of any BC issues here.

I do think we could move forward with updates to target a minimum of PHP 8.0 now though. Security support for PHP 7.4 ends on 28 Nov 2022.

[l-you]

Features available in php 8.1 could greatly increase code readability and reduce potential bugs. Enums, readonly properties, array_is_list and pure intersection types. According to usage statistics php 8.1 has greater usage than 8.0 for now and in my opinion with time this break even going to grow faster.

[oojacoboo]

@bladl Enums are already implemented and don't require that we drop 8.0 support. Readonly properties and intersection types are cool, but I don't see any need to implement them requiring an 8.0 drop yet. Being that this is a widely distributed OSS lib, we have to be more accommodating of user's upgrade restrictions.

I support following PHP's own version support dates: https://www.php.net/supported-versions.php

Security fix support for 7.4 will end Nov 28th. At that point, support should be dropped for 7.4. We should follow the same for all versions, dropping support when security fixes are no longer provided, unless there is a legitimate need to deviate.

[oojacoboo]

It'd be great to get a PR put together to begin cleaning up for the 8.0 min support if someone wants to take the initiative.

[l-you]

@bladl Enums are already implemented and don't require that we drop 8.0 support.

I tell about internal use of enums. For example replacing constants below with enums:

It would be great to expand their usage today.

[l-you]

It'd be great to get a PR put together to begin cleaning up for the 8.0 min support if someone wants to take the initiative.

Will this changes be versioned as minor update?

[oojacoboo]

This would be a minor update, yes - 5.1 for instance.

[oojacoboo]

I tell about internal use of enums. For example replacing constants below with enums:

I agree it'd be nice to replace constants with Enums. However, it doesn't change functionality in any meaningful way. Therefore, it's not enough to break support for 8.0 at this time, I feel. I'm in support of being ahead of PHP's dates by some months, but 8.0 is supported for at least another year.

Also, there are a lot of updates that can be made moving the codebase to 8.0 min support, alone. Let's focus on those first.

[l-you]

This would be a minor update, yes - 5.1 for instance.

Shouldn't it be a version 6.1? Current version is 6.0 as I guess. But there is no version 6.0 in CHANGELOG.md

[oojacoboo]

Yes, 6.1 - sorry. The CHANGELOG looks like it didn't get updated on the last release. Honestly, that file is mostly a duplicate of the release notes. I'm wondering if we should just remove all the content and note to reference the release notes.

https://github.com/thecodingmachine/graphqlite/releases

[l-you]

Can annotations like #[Field], #[Type], etc. be abandoned in favor of attributes from now?

[oojacoboo]

We definitely can. It would obviously be a BC break. I’m, personally, in favor of doing so and removing Doctrine annotation dependencies. It’ll probably require many people to refactor that bit. Luckily Rector has something for refactoring annotations to attributes. Maybe we could provide instructions on how to automate that refactor?

[l-you]

Luckily Rector has something for refactoring annotations to attributes. Maybe we could provide instructions on how to automate that refactor?

Maybe Im wrong, but documentation already has section about rector on the website

[oojacoboo]

Excellent. I probably added that, actually. We can just reference that in the release notes.

[l-you]

Can I replace Webmozart\Assert\Assert with native assert() for performance and better IDE code suggestion reason?

[oojacoboo]

Can I replace Webmozart\Assert\Assert with native assert() for performance and better IDE code suggestion reason?

Does that Assert lib throw exceptions in a non dev env? If the result is the same, I don’t see why not. However, I haven’t looked into it’s intended use.

[l-you]

Does that Assert lib throw exceptions in a non dev env? If the result is the same, I don’t see why not. However, I haven’t looked into it’s intended use.

Okay! I will replace it with condition + exception if that assertion is important to be user-friendly.

[l-you]

There are many unnecessary assertions that can be replaced in favor of methods type-safety. So we could remove entire webmozart/assert lib. PR is on the way!

[oojacoboo]

Right yea, let’s do it

[andrew-demb]

Can I replace Webmozart\Assert\Assert with native assert() for performance and better IDE code suggestion reason?

Does that Assert lib throw exceptions in a non dev env? If the result is the same, I don’t see why not. However, I haven’t looked into it’s intended use.

PHP's assert() wont be executed for non-dev environment. webmozart/assert checks and assert() expressions are the same from type safety perspective. webmozart/assert provides information in method contract about guarranted checked types in result.

So if there is a motivation to get less dependencies, simpler code and less checks for types in non-dev mode - it is OK to replace checks 🙂

[l-you]

So if there is a motivation to get less dependencies, simpler code and less checks for types in non-dev mode - it is OK to replace checks 🙂

Those assertions mostly validate data passed and used by internals. So, main purpose of those assertions were to catch errors on unit-testing stage. Assertions that were interacting with external data thrown a specific message. So I replaced it with constructions if (!$isValid) throw Exception('msg that was before'). In summary, it should not have drawbacks in non-dev mode.