fix: returns a zend_array directly in types.go #1894

AlliBalliBaba · 2025-09-22T19:44:49Z

Should fix #1891.

Changes the PHP* array functions for extensions to return a zend_array instead of a zval.

In most cases it's easier to work with the zend_array and in the current setup the zval container sometimes needs pinning.

This PR also adds some wrapper functions so tests still work and some #cgnocallback and #cgonoescape optimizations

dunglas · 2025-09-23T06:33:45Z

Why doesn't it need pinning? As long as it is heap-allocated, it needs pinning.

AlliBalliBaba · 2025-09-23T07:02:23Z

I think right now it's actually allocated on the stack, which works for go -> c, but doesn't work for c -> go -> c.

We could also explicitly allocate the zval instead, but it might end up being an unnecessary allocation.

dunglas · 2025-09-23T07:42:54Z

Couldn't we just expose the pinner instead?

AlliBalliBaba · 2025-09-24T15:01:17Z

Hmm yeah we could, not sure though if that's a good idea.

I still think the best way is to just return the zend_array since most of the time the flow probably will include some kind of custom PHP function like this:

PHP_FUNCTION(my_function)
{
    HashTable *ht =go_get_an_array()
    
    RETURN_ARR(ht)
}

For tests we can just wrap the array into a zval.

dunglas · 2025-09-24T20:46:22Z

I agree. In general, we should return the specific data structure instead of a zval when possible.

AlliBalliBaba · 2025-09-25T17:15:21Z

Let's stick with the Hashtable then 👍 . Only unfortunate thing is the asymmetry between PHPArray and GoArray since one takes a zval and the other returns a HashTable. But it's probably the most convenient for this type of flow:

php function -> go -> return from php function.

dunglas · 2025-09-25T17:17:20Z

Shouldn't we remove zvals from our public API?

AlliBalliBaba · 2025-09-29T15:05:36Z

I think that makes sense since we have to be strict anyways about what types we are passing. The go functions will now also only accept a HashTable.

Only thing I'm not sure about is where to hint this in the docs or extensions generator, maybe @alexandre-daubois can help out. Basically it now needs an additional call to Z_ARRVAL_P on the C side:

PHP_FUNCTION(my_function)
{
    zval *input;

    ZEND_PARSE_PARAMETERS_START(1, 1)
        Z_PARAM_ARRAY(input)
    ZEND_PARSE_PARAMETERS_END();

    RETURN_ARR(go_process_the_array(Z_ARRVAL_P(input)));
}

alexandre-daubois · 2025-10-02T13:56:32Z

I guess this should do the trick?

diff --git a/internal/extgen/templates/extension.c.tpl b/internal/extgen/templates/extension.c.tpl
index 6ae0b34..fdd61a1 100644
--- a/internal/extgen/templates/extension.c.tpl
+++ b/internal/extgen/templates/extension.c.tpl
@@ -113,19 +113,19 @@ PHP_METHOD({{namespacedClassName $.Namespace .ClassName}}, {{.PhpName}}) {
     
     {{- if ne .ReturnType "void"}}
     {{- if eq .ReturnType "string"}}
-    zend_string* result = {{.Name}}_wrapper(intern->go_handle{{if .Params}}{{range .Params}}, {{if .IsNullable}}{{if eq .PhpType "string"}}{{.Name}}_is_null ? NULL : {{.Name}}{{else if eq .PhpType "int"}}{{.Name}}_is_null ? NULL : &{{.Name}}{{else if eq .PhpType "float"}}{{.Name}}_is_null ? NULL : &{{.Name}}{{else if eq .PhpType "bool"}}{{.Name}}_is_null ? NULL : &{{.Name}}{{else if eq .PhpType "array"}}{{.Name}}{{end}}{{else}}{{.Name}}{{end}}{{end}}{{end}});
+    zend_string* result = {{.Name}}_wrapper(intern->go_handle{{if .Params}}{{range .Params}}, {{if .IsNullable}}{{if eq .PhpType "string"}}{{.Name}}_is_null ? NULL : {{.Name}}{{else if eq .PhpType "int"}}{{.Name}}_is_null ? NULL : &{{.Name}}{{else if eq .PhpType "float"}}{{.Name}}_is_null ? NULL : &{{.Name}}{{else if eq .PhpType "bool"}}{{.Name}}_is_null ? NULL : &{{.Name}}{{else if eq .PhpType "array"}}{{.Name}} ? Z_ARRVAL_P({{.Name}}) : NULL{{end}}{{else}}{{if eq .PhpType "array"}}Z_ARRVAL_P({{.Name}}){{else}}{{.Name}}{{end}}{{end}}{{end}}{{end}});
     RETURN_STR(result);
     {{- else if eq .ReturnType "int"}}
-    zend_long result = {{.Name}}_wrapper(intern->go_handle{{if .Params}}{{range .Params}}, {{if .IsNullable}}{{if eq .PhpType "string"}}{{.Name}}_is_null ? NULL : {{.Name}}{{else if eq .PhpType "int"}}{{.Name}}_is_null ? NULL : &{{.Name}}{{else if eq .PhpType "float"}}{{.Name}}_is_null ? NULL : &{{.Name}}{{else if eq .PhpType "bool"}}{{.Name}}_is_null ? NULL : &{{.Name}}{{else if eq .PhpType "array"}}{{.Name}}{{end}}{{else}}{{if eq .PhpType "array"}}{{.Name}}{{else}}(long){{.Name}}{{end}}{{end}}{{end}}{{end}});
+    zend_long result = {{.Name}}_wrapper(intern->go_handle{{if .Params}}{{range .Params}}, {{if .IsNullable}}{{if eq .PhpType "string"}}{{.Name}}_is_null ? NULL : {{.Name}}{{else if eq .PhpType "int"}}{{.Name}}_is_null ? NULL : &{{.Name}}{{else if eq .PhpType "float"}}{{.Name}}_is_null ? NULL : &{{.Name}}{{else if eq .PhpType "bool"}}{{.Name}}_is_null ? NULL : &{{.Name}}{{else if eq .PhpType "array"}}{{.Name}} ? Z_ARRVAL_P({{.Name}}) : NULL{{end}}{{else}}{{if eq .PhpType "array"}}Z_ARRVAL_P({{.Name}}){{else}}(long){{.Name}}{{end}}{{end}}{{end}}{{end}});
     RETURN_LONG(result);
     {{- else if eq .ReturnType "float"}}
-    double result = {{.Name}}_wrapper(intern->go_handle{{if .Params}}{{range .Params}}, {{if .IsNullable}}{{if eq .PhpType "string"}}{{.Name}}_is_null ? NULL : {{.Name}}{{else if eq .PhpType "int"}}{{.Name}}_is_null ? NULL : &{{.Name}}{{else if eq .PhpType "float"}}{{.Name}}_is_null ? NULL : &{{.Name}}{{else if eq .PhpType "bool"}}{{.Name}}_is_null ? NULL : &{{.Name}}{{else if eq .PhpType "array"}}{{.Name}}{{end}}{{else}}{{if eq .PhpType "array"}}{{.Name}}{{else}}(double){{.Name}}{{end}}{{end}}{{end}}{{end}});
+    double result = {{.Name}}_wrapper(intern->go_handle{{if .Params}}{{range .Params}}, {{if .IsNullable}}{{if eq .PhpType "string"}}{{.Name}}_is_null ? NULL : {{.Name}}{{else if eq .PhpType "int"}}{{.Name}}_is_null ? NULL : &{{.Name}}{{else if eq .PhpType "float"}}{{.Name}}_is_null ? NULL : &{{.Name}}{{else if eq .PhpType "bool"}}{{.Name}}_is_null ? NULL : &{{.Name}}{{else if eq .PhpType "array"}}{{.Name}} ? Z_ARRVAL_P({{.Name}}) : NULL{{end}}{{else}}{{if eq .PhpType "array"}}Z_ARRVAL_P({{.Name}}){{else}}(double){{.Name}}{{end}}{{end}}{{end}}{{end}});
     RETURN_DOUBLE(result);
     {{- else if eq .ReturnType "bool"}}
-    int result = {{.Name}}_wrapper(intern->go_handle{{if .Params}}{{range .Params}}, {{if .IsNullable}}{{if eq .PhpType "string"}}{{.Name}}_is_null ? NULL : {{.Name}}{{else if eq .PhpType "int"}}{{.Name}}_is_null ? NULL : &{{.Name}}{{else if eq .PhpType "float"}}{{.Name}}_is_null ? NULL : &{{.Name}}{{else if eq .PhpType "bool"}}{{.Name}}_is_null ? NULL : &{{.Name}}{{else if eq .PhpType "array"}}{{.Name}}{{end}}{{else}}{{if eq .PhpType "array"}}{{.Name}}{{else}}(int){{.Name}}{{end}}{{end}}{{end}}{{end}});
+    int result = {{.Name}}_wrapper(intern->go_handle{{if .Params}}{{range .Params}}, {{if .IsNullable}}{{if eq .PhpType "string"}}{{.Name}}_is_null ? NULL : {{.Name}}{{else if eq .PhpType "int"}}{{.Name}}_is_null ? NULL : &{{.Name}}{{else if eq .PhpType "float"}}{{.Name}}_is_null ? NULL : &{{.Name}}{{else if eq .PhpType "bool"}}{{.Name}}_is_null ? NULL : &{{.Name}}{{else if eq .PhpType "array"}}{{.Name}} ? Z_ARRVAL_P({{.Name}}) : NULL{{end}}{{else}}{{if eq .PhpType "array"}}Z_ARRVAL_P({{.Name}}){{else}}(int){{.Name}}{{end}}{{end}}{{end}}{{end}});
     RETURN_BOOL(result);
     {{- else if eq .ReturnType "array"}}
-    void* result = {{.Name}}_wrapper(intern->go_handle{{if .Params}}{{range .Params}}, {{if .IsNullable}}{{if eq .PhpType "string"}}{{.Name}}_is_null ? NULL : {{.Name}}{{else if eq .PhpType "int"}}{{.Name}}_is_null ? NULL : &{{.Name}}{{else if eq .PhpType "float"}}{{.Name}}_is_null ? NULL : &{{.Name}}{{else if eq .PhpType "bool"}}{{.Name}}_is_null ? NULL : &{{.Name}}{{else if eq .PhpType "array"}}{{.Name}}{{end}}{{else}}{{.Name}}{{end}}{{end}}{{end}});
+    void* result = {{.Name}}_wrapper(intern->go_handle{{if .Params}}{{range .Params}}, {{if .IsNullable}}{{if eq .PhpType "string"}}{{.Name}}_is_null ? NULL : {{.Name}}{{else if eq .PhpType "int"}}{{.Name}}_is_null ? NULL : &{{.Name}}{{else if eq .PhpType "float"}}{{.Name}}_is_null ? NULL : &{{.Name}}{{else if eq .PhpType "bool"}}{{.Name}}_is_null ? NULL : &{{.Name}}{{else if eq .PhpType "array"}}{{.Name}} ? Z_ARRVAL_P({{.Name}}) : NULL{{end}}{{else}}{{if eq .PhpType "array"}}Z_ARRVAL_P({{.Name}}){{else}}{{.Name}}{{end}}{{end}}{{end}}{{end}});
     if (result != NULL) {
         HashTable *ht = (HashTable*)result;
         RETURN_ARR(ht);
@@ -134,7 +134,7 @@ PHP_METHOD({{namespacedClassName $.Namespace .ClassName}}, {{.PhpName}}) {
     }
     {{- end}}
     {{- else}}
-    {{.Name}}_wrapper(intern->go_handle{{if .Params}}{{range .Params}}, {{if .IsNullable}}{{if eq .PhpType "string"}}{{.Name}}_is_null ? NULL : {{.Name}}{{else if eq .PhpType "int"}}{{.Name}}_is_null ? NULL : &{{.Name}}{{else if eq .PhpType "float"}}{{.Name}}_is_null ? NULL : &{{.Name}}{{else if eq .PhpType "bool"}}{{.Name}}_is_null ? NULL : &{{.Name}}{{else if eq .PhpType "array"}}{{.Name}}{{end}}{{else}}{{if eq .PhpType "string"}}{{.Name}}{{else if eq .PhpType "int"}}(long){{.Name}}{{else if eq .PhpType "float"}}(double){{.Name}}{{else if eq .PhpType "bool"}}(int){{.Name}}{{else if eq .PhpType "array"}}{{.Name}}{{end}}{{end}}{{end}}{{end}});
+    {{.Name}}_wrapper(intern->go_handle{{if .Params}}{{range .Params}}, {{if .IsNullable}}{{if eq .PhpType "string"}}{{.Name}}_is_null ? NULL : {{.Name}}{{else if eq .PhpType "int"}}{{.Name}}_is_null ? NULL : &{{.Name}}{{else if eq .PhpType "float"}}{{.Name}}_is_null ? NULL : &{{.Name}}{{else if eq .PhpType "bool"}}{{.Name}}_is_null ? NULL : &{{.Name}}{{else if eq .PhpType "array"}}{{.Name}} ? Z_ARRVAL_P({{.Name}}) : NULL{{end}}{{else}}{{if eq .PhpType "string"}}{{.Name}}{{else if eq .PhpType "int"}}(long){{.Name}}{{else if eq .PhpType "float"}}(double){{.Name}}{{else if eq .PhpType "bool"}}(int){{.Name}}{{else if eq .PhpType "array"}}Z_ARRVAL_P({{.Name}}){{end}}{{end}}{{end}}{{end}});
     {{- end}}
 }
 {{end}}{{end}}

dunglas · 2025-10-07T14:55:18Z

Hey, I would like to make a release soon. Do you think you can include @alexandre-daubois changes @AlliBalliBaba?

AlliBalliBaba · 2025-10-07T16:37:24Z

Hey yeah, I'll have a look 👍

dunglas · 2025-10-09T09:40:39Z

I'm wondering if this change is actually a good idea. The zend_array itself may contain zvals and may require pining. Aren't we just hiding the problem instead of fixing it?

docs/extensions.md

AlliBalliBaba · 2025-10-11T17:44:53Z

The zend_array has its own buckets that are allocated together with the array, so no pinning needed in that case. When converting with phpValue, the zval only shortly lives on the go stack before going into the array bucket.

There's probably still a lot of room for improvement on these conversions, but they should currently be safe to do. The main issue was that our tests only test the go->c->go direction and not the c->go->c direction. I'm thinking about also supporting objects, then we could probably use and test this together with some of the new messaging apis.

AlliBalliBaba · 2025-10-11T20:31:54Z

Alright, it's now zend_array everywhere. I also noticed that empty strings can currently segfault (they are converted to NULL), so I added a __zval_empty_string__ fallback.

dunglas

LGTM except for these minor comments changes.

docs/extensions.md

types.go

AlliBalliBaba · 2025-10-24T09:40:36Z

Fixed the merge conflicts. types.go still has a lot of inefficiencies and potential memory leaks.

PHPValue should probably accept a *zval pointer to avoid stack overflows and memory pinning. I plan to optimize and fix this in a separate PR. I've also been able to add Objects support on a separate branch, which now has too many merge conflicts.

One issue is also that we cannot properly test c->go->c convertions, which requires a built-in messaging api like in #1883

AlliBalliBaba · 2025-11-01T17:11:46Z

@dunglas can we merge this?

The only alternative that I could think of would be passing the zval container directly since that's often how the control-flow in php-src works. Hard to say what would be more convenient for extensions.

func PHPPackedArray[T any](slice []T) unsafe.Pointer {
	// current behaviour: return an unsafe.Pointer zend_array
}

func PHPPackedArray[T any](slice []T, zval unsafe.Pointer) {
	// alternative: put the array directly into the zval instead of returning it
}

dunglas · 2025-11-01T21:50:19Z

LGTM as is

AlliBalliBaba · 2025-11-14T14:16:33Z

@dunglas merging is still blocked

myleshyson · 2025-11-14T21:45:56Z

I think the extension.go.tpl needs to be updated as well. just tried to to use this fork to test the array fixes and noticed my function wasn't getting exported when running extension-init.

looks like there just needs to be a case added for C.zend_array

AlliBalliBaba · 2025-11-16T12:42:38Z

I'll try to add a test 👍

@francislavoie do you know why the testext tests segafult when running go test ./... from the root directory? Is it a build issue?

Hmm also currently when initializing an extension with frankenphp extension-init it will just delete the whole folder including the go file

AlliBalliBaba · 2025-11-16T13:30:55Z

@myleshyson does it work for you now?

AlliBalliBaba · 2025-11-16T14:12:52Z

~~@alexandre-daubois actually I don't think the extgen command should be removing the BuildDir at all~~

frankenphp/caddy/extinit.go

Line 39 in 4e6d67e

    
           generator := extgen.Generator{BaseName: baseName, SourceFile: sourceFile, BuildDir: filepath.Dir(sourceFile)}

~~Has the potential to just nuke a whole project without warning.~~

Oh nvm, I just saw that this was already fixed on main.

myleshyson · 2025-11-17T04:08:17Z

maybe its related to this but I'm having a hard time testing. im running this in a docker container. And funny enough this actually did nuke my whole ext project!

Steps to reproduce:

have a volume of your ext project in a docker container.
try to run (using this branch) frankenphp ext-init /path/to/your/project inside the docker container.

I got this error, and noticed my project was deleted. its a git repo so I just restored it.

dont't have this issue with 1.9.1.

root@3834af4d486f:/app# frankenphp extension-init ./sdphp/stringext.go
Error: setup build directory: removing build directory: unlinkat ./sdphp: device or resource busy
root@3834af4d486f:/app# exit
exit

~~@alexandre-daubois actually I don't think the extgen command should be removing the BuildDir at all~~

frankenphp/caddy/extinit.go

Line 39 in 4e6d67e

generator := extgen.Generator{BaseName: baseName, SourceFile: sourceFile, BuildDir: filepath.Dir(sourceFile)}

~~Has the potential to just nuke a whole project without warning.~~

Oh nvm, I just saw that this was already fixed on main.

alexandre-daubois · 2025-11-17T08:12:28Z

@myleshyson Yes there was a bug after the removal of the build dir, it's fixed on main. I stumbled across that too, and it wasn't a Git repository in my case 😅 The fix has been pushed last week, sorry for the inconvenience!

AlliBalliBaba · 2025-11-17T18:00:00Z

I merged main into this branch so you can try again @myleshyson

types_test.go

AlliBalliBaba added 3 commits September 22, 2025 21:37

Returns a zend_array to PHP.

3744bf0

Adds cgo optimizations.

c87e4c9

linting.

fb9acec

AlliBalliBaba added 2 commits September 29, 2025 16:53

Makes go functions also return a hashtable.

cc6eae4

Adjusts types.

acfbe51

Applies git diff by @alexandre-daubois.

0bc8de4

dunglas reviewed Oct 9, 2025

View reviewed changes

docs/extensions.md Outdated Show resolved Hide resolved

Changes naming to zend_array.

bb32911

dunglas requested changes Oct 13, 2025

View reviewed changes

AlliBalliBaba added 3 commits October 13, 2025 22:38

Suggestions by @dunglas.

5f1bd59

Fixes merge conflicts.

e9d2294

Fixes toZval case.

a5125f5

AlliBalliBaba requested a review from dunglas November 2, 2025 18:09

dunglas mentioned this pull request Nov 14, 2025

feat(types): expose IsPacked to help dealing with hashmaps and lists in Go code #1977

Open

Adds correct array validation.

16c1168

Fixes tests.

1958288

AlliBalliBaba mentioned this pull request Nov 17, 2025

feat: add frankenphp_log_message() as a PHP function #1979

Open

Merge branch 'refs/heads/main' into fix/return-hasmaps-directly

c574b5d

dunglas reviewed Nov 18, 2025

View reviewed changes

types_test.go Show resolved Hide resolved

fix: returns a zend_array directly in types.go #1894

Are you sure you want to change the base?

fix: returns a zend_array directly in types.go #1894

Uh oh!

Conversation

AlliBalliBaba commented Sep 22, 2025

Uh oh!

dunglas commented Sep 23, 2025

Uh oh!

AlliBalliBaba commented Sep 23, 2025

Uh oh!

dunglas commented Sep 23, 2025

Uh oh!

AlliBalliBaba commented Sep 24, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dunglas commented Sep 24, 2025

Uh oh!

AlliBalliBaba commented Sep 25, 2025

Uh oh!

dunglas commented Sep 25, 2025

Uh oh!

AlliBalliBaba commented Sep 29, 2025

Uh oh!

alexandre-daubois commented Oct 2, 2025

Uh oh!

dunglas commented Oct 7, 2025

Uh oh!

AlliBalliBaba commented Oct 7, 2025

Uh oh!

dunglas commented Oct 9, 2025

Uh oh!

Uh oh!

AlliBalliBaba commented Oct 11, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

AlliBalliBaba commented Oct 11, 2025

Uh oh!

dunglas left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

AlliBalliBaba commented Oct 24, 2025

Uh oh!

AlliBalliBaba commented Nov 1, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dunglas commented Nov 1, 2025

Uh oh!

AlliBalliBaba commented Nov 14, 2025

Uh oh!

myleshyson commented Nov 14, 2025

Uh oh!

AlliBalliBaba commented Nov 16, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

AlliBalliBaba commented Nov 16, 2025

Uh oh!

AlliBalliBaba commented Nov 16, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

myleshyson commented Nov 17, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

alexandre-daubois commented Nov 17, 2025

Uh oh!

AlliBalliBaba commented Nov 17, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

AlliBalliBaba commented Sep 24, 2025 •

edited

Loading

AlliBalliBaba commented Oct 11, 2025 •

edited

Loading

AlliBalliBaba commented Nov 1, 2025 •

edited

Loading

AlliBalliBaba commented Nov 16, 2025 •

edited

Loading

AlliBalliBaba commented Nov 16, 2025 •

edited

Loading

myleshyson commented Nov 17, 2025 •

edited

Loading