PHP ^7.4 Compatibility (Including 8.1) #1805
Conversation
|
Isn't it a bit too early for this? |
@dereuromark I have no problem with continuing to target ^7.4. Do you have suggestions on how that's going to be achieved though? I outlined the issue in the original comment:
|
|
We will not be able to add any such types, thats fine though. |
| // Accessor visibility | ||
| $visibility = $this->getMethodVisibility('accessorVisibility', 'defaultAccessorVisibility'); | ||
| // Accessor visibility - no idea why this returns null, or the use case for that | ||
| $visibility = $this->getMethodVisibility('accessorVisibility', 'defaultAccessorVisibility') ?: ''; |
There was a problem hiding this comment.
But is continuing with an empty string better then?
Shouldnt this throw an exception or default to a sensible value instead?
There was a problem hiding this comment.
I'm all for throwing Exceptions. But, this lib has a ton of business logic around null values. I don't know if it's the tests, or these values are encountered during normal model generation, or what. In many cases, null is being passed to a core string function that converts it to an empty string. However, with PHP 8, that's not allowed, as there are more strict type checks. In this case, we're just passing an empty string into strtolower instead of null. But there are quite a few cases like this through the codebase.
The best solution here is to type our internal methods/properties. This will prevent most of these issues and start to narrow down the actual source of the issue.
There was a problem hiding this comment.
True
But then I would rather have string|null here, what do you think?
As checking against null vs non empty string sounds a bit cleaner from the API level.
There was a problem hiding this comment.
I'm not really sure I'm following you. $visibility ends up null in some cases. It's then passed to strtolower which results in a type error on PHP 8. Before PHP8, that'd just get converted to an empty string. I'm simply converting to an empty string earlier to avoid a type error.
| @@ -29,6 +29,7 @@ public function testUniqueValidatorPass() | |||
| $publisher = new Publisher(); | |||
| $publisher->setName('Happy Reading'); | |||
| $publisher->setWebsite('http://www.happyreading.com'); | |||
| $publisher->save(); | |||
There was a problem hiding this comment.
Why does it need to be saved before validating?
There was a problem hiding this comment.
It was failing before saving, and other examples all saved, so I assumed so.
There was a problem hiding this comment.
After saving I wouldnt expect validation to do anything, so we might actually "cloak" the issue here this way.
It was working before, we should find out how it will work again now without the save() part.
There was a problem hiding this comment.
Propel\Tests\Generator\Behavior\Validate\UniqueConstraintTest::testUniqueValidatorPass
Failed asserting that false is true.
Without the save, we get the above. I don't know what's failing validation or how validation works. Additionally, it fails on master branch as well for me, locally. So, I'm guessing the issue is unrelated to this PR.
Conflicts: src/Propel/Generator/Builder/DataModelBuilder.php
|
@mringler What do you think? |
|
Any second review please? So we can move forward? |
This PR includes support for PHP >= 7.4, resolving all type errors and deprecation warnings.
I should further add that PHP 7.3 is no longer receiving security updates - it's unmaintained. And, 7.4 will be unmaintained next year.
It appears evident that there is a lot of nullable behavior tied to business logic throughout the lib. It's quite unfortunate. I was able to strictly type and enforce in many places. However, in others, there were some business logic decisions that had to be made. And without greater knowledge of how some of these classes are instantiated and used, these decisions were typically permissive.
I’ve gone ahead and conformed with your code-sniffer rules @dereuromark. I will say though, I disagree with the requirement to include a docblock with PHP 8.1. Going forward, now that we have support to fully type a function, they are self documenting. If a function doesn’t require additional comments, for the function - itself, it’s arguments, return value, throws, etc - a docblock shouldn’t be required.
This PR actually took significantly more time than I expected. There were a lot of little things, especially to get all the tests passing.
The docs need updating to make clear only PHP 7.4 and greater is supported. I didn’t see them in the repo. It’d be nice to have the docs in the repo with a static site generator. I’ve used docusaurus on other OSS libs.
Closes #1795