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

[3.5] remaining problem in arginfo #424

Closed
remicollet opened this issue Jun 16, 2021 · 8 comments
Closed

[3.5] remaining problem in arginfo #424

remicollet opened this issue Jun 16, 2021 · 8 comments

Comments

@remicollet
Copy link
Contributor

Comparing reflection from different version is really a mess ;)

BTW, using a small script to extract methods from reflection and sort them, I was able to compare 3.4.4 and 3.5.0

In 20 diff reported, most are OK (so 3.4.4 was wrong)

But still 5 remainings:

Imagick

         Method [ <internal:imagick> public method colormatriximage ] {
 
           - Parameters [1] {
-            Parameter #0 [ <required> $color_matrix ]
+            Parameter #0 [ <optional> $color_matrix ]
           }
         }

FIX: the array is really mandatory

ImagickKernel

        Method [ <internal:imagick> public method scale ] {

           - Parameters [2] {
             Parameter #0 [ <required> $scale ]
-            Parameter #1 [ <optional> $normalize_flag ]
+            Parameter #1 [ <required> $normalize_kernel ]
           }
         }

FIX: normalize is really optional

ImagickPixel

         Method [ <internal:imagick, ctor> public method __construct ] {
 
           - Parameters [1] {
-            Parameter #0 [ <optional> $color ]
+            Parameter #0 [ <required> $color ]
           }
         }

FIX: color is really optional

         Method [ <internal:imagick> public method getcolorvalue ] {
 
           - Parameters [1] {
-            Parameter #0 [ <required> $color ]
+            Parameter #0 [ <optional> $color ]
           }
         }

FIX: color is really mandatory

         Method [ <internal:imagick> public method getcolorvaluequantum ] {
 
           - Parameters [1] {
-            Parameter #0 [ <required> $color ]
+            Parameter #0 [ <optional> $color ]
           }
         }

FIX: color is really mandatory

Notice: this check is for 7.x (so without type hinting)
I hope to not have miss some other...

@remicollet
Copy link
Contributor Author

About type hitting, need to be fixed

-    public function getColorValue(int $color): IMAGICK_QUANTUM_TYPE {}
+    public function getColorValue(int $color): float {}

From code, always return a float, no HDRI specific case

-    public function setIndex(IMAGICK_QUANTUM_TYPE $index): bool {}
+    public function setIndex(int $index): bool {}

From code, always expect a integer , no HDRI specific case

@Danack
Copy link
Collaborator

Danack commented Jun 16, 2021

From code, always return a float, no HDRI specific case

That might be a bug then: https://imagemagick.org/api/pixel-wand.php#PixelSetIndex

void PixelSetIndex(PixelWand *wand,const Quantum index)

going through the others now.

@Danack
Copy link
Collaborator

Danack commented Jun 16, 2021

Correct Imagick::colorMatrixImage - fixed in 15cae4b

ImagickKernel::scale - Looks to be required, presumably was a bug before?

ImagickPixel::construct - fixed in f58a8d7

ImagickPixel::getcolorvalue - still to look at.
ImagickPixel::getcolorvaluequantum - still to look at.

@Danack
Copy link
Collaborator

Danack commented Jun 16, 2021

ImagickPixel::getcolorvalue - fixed in 2605370
ImagickPixel::getcolorvaluequantum - fixed in 2605370

@remicollet
Copy link
Contributor Author

ImagickKernel::scale - Looks to be required, presumably was a bug before?

I see ""d|l"" so second arg is optional (and default value missing )
878e38c

@remicollet
Copy link
Contributor Author

Reflection diff:

@@ -3557,7 +3557,7 @@
         Method [ <internal:imagick> public method colorMatrixImage ] {
 
           - Parameters [1] {
-            Parameter #0 [ <optional> $color_matrix ]
+            Parameter #0 [ <required> $color_matrix ]
           }
         }
 
@@ -3595,8 +3595,8 @@
             Parameter #0 [ <required> Imagick $image ]
             Parameter #1 [ <optional> &$offset ]
             Parameter #2 [ <optional> &$similarity ]
-            Parameter #3 [ <optional> &$threshold ]
-            Parameter #4 [ <optional> &$metric ]
+            Parameter #3 [ <optional> $threshold ]
+            Parameter #4 [ <optional> $metric ]
           }
         }
 
@@ -3606,8 +3606,8 @@
             Parameter #0 [ <required> Imagick $image ]
             Parameter #1 [ <optional> &$offset ]
             Parameter #2 [ <optional> &$similarity ]
-            Parameter #3 [ <optional> &$threshold ]
-            Parameter #4 [ <optional> &$metric ]
+            Parameter #3 [ <optional> $threshold ]
+            Parameter #4 [ <optional> $metric ]
           }
         }
 
@@ -4858,7 +4858,7 @@
         Method [ <internal:imagick, ctor> public method __construct ] {
 
           - Parameters [1] {
-            Parameter #0 [ <required> $color ]
+            Parameter #0 [ <optional> $color ]
           }
         }
 
@@ -4902,14 +4902,14 @@
         Method [ <internal:imagick> public method getColorValue ] {
 
           - Parameters [1] {
-            Parameter #0 [ <optional> $color ]
+            Parameter #0 [ <required> $color ]
           }
         }
 
         Method [ <internal:imagick> public method getColorValueQuantum ] {
 
           - Parameters [1] {
-            Parameter #0 [ <optional> $color ]
+            Parameter #0 [ <required> $color ]
           }
         }
 
@@ -5058,7 +5058,7 @@
 
           - Parameters [2] {
             Parameter #0 [ <required> $scale ]
-            Parameter #1 [ <required> $normalize_kernel ]
+            Parameter #1 [ <optional> $normalize_kernel ]
           }
         }
 

So everything seems ok (for optional/mandatory param)

@Danack
Copy link
Collaborator

Danack commented Jun 16, 2021

So, ImagickKernel::scale was fixed in a27492e

So, assuming the CI completes, does it all look good to you for an RC2?

@Danack Danack closed this as completed Jun 16, 2021
@remicollet
Copy link
Contributor Author

So, assuming the CI completes, does it all look good to you for an RC2?

Run a few test build (not the full matrix, only some)

  • Fedora 32, 33, 34 and RHEL 7, 8
  • PHP 5.4, 5.5, 5.6, 7.0, 7.1, 7.2, 7.3, 7.4, 8.0, 8.1
  • ImageMagick 6.9.12-15 and 7.1.0-0
  • NTS or ZTS

All build OK and test suite OK

So good to release :)

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

No branches or pull requests

2 participants