PHP 8.0 support #56
PHP 8.0 support #56
Conversation
|
Hello, @Rastusik! Thank you for sending this PR! Really appreciate it, but want to ask your opinion first. I planned previously to merge #52 (even planned to review and test this PR on weekend) - it is very cool way to support multiple versions of PHP and more elegant, because code is auto-generated from the source, whereas previously it was painful to support new versions by looking at all structs, picking necessary parts, checking offsets and it is very error-prone way because wrong offset in struct easily leads to segmentation fault and memory corruption. I started with idea of keeping 0.x branches per PHP version and do cascade merges, but it was not easy. So, this is why I want to ask about your thoughts and what way would you like to follow? Can we do your changes in PHP on top of #52? And separate question about your request to be repo maintainer - I will be happy to see someone working on this repo, because now I do not have enough time for my OSS projects, thus additional hands can help this project to grow ) Just want to be informed about ideas/plans what will be merged and maybe review changes or share an opinion about significant parts. Looking forward to hear about your decision about applying #52 or moving forward with your PR. And confirm if you are ready to be a maintainer of this repo |
| @@ -5,15 +5,24 @@ typedef int64_t zend_long; | |||
| typedef uint64_t zend_ulong; | |||
| typedef int64_t zend_off_t; | |||
|
|
|||
| typedef unsigned char zend_bool; | |||
| typedef bool zend_bool; | |||
There was a problem hiding this comment.
I would like to suggest to proceed with #52 first, as touching manually header files is really what I would like to avoid at all.
There was a problem hiding this comment.
So, let's try not to touch manually header files at all - this is error-prone way and library should use something that can do this in automated way.
| @@ -368,23 +384,20 @@ typedef struct _zend_class_constant { | |||
| typedef struct _zend_internal_arg_info { | |||
| const char *name; | |||
| zend_type type; | |||
| zend_uchar pass_by_reference; | |||
| zend_bool is_variadic; | |||
| const char *default_value; | |||
There was a problem hiding this comment.
Change is not compatible between versions, unfortunately. Need to have header files per PHP version, platform and TS mode
There was a problem hiding this comment.
my approach was that there will be always only one PHP version supported. I wanted to do minimal effort and maintenance. No problem with multiple version support though, but the maintenance can become more tricky with this
| $object = Core::cast('zend_object *', $memory); | ||
|
|
||
| Core::call('zend_object_std_init', $object, $classType); | ||
| $object = Core::call('zend_objects_new', $classType); |
There was a problem hiding this comment.
Previously I decided not to go with zend_objects_new as it was not too low-level. I had previously ideas about creation of persistent PHP objects and emalloc inside zend_objects_new was restricting this. This is why you see this manual block of memory allocation and $persisitent flag, which is now unused.
Let's keep my original idea, it can help us in the feature to do some crazy stuff. Or we can have another method for this, which will call zend_objects_new directly.
There was a problem hiding this comment.
no problem with the original idea, it just wasn't documented so I just thought that the low level call is much simpler.Why do you need the PHP objects to be persistent btw? (tbh I still don't know what that means, I didn't read much about PHP internals, I just wanted to do some basic fixes with minimal effort :) )
|
Hello @lisachenko! thank you for your reply, I didn't expect it so soon :) Regarding #52 , I quite like this idea, I didn't know that it was possible to transform C code into XML structure. The solution is quite fancy. But TBH, I'm not sure if supporting multiple PHP versions at once is a good call, because it adds additional complexity to the whole project. There might be unnecessary branching of some parts of the PHP code, like this one: if (version_compare(PHP_VERSION, "8.0.0", ">=") && $classEntry->attributes !== null) {
$this->attributesTable = new HashTable(Core::addr($classEntry->attributes));
}It's not a biggie, but it might be in time. But this is not my decision and the PR is very cool. Not sure if it has all problems solved though, seems like I saw some todos and open questions. But that can be solved in time. I also understand your preference for the approach from #52 . If you ask me whether I can do my changes on top of #52 - sure, no problem, I can. Not sure if they will be needed, but I suppose yes, because some things were crucial for some tests an I haven't seen the changes in the PR. But it was not a lot of changes. Regarding the repo maintainer privileges/attitude - this was actually your idea, I didn't have such aspirations :) But since you wrote about not doing OSS anymore, I thought that at least I could do some basic maintenance, like support for PHP 8+. I'm using this library just for one feature in SwooleBundle, which I'm maintaining and it was quite cool to play with I feel like what you are describing now seems like a more active approach, and for that I already know that I won't be having any spare time left. Or maybe I will have some, but I have no idea how much and I don't want to commit to anything. I think this is less than you would expect or imagine, so there is no pressure regarding the admin privileges. However this plays out, I feel like I'll do the basic work with new php versions support. At least until it will be possible to change the final class flag in the core PHP Reflection class :) And maybe something on top, maybe not (I still consider this as a fun project :) ). But I still want to play more with OpenSwoole, because it gives more value to the company I work for. What do you think about this? Can you imagine some middle ground? Or will simple contributions be sufficient? |
I fixed the PHP structs file with support for PHP 8 and also fixed some bugs so all the former tests are working.
@lisachenko can I get the rights for merging this and make a release?
After that I would push code for PHP 8.1 and when 8.2 is out also for that.