kjdev / php-ext-zstd

Zstd Extension for PHP
MIT License
201 stars 27 forks source link

Use Fast Parameter Parsing API #47

Closed JakubOnderka closed 1 year ago

JakubOnderka commented 1 year ago

Also:

kjdev commented 1 year ago

Please fix the build error in the lower version.

 cc -I. -I/__w/php-ext-zstd/php-ext-zstd -DPHP_ATOM_INC -I/__w/php-ext-zstd/php-ext-zstd/include -I/__w/php-ext-zstd/php-ext-zstd/main -I/__w/php-ext-zstd/php-ext-zstd -I/usr/local/include/php -I/usr/local/include/php/main -I/usr/local/include/php/TSRM -I/usr/local/include/php/Zend -I/usr/local/include/php/ext -I/usr/local/include/php/ext/date/lib -I/__w/php-ext-zstd/php-ext-zstd/zstd/lib/common -I/__w/php-ext-zstd/php-ext-zstd/zstd/lib -DHAVE_CONFIG_H -g -O2 -c /__w/php-ext-zstd/php-ext-zstd/zstd.c  -fPIC -DPIC -o .libs/zstd.o
/__w/php-ext-zstd/php-ext-zstd/zstd.c:82:1: error: unknown type name 'bool'
   82 | bool zstd_check_compress_level(long level)
      | ^~~~
/__w/php-ext-zstd/php-ext-zstd/zstd.c: In function 'zstd_check_compress_level':
/__w/php-ext-zstd/php-ext-zstd/zstd.c:90:14: error: 'false' undeclared (first use in this function)
   90 |       return false;
      |              ^~~~~
/__w/php-ext-zstd/php-ext-zstd/zstd.c:90:14: note: each undeclared identifier is reported only once for each function it appears in
/__w/php-ext-zstd/php-ext-zstd/zstd.c:99:12: error: 'true' undeclared (first use in this function)
   99 |     return true;
      |            ^~~~
/__w/php-ext-zstd/php-ext-zstd/zstd.c: In function 'zif_zstd_compress':
/__w/php-ext-zstd/php-ext-zstd/zstd.c:145:9: warning: implicit declaration of function 'zend_string_efree'; did you mean 'zend_string_free'? [-Wimplicit-function-declaration]
  145 |         zend_string_efree(output);
      |         ^~~~~~~~~~~~~~~~~
      |         zend_string_free
make: *** [Makefile:194: zstd.lo] Error 1
JakubOnderka commented 1 year ago

@kjdev This fixes support for PHP7. Do you still want to support PHP5? These versions are not supported for more than 3 years and for me doesn't make sense to make code more complicated or slower to support these outdated versions.

kjdev commented 1 year ago

PHP 5 is not supported. Please correct the rebase and other modifications.

We would still like to support PHP7, but do you think we should separate the sources?

ex:

zstd.c (PHP 8.x+)
php7/zstd.c (PHP 7.x)
JakubOnderka commented 1 year ago

@kjdev Rebased. And I think currently there is no reason to use different file for PHP7 and PHP8 as there is not much changes in API.

kjdev commented 1 year ago

Please correct the conflict. Please change the specifications according to the version.

JakubOnderka commented 1 year ago

Rebased.

Please change the specifications according to the version.

What do you mean change the specification?

kjdev commented 1 year ago

Return value is changed from boolean to NULL.

JakubOnderka commented 1 year ago

I think specification didn't change. For example check strlen method. According to specification it always returns int, but if you do not provide argument (like if you just call strlen()) or you provide invalid argument (like strlen([])), return value is null. https://3v4l.org/thJr0

kjdev commented 1 year ago

https://github.com/kjdev/php-ext-zstd/pull/47/files#diff-65367e2946c3afaf7cf180fe3ca6881e665942cc5bc93047443e41f1d1dc1bac

Why are you rewriting it in the test?

JakubOnderka commented 1 year ago

Yes, the behavior for invalid argument is changed to match behavior with other PHP functions. But I think there is no need to change specification, as it is standard behavior for PHP functions and they also do not specify that in function definition.

For PHP8 we can change the behavior to throw exception for invalid arguments: see https://www.php.net/releases/8.0/en.php#consistent-type-errors-for-internal-functions

kjdev commented 1 year ago

We believe that the test will be changed to a different specification because it will no longer be compatible with the previous one. (PHP 7)

JakubOnderka commented 1 year ago

It is still compatible, as according to PHP documentation this is undefined behavior:

If the parameters given to a function are not what it expects, such as passing an array where a string is expected, the return value of the function is undefined. In this case it will likely return null but this is just a convention, and cannot be relied upon. As of PHP 8.0.0, a TypeError exception is supposed to be thrown in this case.

But I understand that somebody could really on that, but I think it will be enough to describe that change in CHANGELOG.

kjdev commented 1 year ago

Modifications to this Pull-Request will be captured separately.