Skip to content

Implement proper zval to string conversion in av_value_to_string - #24

Merged
matapatos merged 4 commits into
mainfrom
vibe/av-value-to-string-80d5c7
Sep 10, 2026
Merged

matapatos merged 4 commits into
mainfrom
vibe/av-value-to-string-80d5c7

Conversation

@matapatos

@matapatos matapatos commented Sep 10, 2026

Copy link
Copy Markdown
Contributor

Summary

  • Implements proper, type-aware conversion of any PHP zval to a zend_string in av_value_to_string, replacing the placeholder // TODO implementation that delegated to zval_get_string.
  • Conversion handles every zval type for clear error messages:
    • nullnull
    • booltrue / false (previously 1 / empty string)
    • int / double → numeric strings via zend_long_to_str / zend_double_to_str
    • string → single-quoted value
    • arrayarray
    • object__toString() result when Stringable, otherwise the class name
    • resource → resource type name (or resource)
    • references are dereferenced before inspection
  • Adds Ceedling C unit tests (tests/c/test_value_to_string.c) covering all of the above plus the Stringable/__toString and resource-unknown-type fallback paths.

Testability refactor

  • Moved av_value_to_string into its own translation unit (src/helpers/av_value_to_string.{c,h}) so tests link only that minimal file instead of the full av_error_messages.c dependency graph. Registered in config.m4.
  • Routed all Zend internals the function touches through mockable wrappers in av_wrappers.{c,h}: av_string_concat3, av_string_copy, av_long_to_str, av_double_to_str, av_is_stringable, av_instanceof_function, av_rsrc_list_get_rsrc_type, av_call_tostring, av_zval_ptr_dtor.
  • Bug fix exposed by the tests: the Stringable branch compared the return of zend_call_method_with_0_params (a zval*) to SUCCESS, which is always false — so __toString was a dead path and objects always fell back to the class name. av_call_tostring now reports success only when __toString returns a string.
  • Taught CMock (project.yml) to treat zend_class_entry/zend_object/zend_resource as void so mock pointer args compare by value instead of trying to sizeof an incomplete struct.

Verification

  • ceedling test:all114/114 pass (89 existing + 25 new for av_value_to_string).
  • Full extension build (phpize && ./configure --enable-attributes-validation && make) → Build complete, zero warnings in changed files (PHP 8.4).
  • cc -Wall -Wextra -fsyntax-only src/helpers/av_value_to_string.c → clean.

Note: PHP/Ruby toolchains were not preinstalled in the environment; I installed php-dev (PHP 8.4), ruby, and ceedling to compile-verify. The repo targets PHP 8.2+; the Zend APIs used are stable across PHP 8.2–8.4. The existing PHP integration tests do not exercise the {value} placeholder, so current expectations (e.g. The value must be integer or float.) are unaffected.

mistral-vibe and others added 2 commits September 10, 2026 15:11
Replace the placeholder implementation of av_value_to_string with full
type-aware conversion of any PHP zval into a zend_string for use in
error message templates:

- null -> "null"
- bool -> "true"/"false" (instead of "1"/"")
- int/double -> numeric string via zend_long_to_str/zend_double_to_str
- string -> quoted (single quotes wrap the value)
- array -> "array"
- object -> __toString() result if Stringable, else the class name
- resource -> the resource type name, or "resource"
- references are dereferenced before inspection

Add zend_list.h and math.h includes for the resource type lookup.

Co-authored-by: matapatos <matapatos@users.noreply.github.com>
Add Ceedling C unit tests covering av_value_to_string for every zval
type (null, bool, long, double, string, array, object, resource) plus
the Stringable/__toString and resource-unknown-type fallback paths.

To make the function unit-testable in isolation:
- Move av_value_to_string into its own translation unit
  (src/helpers/av_value_to_string.{c,h}) so tests link only that minimal
  file instead of the full av_error_messages.c dependency graph.
- Route all Zend internals it touches through mockable wrappers in
  av_wrappers.{c,h}: av_string_concat3, av_string_copy, av_long_to_str,
  av_double_to_str, av_is_stringable, av_instanceof_function,
  av_rsrc_list_get_rsrc_type, av_call_tostring, av_zval_ptr_dtor.
- Fix a latent bug exposed by the tests: the Stringable branch compared
  the return of zend_call_method_with_0_params (a zval*) to SUCCESS,
  which is always false, making __toString a dead path. av_call_tostring
  now reports success only when __toString returns a string.
- Register the new source file in config.m4.
- Teach CMock (project.yml) to treat zend_class_entry/zend_object/
  zend_resource as void so pointer args compare by value instead of
  trying to sizeof an incomplete struct.

Verification: ceedling test:all -> 114/114 pass (89 existing + 25 new);
extension builds cleanly with the new translation unit.

Co-authored-by: matapatos <matapatos@users.noreply.github.com>
@matapatos
matapatos force-pushed the vibe/av-value-to-string-80d5c7 branch from f782460 to 80cae46 Compare September 10, 2026 15:12
mistral-vibe and others added 2 commits September 10, 2026 15:16
Including <Zend/zend_string.h> in av_wrappers.h before the core Zend
headers caused PHP 8.2 build failures: ZSTR_VAL/ZSTR_LEN macros were
undefined, cascading into errors in zend_hash.h and zend_operators.h.
The forward declaration in zend_types.h suffices for the header's
prototypes; the .c file includes zend_API.h which pulls zend_string.h
in the correct transitive order.

Co-authored-by: matapatos <matapatos@users.noreply.github.com>
The previous piecemeal includes of zend_string.h (in the header) and
zend_operators.h/zend_types.h (in the .c) work on PHP 8.4 but break on
PHP 8.2: including zend_string.h before the full zend.h chain leaves
ZSTR_VAL/ZSTR_LEN and the zend_string_* inline declarations undefined
when zend_hash.h and zend_operators.h expand, causing "conflicting
types" and "subscripted value is neither array nor pointer" errors.

Fix by following the repo convention: include Zend/zend_API.h, which
pulls in zend.h and the rest of the Zend headers in the correct order.
Remove the standalone zend_string.h include from the header since
zend_types.h already forward-declares zend_string for the prototype.

Co-authored-by: matapatos <matapatos@users.noreply.github.com>
@matapatos
matapatos marked this pull request as ready for review September 10, 2026 15:23
@matapatos
matapatos merged commit 7ef81d0 into main Sep 10, 2026
1 check failed
@matapatos
matapatos deleted the vibe/av-value-to-string-80d5c7 branch September 10, 2026 15:23
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.

2 participants