-
-
Notifications
You must be signed in to change notification settings - Fork 145
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
PHP 8.3 compatibility (and 8.x prototypes fixes) #616
Conversation
Current state, single failure, probably not related to 8.3
AND
|
@Danack Ready for review Notice: There is lot of place with
So this implies allowing null for the return type (ex: An alternative way will be to raise an exception in such cases. |
Cool, thanks.
I think it does actually raise an exception, but it would probably be good to change the name to make that more obvious. |
My bad, should have check this helper Should be ok now |
Or perhaps use |
- use RETURN_THROWS when possible - fix setFillColor, setStrokeColor, setTextUnderColor and setBorderColor to return FALSE instead of NULL on error - fix getTextEncoding and getClipPath proto (may return false)
@Danack last commit use |
Despite initial work was for 8.3, the same errors appear on 8.2 (using 8.2.8-dev debug build) So only the review object_handlers commit is really specific for 8.3 |
@@ -1051,52 +1060,76 @@ PHP_MINIT_FUNCTION(imagick) | |||
#endif | |||
|
|||
php_imagick_sc_entry = zend_register_internal_class(&ce TSRMLS_CC); | |||
#if PHP_VERSION_ID >= 80300 | |||
php_imagick_sc_entry->create_object = php_imagick_object_new; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
create object affectation can be done this way at least since 8.1, just wanted to reduce complexity and avoid additional conditions.
The RETURN_THROWS for the parameter passing looks good. I'm thinking that maybe the code that is currently:
Could be replaced by a macro, as there is an awful lot of repetition of that in the code. What do you think? |
Indeed... 334 usages |
- use RETURN_THROWS after zpp and exception - use new IMAGICK_PIXEL_NOT_EMPTY macro - fix isPixelSimilar and isPixelSimilarQuantum proto (may return null)
- use RETURN_THROWS after zpp and exception - use IMAGICK_NOT_EMPTY macro
- use RETURN_THROWS after zpp and exception
CI is happy again, so ready for review. I have tried to remove all unneeded " All remaining have to be checked later, as this means the method may return |
@remicollet Thank you, I appreciate the time you invested in preparing this pull request and for all of the work you've contributed to the PHP community. You too @Danack, Thank you for the time and energy you've spent maintaining this project. I appreciate you both! Have a great weekend! ✌🏾 |
Just wanted to confirm this fixes issue with installing Imagick on PHP8.3 during Docker build ( |
28f2704 compiled well on |
@Danack ping |
|
FYI: we were able to build it on PHP 8.3 for
|
IN log:
This should not happen, such generated files are not usable (in this ext) |
Just tested on aarch64, with PHP 8.3.0
|
@Danack sorry for pinging, but is there any particular reason this was not reviewed and merged? We would like to bump our Docker runtime to PHP 8.3 but don't want to rely on fork. I have this in my PoC
but as you may know it's not a solution you would like to rely in such a big, client-facing application like GetResponse 😉. |
FYI: here's a gist with Docker build output that shows installation failure for Imagick 3.7.0 on PHP 8.3.2 (same was on 8.3.0RC5). Using this PR's code for installation (at least commit 9df9261) fixed it for us. |
@remicollet thanks. Sorry for taking so long. |
@Wirone tbh, health issues mostly. |
Hi @schmunk42,
If you still have issues, please open a separate issue. |
Just for the record ... 8.3 builds with default versions: https://github.com/yiisoft/yii2-docker/actions/runs/9908435638 👍 |
Work in progress, before: