krakjoe / uopz

User Operations for Zend
Other
358 stars 47 forks source link

PHP 7.1 compatibility #105

Closed remicollet closed 5 years ago

remicollet commented 5 years ago

PHP Warning: PHP Startup: Unable to load dynamic library '/work/GIT/uopz/modules/uopz.so' - /work/GIT/uopz/modules/uopz.so: undefined symbol: zend_strpprintf in Unknown on line 0

remicollet commented 5 years ago

The zend_strpprintf is part of the public API since 7.2

In 7.2+ (Zend/zend.h) ZEND_API zend_string *zend_strpprintf(size_t max_len, const char *format, ...) ZEND_ATTRIBUTE_FORMAT(printf, 2, 3);

In 7.1: (Zend/zend_exceptions.h) zend_string *zend_strpprintf(size_t max_len, const char *format, ...) ZEND_ATTRIBUTE_FORMAT(printf, 2, 3);

krakjoe commented 5 years ago

do you see an easy fix for that, or shall I look ?

remicollet commented 5 years ago

I'm looking (ned to switch to zend_alter_ini_entry_chars)

remicollet commented 5 years ago

@krakjoe please check this one which seems suspicious to me

ZSTR_VAL(level)[ZSTR_LEN(level)] = '0';

krakjoe commented 5 years ago

It's correct, it's setting the last char to '0' to disable first pass ...

diff --git a/uopz.c b/uopz.c
index 94d6872..3d53719 100644
--- a/uopz.c
+++ b/uopz.c
@@ -120,18 +120,18 @@ static PHP_RINIT_FUNCTION(uopz)

        if (INI_INT("opcache.optimization_level") & (1<<0)) {
                /* must disable block pass 1 constant substitution */
+               char buf[12];
                zend_string *optimizer = zend_string_init(
                        ZEND_STRL("opcache.optimization_level"), 1);
-               zend_string *level = zend_strpprintf(0, 
-                       "0x%0X", (unsigned int) INI_INT("opcache.optimization_level"));
+               size_t len = php_sprintf(buf, "0x%0X", 
+                       (unsigned int) INI_INT("opcache.optimization_level"));

-               ZSTR_VAL(level)[ZSTR_LEN(level)] = '0';
+               buf[len-1] = '0';

-               zend_alter_ini_entry(optimizer, level, 
+               zend_alter_ini_entry_chars(optimizer, buf, len, 
                        ZEND_INI_SYSTEM, ZEND_INI_STAGE_ACTIVATE);

                zend_string_release(optimizer);
-               zend_string_release(level);
        }

        spl = zend_string_init(ZEND_STRL("RuntimeException"), 0);

This looks right to me, because php_sprintf returns len+term(1) ....

krakjoe commented 5 years ago

You missed disabling the pass ? probably just use as above for all versions ...

remicollet commented 5 years ago

ok, please review current code

krakjoe commented 5 years ago

Looks okay now, and added a test to be sure ... thanks

remicollet commented 5 years ago

BTW, buf[len-1] = '0'; clear 4 digit, not only the last one...

krakjoe commented 5 years ago

d4045636e0dfd67b3f74e2f240d22ec3b4abe7e6

Can you check why that test doesn't skip when opcache is disabled locally ? I must be missing something ...

BTW, buf[len-1] = '0'; clear 4 digit, not only the last one...

urm, not sure what you're thinking there ? sizeof('0') is one, how could it change more than one char ?

remicollet commented 5 years ago

this is a hex digit, so a nibble, so 4 binary digits.

krakjoe commented 5 years ago

https://travis-ci.org/krakjoe/uopz/jobs/486446808 should have 1 skipped test, right ?

krakjoe commented 5 years ago

oh oh ... I get it ... sorry, fix it with ~1 as you wanted ... sorry ... tired ...

EDIT: apparently strpprintf is available to all versions, if you fancy that instead ?

krakjoe commented 5 years ago

I fixed it, sorry for wasting your time .... still not sure about that skipping test ... any idea there ?

remicollet commented 5 years ago

@krakjoe the issue with skip, if that uopz break the die command...

$ php -n -d extension=modules/uopz.so -d uopz.disable=1  -r 'die("Killed\n"); echo "GRR\n";'
Killed

$ php -n -d extension=modules/uopz.so -d uopz.disable=0  -r 'die("Killed\n"); echo "GRR\n";'
GRR
krakjoe commented 5 years ago

I'm so broken today ... I ... I ... fixed it ...

remicollet commented 5 years ago

@krakjoe indeed, fixed

BTW, the skipif also use "die" ;)

krakjoe commented 5 years ago

Yeah but that one checks if uopz is loaded, and if it isn't, then the die will work ... unless I'm totally wrong again ? am I even making sense ?

remicollet commented 5 years ago

Indeed..., so going for the release now!

krakjoe commented 5 years ago

I'm still not seeing what I expect on travis, maybe extension_loaded is case sensitive ?

krakjoe commented 5 years ago

Seems extension_loaded doesn't work for opcache, no idea why, edit, got it Zend OPcache ....

krakjoe commented 5 years ago

okay okay, we gotta use ini_get, because of the way travis is setup ...

krakjoe commented 5 years ago

I quit trying to make the test skip, out of bravery ...

We'll fix the skip later on, it's not important ... I know the test is okay ...