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

Add support for php7.4 #429

Merged

Conversation

gnat42
Copy link
Contributor

@gnat42 gnat42 commented Sep 12, 2019

Changes to match the internal changes for PHP 7.4

@gnat42
Copy link
Contributor Author

gnat42 commented Sep 16, 2019

There is one issue. Although the extension compiles, classes and their methods are not visible in PHP space. I've confirmed this with the Examples/CppClassesInPhp as well as my own module. I'm not sure why that is the case. I've added Php::Public to all .method() calls but since the class isn't there (as per get_declared_clases()) it doesn't really do anything.

@gnat42
Copy link
Contributor Author

gnat42 commented Sep 18, 2019

So it looks like some flags have changed, I'm not sure if my solution is the correct fix however I've seen other extensions doing the same with the ce_flags. Instead of assigning directly its bitwise assigned and then the classes are now visible. So either that is the fix or the ClassType constants need to be updated. There is so much different between 7.3 and 7.4 in zend_compile.h I wasn't sure what should be done.

@gnat42
Copy link
Contributor Author

gnat42 commented Sep 19, 2019

Looks like lots #define's for ZEND_ACC_PUBLIC etc values have changed in this commit: php/php-src@0fbd2e6#diff-bb1c0392a5de04576925c02a7c078dc1

So I'm guessing my |= (int)_type; is incorrect and what actually needs to happen is a change to classtype.h however when looking at zend_compile.h I can't 100% be sure that the numbers set there correspond to the old zend_compile.h. I think this is the last thing to solve for this to work with php 7.4.

Some insight would be great. Even comments in classtype.h on how the values were calculated or what constants they were derived from would be great.

@gnat42
Copy link
Contributor Author

gnat42 commented Sep 19, 2019

Ok, so I've dug a bit more and found the root cause of the issue.

In PHP 7.4 zend_API.c the function that registers classes there are a number of constants being or'd.

In PHP 7.3 zend_API.c has fewer options being set.

So perhaps my |= (int)_type; may be the appropriate solution. However its also possible that the ClassType values are updated to the combination of their value and the defaults in the zend_API.c

@iMbell
Copy link

iMbell commented Sep 24, 2019

mkdir -p shared/common mkdir -p shared/zend g++ -Wall -c -std=c++11 -fvisibility=hidden -DBUILDING_PHPCPP -Wno-write-strings -MD -g php-config --includes -fpic -o shared/zend/classimpl.o zend/classimpl.cpp zend/classimpl.cpp: In static member function ‘static int Php::ClassImpl::getClosure(zval*, zend_class_entry**, zend_function**, zend_object**)’: zend/classimpl.cpp:307:35: error: ‘zend_empty_string’ was not declared in this scope function->function_name = zend_empty_string; // should not be null, as this is free'ed by zend when doing exception handling ^ make: *** [shared/zend/classimpl.o] Error 1

why?

@gnat42
Copy link
Contributor Author

gnat42 commented Oct 1, 2019

I'm not sure why that is there, I've noticed it but haven't dug into it

@gnat42
Copy link
Contributor Author

gnat42 commented Oct 1, 2019

I have to say that for the most part this patch works except there's still an issue about the public/private/protected nature of the methods. I'm 90% sure that the ClassType.h file needs to be updated but because I don't actually know which flags were used to build it I don't really know how to fix this. Any insight would be appreciated ping @mvdwerve or @sjinks any thoughts on this?

@gnat42
Copy link
Contributor Author

gnat42 commented Oct 1, 2019

So I dug a bit more and it looks like I was confusing the class visibility with the method visibility. I need to look at modifiers.cpp I think.

@scorninpc
Copy link

I confirmed with last dev src

$ php -dextension=./test.so test.php 
PHP Warning:  Invalid access level for Class1::__construct() - access must be exactly one of public, protected or private in Unknown on line 0
PHP Warning:  Invalid access level for Class1::method1() - access must be exactly one of public, protected or private in Unknown on line 0
OK

$php -v
PHP 7.4.0-dev (cli) (built: Oct  2 2019 22:33:30) ( NTS )
Copyright (c) The PHP Group
Zend Engine v3.4.0-dev, Copyright (c) Zend Technologies

$
#include <phpcpp.h>

class Class1 : public Php::Base
{
	public:
		Class1() = default;
		~Class1() = default;
		
		void __construct() { printf("__construct\n"); }
		
		void method1() { printf("method1\n"); }
};

extern "C" {
	
    PHPCPP_EXPORT void *get_module() 
    {
        // Register
        static Php::Extension extension("test", "1.0");
        
        // Create class
		Php::Class<Class1> class1("Class1");
			class1.method<&Class1::__construct>("__construct");
            class1.method<&Class1::method1>("method1");

		extension.add(std::move(class1));
		
        // return
        return extension;
    }
}

I'm investigating why too. You got a nice direction. However, your changes works fine

@gnat42
Copy link
Contributor Author

gnat42 commented Oct 5, 2019

Hey @scorninpc yeah so I've re-pushed with my latest changes. It doesn't fix the issue but I think is required. I've based it off the difference of Zend/zend_compile.h in php from 7.3 to 7.4 branch.

https://pastebin.com/FXBPt7U1

This shows that PHP changed the values for public, protected, private etc. However with this change I still get the errors. I haven't tracked the code setting the actual php visibility flag for a class method but the error message you get on the console will take you to where it is issued in the php-cpp file. I haven't had time to finish looking at that.

Any additional insight/help on this would be awesome

@scorninpc
Copy link

Thank you to start it. I'v small time too, but i'm investigating the php-src. I'v see this change on constant of visibility types, changed, but nothing to.

I'll keep you alive

@danog
Copy link

danog commented Oct 29, 2019

The visibility modifiers for PHP 7.4 weren't applied because the PHP_VERSION_ID constant wasn't defined for files in the common target.
PR NobletSolutions#1 fixes the issue by using the appropriate include flags and including the php header files.

@gnat42
Copy link
Contributor Author

gnat42 commented Oct 29, 2019

@danog's patch indeed fixes the issue for me. I've incorporated it, squashed it and pushed here. Our CI now seems to work with PHP74 and the extension we wrote based off php-cpp... Merging this would be great with a new patch release.

@TheROPFather
Copy link

@gnat42 Trying to build this, however I receive many compilation errors.

g++ -Wall -c -std=c++11 -fvisibility=hidden -DBUILDING_PHPCPP -Wno-write-strings -MD -g 'php-config --includes' -fpic -o shared/zend/inivalue.o zend/inivalue.cpp In file included from zend/includes.h:120, from zend/inivalue.cpp:8: zend/callable.h: In constructor ‘Php::Callable::Callable(Php::ZendCallback, const char*, const Arguments&)’: zend/callable.h:56:18: error: ‘struct _zend_internal_arg_info’ has no member named ‘is_variadic’

I've attached a full build log here: https://pastebin.com/MGpWKVSd

PHP 7.4.0 (cli) (built: Nov 30 2019 10:43:49) ( NTS ) Copyright (c) The PHP Group Zend Engine v3.4.0, Copyright (c) Zend Technologies with Xdebug v2.8.1, Copyright (c) 2002-2019, by Derick Rethans
Any thoughts?

@gnat42
Copy link
Contributor Author

gnat42 commented Dec 9, 2019

I can't say I've ever seen an error like that in trying to get this to build against php 7.4.0.

I just rebuilt php-cpp against 7.4.0 without issues. I've pasted the patch I was using and the build logs:

Build Log: https://pastebin.com/0DzU9bLw
Patch: https://pastebin.com/jTMVmzCM

@scorninpc
Copy link

Works a lot fine, why no merge?

image

@danog
Copy link

danog commented Dec 10, 2019

Ping please merge

@DerAlSem
Copy link

DerAlSem commented Jan 5, 2020

Please, merge, we do need 7.4 support, your work is highly appreciated.

@stereomatchingkiss
Copy link

Is this project dead?

@scorninpc
Copy link

This project are dead by author, but people are making that work. I think someone need to create a new project to make that updated. I'm non-git user, so if anyone can to that better, i help

@jpuck
Copy link

jpuck commented Feb 1, 2020

This branch works very well thank you! 🙇

Perhaps a 7.4 build matrix should be added to the travis-ci.yml

@scorninpc
Copy link

The dev is away alot time. I'm mergeing branchs here https://github.com/scorninpc/PHP-CPP until the author appears

@yaroslavche
Copy link

The dev is away alot time. I'm mergeing branchs here https://github.com/scorninpc/PHP-CPP until the author appears

It is a pity that the author went into inactive, I hope he is doing well.

@scorninpc I've used the fork in the extension, and everything seems to be working fine.
https://travis-ci.com/yaroslavche/phptdlib/jobs/283203070

$ php --version
PHP 7.4.3-dev (cli) (built: Jan 23 2020 12:06:04) ( ZTS )

Could you check also #399 please?

@DerAlSem
Copy link

DerAlSem commented Feb 6, 2020

The dev is away alot time. I'm mergeing branchs here https://github.com/scorninpc/PHP-CPP until the author appears

Man, you're awesome. Rebuilt php-tdlib with your fork. Works like a charm.

@scorninpc
Copy link

The dev is away alot time. I'm mergeing branchs here https://github.com/scorninpc/PHP-CPP until the author appears

Man, you're awesome. Rebuilt php-tdlib with your fork. Works like a charm.

Thank you, but i'm only a "merger" hahhhah, thanks for all community. When main dev appear I send only one pull request

@EmielBruijntjes EmielBruijntjes merged commit 364c2c1 into CopernicaMarketingSoftware:master Feb 9, 2020
@mlocati
Copy link

mlocati commented Mar 2, 2020

@EmielBruijntjes Any plans for a new release with this patch applied?

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.