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

use standard way to generate arginfo - discussion #423

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

Conversation

remicollet
Copy link
Contributor

@Danack I noticed you have tried to fix compatibility in shim_php7_to_php8.h

This PR is only a PoC, onlyfor 1 class (kernel)

  • rename stub and arginfo files to math upstream way, so any change on the stub file will regenerate the file during "make"
  • improve shim_php7_to_php8.h

Notice, this way seems cleaner but

  • PHP > 7.0: add type hinting for parameters (only for object in previous versions)
  • PHP > 7.2: add return type hinting

P.S. tested only with 5.6, 7.4 and 8.0

@remicollet
Copy link
Contributor Author

To be honest, I'm totally lost about the way you want to handle this, and your util/fixup_arginfo.php script (which I try to avoid in this one)

@remicollet
Copy link
Contributor Author

the second commit apply the same change for ImagePixel and move the logic for MAGICKCORE_HDRI_ENABLE out of util/fixup_arginfo.php

It also fix the arginfo for getColorValue / getColorValueQuantum (color is mandatory

Reflection diff:

--- old8	2021-06-15 17:32:08.632750651 +0200
+++ new8	2021-06-15 18:24:47.502921113 +0200
@@ -5603,7 +5603,7 @@
         Method [ <internal:imagick> public method getColorValue ] {
 
           - Parameters [1] {
-            Parameter #0 [ <optional> int $color = <default> ]
+            Parameter #0 [ <required> int $color ]
           }
           - Return [ int ]
         }
@@ -5611,7 +5611,7 @@
         Method [ <internal:imagick> public method getColorValueQuantum ] {
 
           - Parameters [1] {
-            Parameter #0 [ <optional> int $color = <default> ]
+            Parameter #0 [ <required> int $color ]
           }
           - Return [ int ]
         }

@Danack
Copy link
Collaborator

Danack commented Jun 16, 2021

The general idea is that I'd really like to have a single source of truth for which functions take/return int or float depending on the HDRI compile option, so that I can go generate the appropriate documentation from that, rather than having to edit that by hand.

Although it would certainly be better to have the arginfo regenerated automatically (and there should definitely be something that checks it hasn't forgotten to be regenerated, if nothing else), I don't think I want to be editing stuff like this:

#if PHP_VERSION_ID < 70200
#undef  ZEND_BEGIN_ARG_WITH_RETURN_TYPE_INFO_EX
#define ZEND_BEGIN_ARG_WITH_RETURN_TYPE_INFO_EX(name, return_reference, required_num_args, type, allow_null) \
        ZEND_BEGIN_ARG_INFO_EX(name, 0, return_reference, required_num_args)

#undef  ZEND_BEGIN_ARG_WITH_RETURN_OBJ_INFO_EX
#define ZEND_BEGIN_ARG_WITH_RETURN_OBJ_INFO_EX(name, return_reference, required_num_args, class_name, allow_null) \
        ZEND_BEGIN_ARG_INFO_EX(name, 0, return_reference, required_num_args)

myself.

so any change on the stub file will regenerate the file during "make"

One reason I didn't try doing that, is that it is my understanding that the stub generator doesn't run on the ancient versions of PHP that I stupidly still support. e.g.

Warning: Unsupported declare 'strict_types' in /usr/local/lib/php/build/gen_stub.php on line 2

And I didn't feel like investigating making the stub generator work on the versions it doesn't currently support.

To be honest, I'm totally lost about the way you want to handle this,

insert customary joke about going back to school to learn carpentry/plumbing.

I think what I'll do is:

  • add some more hacks for the ZEND_ARG_TYPE_INFO_WITH_DEFAULT_VALUE so that I can get the release out.
  • delay thinking about this until a later date. In particular after getting the CI to be faster, so that checking the result of it doesn't take as long.

@remicollet
Copy link
Contributor Author

I have open bug #424 for arginfo fix needed (some being fix here, but only for discussion PoC)

One reason I didn't try doing that, is that it is my understanding that the stub generator doesn't run on the ancient versions of PHP that I stupidly still support. e.g.

Yes, need to use PHP 8 only to generate the arginfo from the stub, but can be use with all versions
Various extensions already use this way

The main thing with this PoC is type hinting introduced for PHP 7

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.

2 participants