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

PHP 8.3 compatibility (and 8.x prototypes fixes) #616

Open
wants to merge 31 commits into
base: master
Choose a base branch
from

Conversation

remicollet
Copy link
Contributor

Work in progress, before:

========DIFF========
001- success
001+ Fatal error: Imagick::getResourceLimit(): Return value must be of type int, float returned in Unknown on line 0
========DONE========
FAIL Imagick::setResourceLimit test [tests/014-setresourcelimit.phpt] 

001- Ok
001+ Fatal error: Arginfo / zpp mismatch during call of ImagickPixel::setColorValueQuantum() in /work/GIT/pecl-and-ext/imagick/tests/172_ImagickPixel_setColorValueQuantum_basic.php on line 11
========DONE========
FAIL Test ImagickPixel, setColorValueQuantum [tests/172_ImagickPixel_setColorValueQuantum_basic.phpt] 

========DIFF========
001- %s: ImagickPixelIterator::newPixelIterator is deprecated. ImagickPixelIterator::getPixelIterator should be used instead in %s on line %d
002- done
001+ php: /work/build/phpmaster/Zend/zend_execute_API.c:975: zend_call_function: Assertion `!(call->func->common.fn_flags & (1 << 13)) || zend_verify_internal_return_type(call->func, fci->retval)' failed.
002+ 
003+ Termsig=6
========DONE========
FAIL Pixel Iterator tests [tests/020-pixeliterator.phpt] 

========DIFF========
001- Complete
001+ Fatal error: Arginfo / zpp mismatch during call of ImagickKernel::fromMatrix() in /work/GIT/pecl-and-ext/imagick/tests/145_imagickkernel_coverage.php on line 10
========DONE========
FAIL ImagickKernel::fromMatrix test [tests/145_imagickkernel_coverage.phpt] 

========DIFF========
001- Temporary-path was empty at start.
002- Temporary path was set correctly.
003- Temporary path was listed correctly.
004- This is fine.
001+ Fatal error: Imagick::getRegistry(): Return value must be of type string, false returned in Unknown on line 0
========DONE========
FAIL Test Imagick, setRegistry and getRegistry [tests/150_Imagick_setregistry.phpt] 

========DIFF========
001- Ok
001+ Fatal error: Arginfo / zpp mismatch during call of ImagickKernel::fromMatrix() in /work/GIT/pecl-and-ext/imagick/tests/047_Imagick_convolveImage_7.php on line 13
========DONE========
FAIL Test Imagick, convolveImage [tests/047_Imagick_convolveImage_7.phpt] 

001- height : 25
002- width : 25
003- x : 50
004- y : 50
005- Ok
001+ Fatal error: Arginfo / zpp mismatch during call of Imagick::subimageMatch() in /work/GIT/pecl-and-ext/imagick/tests/151_Imagick_subImageMatch_basic.php on line 18
========DONE========
FAIL Test Imagick, subImageMatch [tests/151_Imagick_subImageMatch_basic.phpt] 

========DIFF========
     format: png (portable network graphics)
     units: undefined
     type: palette
004- Image geometry 640x480
004+ Image geometry 640x480[Wed May 31 08:15:00 2023]  Script:  '/work/GIT/pecl-and-ext/imagick/tests/236_Imagick_identify_basic.php'
005+ /opt/php83/include/php/Zend/zend_string.h(174) :  Freeing 0x00007fb9bfc04070 (72 bytes), script=/work/GIT/pecl-and-ext/imagick/tests/236_Imagick_identify_basic.php
006+ Last leak repeated 598 times
007+ [Wed May 31 08:15:00 2023]  Script:  '/work/GIT/pecl-and-ext/imagick/tests/236_Imagick_identify_basic.php'
008+ /work/build/phpmaster/Zend/zend_hash.c(291) :  Freeing 0x00007fb9bfcaa480 (56 bytes), script=/work/GIT/pecl-and-ext/imagick/tests/236_Imagick_identify_basic.php
009+ === Total 600 memory leaks detected ===
========DONE========
FAIL Test Imagick, identifyImage [tests/236_Imagick_identify_basic.phpt] 

001- Ok
001+ php: /work/build/phpmaster/Zend/zend_execute_API.c:975: zend_call_function: Assertion `!(call->func->common.fn_flags & (1 << 13)) || zend_verify_internal_return_type(call->func, fci->retval)' failed.
002+ 
003+ Termsig=6
========DONE========
FAIL Test Tutorial, fxAnalyzeImage [tests/229_Tutorial_fxAnalyzeImage_case1.phpt] 

========DIFF========
--
     Frame: 3
     Frame: 4
     Frame: 5
007- Ok
007+ 
008+ Fatal error: Imagick::optimizeImageLayers(): Return value must be of type bool, Imagick returned in Unknown on line 0
========DONE========
FAIL Test Imagick::optimizeimagelayers and Imagick::optimizeimagetransparency [tests/278_Imagick_optimaze_gif.phpt] 

========DIFF========
001- Ok
001+ php: /work/build/phpmaster/Zend/zend_execute_API.c:975: zend_call_function: Assertion `!(call->func->common.fn_flags & (1 << 13)) || zend_verify_internal_return_type(call->func, fci->retval)' failed.
002+ 
003+ Termsig=6
========DONE========
FAIL Test Imagick, getPixelIterator [tests/083_Imagick_getPixelIterator_basic.phpt] 

========DIFF========
001- Ok
001+ php: /work/build/phpmaster/Zend/zend_execute_API.c:975: zend_call_function: Assertion `!(call->func->common.fn_flags & (1 << 13)) || zend_verify_internal_return_type(call->func, fci->retval)' failed.
002+ 
003+ Termsig=6
========DONE========
FAIL Test Imagick, getPixelRegionIterator [tests/084_Imagick_getPixelRegionIterator_basic.phpt] 

========DIFF========
     Depth is 16
002- Ok
002+ 
003+ Fatal error: Imagick::getImageBlob(): Return value must be of type string, null returned in Unknown on line 0
========DONE========
FAIL Test Imagick, setDepth [tests/325_Imagick_setDepth.phpt] 

========DIFF========
001- Ok
001+ php: /work/build/phpmaster/Zend/zend_vm_execute.h:1877: ZEND_DO_FCALL_SPEC_RETVAL_UNUSED_HANDLER: Assertion `!(call->func->common.fn_flags & (1 << 13)) || zend_verify_internal_return_type(call->func, ret)' failed.
002+ 
003+ Termsig=6
========DONE========
FAIL Test Imagick, setImageMask basic [tests/286_Imagick_setMask_basic.phpt] 

========DIFF========
001- Ok
001+ php: /work/build/phpmaster/Zend/zend_vm_execute.h:1877: ZEND_DO_FCALL_SPEC_RETVAL_UNUSED_HANDLER: Assertion `!(call->func->common.fn_flags & (1 << 13)) || zend_verify_internal_return_type(call->func, ret)' failed.
002+ 
003+ Termsig=6
========DONE========
FAIL Test Imagick, medianFilterImage [tests/289_Imagick_setImageMask_basic.phpt] 

001- 0
002- 0
003- 1
004- 1
005- 2
006- 2
007- 0
008- 1
009- 2
010- 0
011- 1
012- 2
013- still 2 as hasn't changed
014- Exception: Unable to set image index
015- Ok
001+ php: /work/build/phpmaster/Zend/zend_execute_API.c:975: zend_call_function: Assertion `!(call->func->common.fn_flags & (1 << 13)) || zend_verify_internal_return_type(call->func, fci->retval)' failed.
002+ 
003+ Termsig=6
========DONE========
FAIL Test iterating over images works [tests/292_index_iterator.phpt] 

========DIFF========
     true
002- false
003- true
002+ php: /work/build/phpmaster/Zend/zend_vm_execute.h:1877: ZEND_DO_FCALL_SPEC_RETVAL_UNUSED_HANDLER: Assertion `!(call->func->common.fn_flags & (1 << 13)) || zend_verify_internal_return_type(call->func, ret)' failed.
003+ 
004+ Termsig=6
========DONE========
FAIL Test pseudo formats [tests/246_antialias_image.phpt] 
                                                   
========DIFF========
001- Ok
001+ php: /work/build/phpmaster/Zend/zend_execute_API.c:975: zend_call_function: Assertion `!(call->func->common.fn_flags & (1 << 13)) || zend_verify_internal_return_type(call->func, fci->retval)' failed.
002+ 
003+ Termsig=6
========DONE========
FAIL Test ImagickPixelIterator, construct [tests/247_ImagickPixelIterator_construct_basic.phpt] 
                                                   
========DIFF========
001- Ok
001+ php: /work/build/phpmaster/Zend/zend_execute_API.c:975: zend_call_function: Assertion `!(call->func->common.fn_flags & (1 << 13)) || zend_verify_internal_return_type(call->func, fci->retval)' failed.
002+ 
003+ Termsig=6
========DONE========
FAIL Test ImagickPixelIterator, clear [tests/248_ImagickPixelIterator_clear_basic.phpt] 

001- Ok
001+ Fatal error: ImagickPixelIterator::getNextIteratorRow(): Return value must be of type array, null returned in Unknown on line 0
========DONE========
FAIL Test ImagickPixelIterator, getNextIteratorRow [tests/249_ImagickPixelIterator_getNextIteratorRow_basic.phpt] 

001- Ok
001+ php: /work/build/phpmaster/Zend/zend_execute_API.c:975: zend_call_function: Assertion `!(call->func->common.fn_flags & (1 << 13)) || zend_verify_internal_return_type(call->func, fci->retval)' failed.
002+ 
003+ Termsig=6
========DONE========
FAIL Test ImagickPixelIterator, resetIterator [tests/250_ImagickPixelIterator_resetIterator_basic.phpt] 

========DIFF========
001- Ok
001+ php: /work/build/phpmaster/Zend/zend_execute_API.c:975: zend_call_function: Assertion `!(call->func->common.fn_flags & (1 << 13)) || zend_verify_internal_return_type(call->func, fci->retval)' failed.
002+ 
003+ Termsig=6
========DONE========
FAIL Test ImagickPixelIterator, construct [tests/252_ImagickPixelIterator_construct_basic.phpt] 

========DIFF========
001- Ok
001+ Fatal error: Imagick::evaluateImages(): Return value must be of type bool, Imagick returned in Unknown on line 0
========DONE========
FAIL Test Imagick, Imagick::evaluateImages [tests/258_Imagick_evaluateImages_basic.phpt] 

========DIFF========
001- Ok
001+ php: /work/build/phpmaster/Zend/zend_vm_execute.h:1877: ZEND_DO_FCALL_SPEC_RETVAL_UNUSED_HANDLER: Assertion `!(call->func->common.fn_flags & (1 << 13)) || zend_verify_internal_return_type(call->func, ret)' failed.
002+ 
003+ Termsig=6
========DONE========
FAIL Test localContrastImage [tests/260_localContrastImage.phpt] 

========DIFF========
001- Ok
001+ php: /work/build/phpmaster/Zend/zend_vm_execute.h:1877: ZEND_DO_FCALL_SPEC_RETVAL_UNUSED_HANDLER: Assertion `!(call->func->common.fn_flags & (1 << 13)) || zend_verify_internal_return_type(call->func, ret)' failed.
002+ 
003+ Termsig=6
========DONE========

========DIFF========
001- Ok
001+ php: /work/build/phpmaster/Zend/zend_vm_execute.h:1877: ZEND_DO_FCALL_SPEC_RETVAL_UNUSED_HANDLER: Assertion `!(call->func->common.fn_flags & (1 << 13)) || zend_verify_internal_return_type(call->func, ret)' failed.
002+ 
003+ Termsig=6
========DONE========
FAIL Test autoGammaImage [tests/263_autoGammaImage.phpt] 

========DIFF========
001- Ok
001+ php: /work/build/phpmaster/Zend/zend_vm_execute.h:1877: ZEND_DO_FCALL_SPEC_RETVAL_UNUSED_HANDLER: Assertion `!(call->func->common.fn_flags & (1 << 13)) || zend_verify_internal_return_type(call->func, ret)' failed.
002+ 
003+ Termsig=6
========DONE========
FAIL Test Imagick, mergeImageLayers [tests/092_Imagick_mergeImageLayers_basic.phpt] 

@remicollet
Copy link
Contributor Author

Current state, single failure, probably not related to 8.3

========DIFF========
     format: png (portable network graphics)
     units: undefined
     type: palette
004- Image geometry 640x480
004+ Image geometry 640x480[Wed May 31 12:18:34 2023]  Script:  '/work/GIT/pecl-and-ext/imagick/tests/236_Imagick_identify_basic.php'
005+ /opt/php83/include/php/Zend/zend_string.h(174) :  Freeing 0x00007fcf55004070 (72 bytes), script=/work/GIT/pecl-and-ext/imagick/tests/236_Imagick_identify_basic.php
006+ Last leak repeated 598 times
007+ [Wed May 31 12:18:34 2023]  Script:  '/work/GIT/pecl-and-ext/imagick/tests/236_Imagick_identify_basic.php'
008+ /work/build/phpmaster/Zend/zend_hash.c(291) :  Freeing 0x00007fcf550aa480 (56 bytes), script=/work/GIT/pecl-and-ext/imagick/tests/236_Imagick_identify_basic.php
009+ === Total 600 memory leaks detected ===
========DONE========
FAIL Test Imagick, identifyImage [tests/236_Imagick_identify_basic.phpt] 

AND

=====================================================================
TEST RESULT SUMMARY
---------------------------------------------------------------------
Exts skipped    :     0
Exts tested     :    35
---------------------------------------------------------------------

Number of tests :   333               322
Tests skipped   :    11 (  3.3%) --------
Tests warned    :     3 (  0.9%) (  0.9%)
Tests failed    :     1 (  0.3%) (  0.3%)
Expected fail   :     1 (  0.3%) (  0.3%)
Tests passed    :   317 ( 95.2%) ( 98.4%)
---------------------------------------------------------------------
Time taken      :    13 seconds
=====================================================================

=====================================================================
EXPECTED FAILED TEST SUMMARY
---------------------------------------------------------------------
Test Imagick, affineTransformImage [tests/031_Imagick_affineTransformImage_basic.phpt]  XFAIL REASON: I don't understand what values are returned in which elements of getImageChannelStatistics
=====================================================================

=====================================================================
FAILED TEST SUMMARY
---------------------------------------------------------------------
Test Imagick, identifyImage [tests/236_Imagick_identify_basic.phpt]
=====================================================================

=====================================================================
WARNED TEST SUMMARY
---------------------------------------------------------------------
ImagickPixel iterator [tests/bug_73840.phpt] (warn: XFAIL section but test passes)
Test ImagickDraw, getDensity [tests/268_ImagickDraw_getDensity_basic.phpt] (warn: XFAIL section but test passes)
Test Imagick, GetImageChannelRange basic [tests/287_Imagick_GetImageChannelRange_basic.phpt] (warn: XFAIL section but test passes)
=====================================================================

@remicollet
Copy link
Contributor Author

@Danack Ready for review

Notice:

There is lot of place with

	if (php_imagick_ensure_not_empty (intern->magick_wand) == 0)
		return;

So this implies allowing null for the return type (ex: public function autoOrient(): ?bool {})

An alternative way will be to raise an exception in such cases.

@Danack
Copy link
Collaborator

Danack commented May 31, 2023

@Danack Ready for review

Cool, thanks.

An alternative way will be to raise an exception in such cases.

I think it does actually raise an exception, but it would probably be good to change the name to make that more obvious.

@remicollet
Copy link
Contributor Author

I think it does actually raise an exception,

My bad, should have check this helper

Should be ok now

@remicollet
Copy link
Contributor Author

but it would probably be good to change the name to make that more obvious.

Or perhaps use RETURN_THROW() everywhere after exception ?

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

@Danack last commit use RETURN_THROWS everywhere in ImagickDraw (which allows me to find some more minor issues), If you are OK, I will add same for other classes.

@remicollet
Copy link
Contributor Author

And CI is happy (2 failures related to #617 )

@remicollet remicollet changed the title PHP 8.3 compatibility PHP 8.3 compatibility (and 8.x prototypes fixes) Jun 1, 2023
@remicollet
Copy link
Contributor Author

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

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.

@Danack
Copy link
Collaborator

Danack commented Jun 1, 2023

The RETURN_THROWS for the parameter passing looks good.

I'm thinking that maybe the code that is currently:

    intern = Z_IMAGICK_P(getThis());
    if (php_imagick_ensure_not_empty (intern->magick_wand) == 0)
	return;

Could be replaced by a macro, as there is an awful lot of repetition of that in the code. What do you think?

@remicollet
Copy link
Contributor Author

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
See last commit.

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

remicollet commented Jun 1, 2023

CI is happy again, so ready for review.

I have tried to remove all unneeded "return;"

All remaining have to be checked later, as this means the method may return null (unset return_value), so have to be in its proto, but this PR already has too much work... and the review will be painful, so don't want to add more (other PR may come later)

@ghostwriter
Copy link

@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! ✌🏾

@Wirone
Copy link

Wirone commented Nov 3, 2023

Just wanted to confirm this fixes issue with installing Imagick on PHP8.3 during Docker build (php:8.3.0RC5-fpm-bullseye). Thank you very much for this @remicollet and fingers crossed it will get merged and released soon 🙂.

@waja
Copy link

waja commented Nov 3, 2023

28f2704 compiled well on php:8.3.0RC5-fpm-alpine. See https://github.com/waja/docker-php83-fpm/blob/development/Dockerfile

@remicollet
Copy link
Contributor Author

@Danack ping

@waja
Copy link

waja commented Nov 23, 2023

master compiles also well on php:8.3.0RC6-fpm-alpine.

@schmunk42
Copy link

FYI: we were able to build it on PHP 8.3 for amd64, but not arm64

#52 2615.4 In /tmp/pear/temp/imagick/Imagick.stub.php:
#52 2615.4 Unterminated preprocessor conditions
#52 2615.4 make: *** [Makefile:196: /tmp/pear/temp/imagick/Imagick_arginfo.h] Error 1
#52 2615.4 ERROR: `make -j4 INSTALL_ROOT="/tmp/pear/temp/pear-build-defaultuseryTW5hI/install-imagick-3.7.0" install' failed

https://github.com/yiisoft/yii2-docker/actions/runs/6979511769/job/18992949588#step:13:5408

@remicollet
Copy link
Contributor Author

remicollet commented Nov 24, 2023

FYI: we were able to build it on PHP 8.3 for amd64, but not arm64
https://github.com/yiisoft/yii2-docker/actions/runs/6979511769/job/18992949588#step:13:5408

IN log:

#52 2612.3 running: make -j4 INSTALL_ROOT="/tmp/pear/temp/pear-build-defaultuseryTW5hI/install-imagick-3.7.0" install
#52 2612.5 Parse /tmp/pear/temp/imagick/ImagickDraw.stub.php to generate /tmp/pear/temp/imagick/ImagickDraw_arginfo.h
#52 2612.5 Parse /tmp/pear/temp/imagick/ImagickPixel.stub.php to generate /tmp/pear/temp/imagick/ImagickPixel_arginfo.h
#52 2612.5 Parse /tmp/pear/temp/imagick/Imagick.stub.php to generate /tmp/pear/temp/imagick/Imagick_arginfo.h

This should not happen, such generated files are not usable (in this ext)
A touch on the headers may be a workaround

@remicollet
Copy link
Contributor Author

remicollet commented Nov 24, 2023

Just tested on aarch64, with PHP 8.3.0

[remi@builder2 imagick (issue-php83)]$ make test TESTS='-j64 --show-diff'
=====================================================================
PHP         : /usr/bin/php 
PHP_SAPI    : cli
PHP_VERSION : 8.3.0
ZEND_VERSION: 4.3.0
PHP_OS      : Linux - Linux builder2.remirepo.net 5.14.0-362.8.1.el9_3.aarch64 #1 SMP PREEMPT_DYNAMIC Tue Oct 3 11:57:53 EDT 2023 aarch64
INI actual  : /home/remi/work/imagick/tmp-php.ini
More .INIs  :  
---------------------------------------------------------------------

...

=====================================================================
TEST RESULT SUMMARY
---------------------------------------------------------------------
Exts skipped    :     0
Exts tested     :    17
---------------------------------------------------------------------

Number of tests :   333               322
Tests skipped   :    11 (  3.3%) --------
Tests warned    :     3 (  0.9%) (  0.9%)
Tests failed    :     0 (  0.0%) (  0.0%)
Expected fail   :     1 (  0.3%) (  0.3%)
Tests passed    :   318 ( 95.5%) ( 98.8%)
---------------------------------------------------------------------
Time taken      :    11 seconds
=====================================================================

@Wirone
Copy link

Wirone commented Jan 31, 2024

@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 Dockerfile:

ARG IMAGICK_PHP83_FIX_COMMIT=9df92616f577e38625b96b7b903582a46c064739

...

    && curl -L https://github.com/remicollet/imagick/archive/${IMAGICK_PHP83_FIX_COMMIT}.zip -o /tmp/imagick-issue-php83.zip \
    && unzip /tmp/imagick-issue-php83.zip -d /tmp \
    && pecl install /tmp/imagick-${IMAGICK_PHP83_FIX_COMMIT}/package.xml \

but as you may know it's not a solution you would like to rely in such a big, client-facing application like GetResponse 😉.

@Wirone
Copy link

Wirone commented Feb 1, 2024

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.

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 this pull request may close these issues.

None yet

6 participants