- 🇩🇪Germany Anybody Porta Westfalica
We shouldn't add a minimum PHP version of there's no reason for in our code. Only if code needs a minor version, it should be added. Caring for security updates of PHP is not our job.
- Status changed to Active
over 1 year ago 10:57pm 19 May 2023 - 🇨🇦Canada ambient.impact Toronto
I think I mainly added the security stuff as an extra motivation for people to use newer versions of PHP, but I agree that it's not our job. The main reason to enforce a minimum PHP version is so that the module won't be installable by Composer on a version of PHP that doesn't have the language features we use. For example, PHP 7.4 added class property type declarations, so classes that utilize them would be incompatible with PHP 7.3 and older. Specifying a minimum PHP version might prevent people from installing the module on an older version of PHP and then opening bug reports about the weird fatal errors they're getting because they didn't check the installation instructions or other documentation. It's dumb, but I've seen plenty of bug reports that could have been prevented in this way. Feel free to close if you don't agree, but wanted to explain why I think this is good to have.
- 🇩🇪Germany Anybody Porta Westfalica
The main reason to enforce a minimum PHP version is so that the module won't be installable by Composer on a version of PHP that doesn't have the language features we use.
That's the correct version and totally fine then! :)
So we should set the version that's minimally required by the code. We shouldn't care if it's EOL. So 7.3 is still correct or do we need 7.4 or even 8.0?
- 🇩🇪Germany christianadamski Berlin, Germany
This is a relevant question. Especially for 2.x. D10 requires PHP 8.1. This is turn would allow for way stricter type hints which drastically improves code quality.
So I would recommend increasing requirements for the 2.x branch to PHP 8.1 and then start adding type hints.
- 🇨🇦Canada ambient.impact Toronto
Personally, I'd prefer to increase the minimum PHP taking both language features and end of life into consideration. While it's true that most of the stuff I work on doesn't have the primary purpose of keeping someone's PHP project secure, I treat everything I put out there as a teaching tool and to demonstrate best practices; I got to where I am because of all of the code other people have put out there, so this pays it forward. It's one of the reasons I document my code so thoroughly as well, so that people can learn from it. That's mostly just my own philosophy, so it's not a requirement here but wanted to explain where I'm coming from.
So we should set the version that's minimally required by the code. We shouldn't care if it's EOL. So 7.3 is still correct or do we need 7.4 or even 8.0?
I think 7.4 might be the minimum if we use certain union type hints but I haven't checked.
This is a relevant question. Especially for 2.x. D10 requires PHP 8.1. This is turn would allow for way stricter type hints which drastically improves code quality.
That's a good point - since Drupal 10 has a minimum of PHP 8.1, that would answer this question for us for 2.x. It might be a bit technically redundant to specify both a Drupal core minimum of 10 and minimum PHP 8.1, but if we start using language features that were introduced in PHP 8.0, for example, it would still be in line with my overall best practices philosophy to specify both PHP 8.0 and Drupal 10, and Composer will choose the highest minimum automatically.
- Status changed to Needs review
over 1 year ago 6:00am 16 June 2023 - 🇩🇪Germany Anybody Porta Westfalica
I implemented the requested change, as there seems to be a consensus. Please review.
- last update
over 1 year ago 1 pass - @anybody opened merge request.
- Status changed to Needs work
over 1 year ago 9:54am 10 August 2023 - 🇩🇪Germany Grevil
Hm, I disagree. We shouldn't drop the PHP 7.4 support only to support Union type hinting. If union type hinting is really necessary, we can still define it as a PHPDoc annotation. No need to drop the support if it isn't actually used (or necessary).
- 🇩🇪Germany Anybody Porta Westfalica
Let's plan this for 3.x so we don't force anyone with an existing installation.
- Status changed to Closed: won't fix
over 1 year ago 2:19pm 10 August 2023 - 🇩🇪Germany Grevil
I don't see a reason to introduce this (yet). Let's introduce this feature inside an issue, which introduces php 8 only features! Closing this for now.