3708: Add new code generator from AVRO to PHP#36
3708: Add new code generator from AVRO to PHP#36martin-augment wants to merge 3 commits intomainfrom
Conversation
WalkthroughThis pull request introduces a code generation feature for the Avro PHP library. It adds a new ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a new PHP code generator for Avro schemas, utilizing the nikic/php-parser library to generate PSR-compliant classes and enums. The implementation includes a main generator class, custom exception handling, and comprehensive unit tests. However, several critical issues were identified in the generator logic: a mismatch between filenames and class names that will break PSR-4 autoloading, a runtime error due to the use of a non-existent Node\ArrayItem class, and potential ParseError risks with indented heredocs. Additionally, the jsonSerialize implementation currently fails to correctly handle collections (arrays or maps) of enums, which will lead to invalid JSON output.
|
|
||
| PHP; | ||
|
|
||
| $filename = $path.'/'.ucwords($name).'.php'; |
There was a problem hiding this comment.
The filename is being generated using the Avro fullname ($name), but the class name inside the file is generated using the short name ($registeredSchema->name()). This mismatch will break PSR-4 autoloading. Additionally, ucwords on a fullname containing dots (e.g., com.example.User) will not produce a valid filename or directory structure as intended. You should use the short name for the filename to match the class name, especially since all classes are being placed in the same PHP namespace.
$filename = $path.'/'.ucwords($registeredSchema->name()).'.php';| $arrayItems[] = new Node\ArrayItem( | ||
| $this->buildJsonSerializeValue($field->type(), $field->name()), | ||
| new String_($field->name()) | ||
| ); |
There was a problem hiding this comment.
The class Node\ArrayItem does not exist in php-parser. Based on your other usages of the Node\Expr namespace in this file, this should be Node\Expr\ArrayItem. This will cause a runtime error when generating code for records with fields.
$arrayItems[] = new Node\Expr\ArrayItem(
$this->buildJsonSerializeValue($field->type(), $field->name()),
new String_($field->name())
);| $code = <<<PHP | ||
| <?php | ||
|
|
||
| declare(strict_types=1); | ||
|
|
||
| {$this->printer->prettyPrint([$node])} | ||
|
|
||
| PHP; |
There was a problem hiding this comment.
Using indented flexible heredocs with empty lines can be fragile. In PHP, every line within the heredoc must have at least the same indentation as the closing identifier. If the 'empty' lines (81, 83, 85) do not contain the exact number of spaces as the closing PHP; tag, a ParseError will occur. It is safer to use a non-indented heredoc or ensure the empty lines are properly padded, though the former is much more maintainable.
| private function buildJsonSerializeValue(AvroSchema $fieldType, string $fieldName): Node\Expr | ||
| { | ||
| $propertyFetch = new Node\Expr\PropertyFetch(new Node\Expr\Variable('this'), $fieldName); | ||
|
|
||
| if ($fieldType instanceof AvroEnumSchema) { | ||
| return new Node\Expr\PropertyFetch($propertyFetch, 'value'); | ||
| } | ||
|
|
||
| if ($fieldType instanceof AvroUnionSchema) { | ||
| $nonNullSchemas = array_values(array_filter( | ||
| $fieldType->schemas(), | ||
| static fn (AvroSchema $s): bool => !($s instanceof AvroPrimitiveSchema && AvroSchema::NULL_TYPE === $s->type()) | ||
| )); | ||
|
|
||
| if (1 === count($nonNullSchemas) && $nonNullSchemas[0] instanceof AvroEnumSchema) { | ||
| return new Node\Expr\NullsafePropertyFetch($propertyFetch, 'value'); | ||
| } | ||
| } | ||
|
|
||
| return $propertyFetch; | ||
| } |
There was a problem hiding this comment.
The jsonSerialize logic correctly handles single Enum fields by accessing the ->value property, but it does not handle collections (arrays or maps) of Enums. Since the generated Enums are Backed Enums and do not implement JsonSerializable, json_encode will not automatically convert them to their string values when they are nested in arrays or maps. This will result in invalid Avro JSON output for schemas containing collections of enums.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
lang/php/test/Generator/AvroCodeGeneratorTest.php (1)
30-37: Consider renaming$transpilerto$generatorfor consistency.The property name
$transpilerdoesn't match the class nameAvroCodeGenerator. This minor inconsistency could be addressed for clarity.Proposed fix
class AvroCodeGeneratorTest extends TestCase { - private AvroCodeGenerator $transpiler; + private AvroCodeGenerator $generator; public function setUp(): void { - $this->transpiler = new AvroCodeGenerator(); + $this->generator = new AvroCodeGenerator(); }Note: This would require updating all
$this->transpilerreferences to$this->generatorthroughout the test file.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lang/php/test/Generator/AvroCodeGeneratorTest.php` around lines 30 - 37, Rename the test property $transpiler to $generator in the AvroCodeGeneratorTest class and update all usages to maintain consistency with AvroCodeGenerator; specifically change the property declaration "private AvroCodeGenerator $transpiler;" to use $generator, update the setUp() method where $this->transpiler = new AvroCodeGenerator(); to assign to $this->generator, and replace every $this->transpiler reference in the file with $this->generator (ensure type hint remains AvroCodeGenerator).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@lang/php/lib/Generator/AvroCodeGenerator.php`:
- Around line 273-277: The enum generation uses strtoupper($value) to make
$caseName which can collide for symbols that differ only by case (e.g. "red" vs
"RED"); update the logic in AvroCodeGenerator (the loop building $caseName and
calling $enum->addStmt) to detect duplicate $caseName values before adding cases
and handle them deterministically—either throw a clear error listing the
conflicting symbols or disambiguate by appending a unique suffix/index or using
a safer transformation that preserves uniqueness (e.g., include original chars
or an incremental suffix); ensure the chosen approach prevents duplicate enum
cases and that the error/handling references the original $value strings so
generated code remains valid.
- Around line 88-89: The filename generation currently uses ucwords($name) where
$name is the fullname (e.g., "com.example.User"), which produces incorrect
filenames for namespaced schemas; update the code in AvroCodeGenerator.php to
use the schema's simple name (e.g., $registeredSchema->name()) when building
$filename so that $files[$filename] contains the correct file path (ucwords can
be removed or applied to the simple name if desired) and ensure the registry key
can remain the fullname in $name.
---
Nitpick comments:
In `@lang/php/test/Generator/AvroCodeGeneratorTest.php`:
- Around line 30-37: Rename the test property $transpiler to $generator in the
AvroCodeGeneratorTest class and update all usages to maintain consistency with
AvroCodeGenerator; specifically change the property declaration "private
AvroCodeGenerator $transpiler;" to use $generator, update the setUp() method
where $this->transpiler = new AvroCodeGenerator(); to assign to
$this->generator, and replace every $this->transpiler reference in the file with
$this->generator (ensure type hint remains AvroCodeGenerator).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: c728f618-1f35-4b95-8dd0-2b4249e52692
📒 Files selected for processing (7)
composer.jsonlang/php/lib/Generator/AvroCodeGenerator.phplang/php/lib/Generator/AvroCodeGeneratorException.phplang/php/lib/Schema/AvroEnumSchema.phplang/php/lib/Schema/AvroName.phplang/php/lib/Schema/AvroNamedSchema.phplang/php/test/Generator/AvroCodeGeneratorTest.php
| $filename = $path.'/'.ucwords($name).'.php'; | ||
| $files[$filename] = $code; |
There was a problem hiding this comment.
ucwords() may produce unexpected results for namespaced schema names.
ucwords($name) capitalizes the first letter of each word (space-delimited by default). For Avro fullnames like com.example.User, this produces Com.example.user.php instead of the expected User.php. The registry key is the fullname, but the file should use only the simple name.
Consider using $registeredSchema->name() (the simple name) instead of the fullname for the filename:
Proposed fix
- $filename = $path.'/'.ucwords($name).'.php';
+ $filename = $path.'/'.ucfirst($registeredSchema->name()).'.php';🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@lang/php/lib/Generator/AvroCodeGenerator.php` around lines 88 - 89, The
filename generation currently uses ucwords($name) where $name is the fullname
(e.g., "com.example.User"), which produces incorrect filenames for namespaced
schemas; update the code in AvroCodeGenerator.php to use the schema's simple
name (e.g., $registeredSchema->name()) when building $filename so that
$files[$filename] contains the correct file path (ucwords can be removed or
applied to the simple name if desired) and ensure the registry key can remain
the fullname in $name.
| foreach ($values as $value) { | ||
| $caseName = strtoupper($value); | ||
| $enum->addStmt( | ||
| $this->factory->enumCase($caseName)->setValue($value) | ||
| ); |
There was a problem hiding this comment.
Potential enum case name collision when symbols differ only by case.
strtoupper($value) will produce identical case names for symbols like ["red", "RED"] or ["Yes", "YES"], resulting in duplicate enum cases and a PHP parse error in the generated code.
Consider adding a check for collisions or preserving original casing:
Proposed fix - detect collisions
+ private function buildEnumCases(array $values): array
+ {
+ $caseNames = [];
+ $statements = [];
+ foreach ($values as $value) {
+ $caseName = strtoupper($value);
+ if (isset($caseNames[$caseName])) {
+ throw new AvroCodeGeneratorException(
+ sprintf('Enum case collision: "%s" and "%s" both map to "%s"', $caseNames[$caseName], $value, $caseName)
+ );
+ }
+ $caseNames[$caseName] = $value;
+ $statements[] = $this->factory->enumCase($caseName)->setValue($value);
+ }
+ return $statements;
+ }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@lang/php/lib/Generator/AvroCodeGenerator.php` around lines 273 - 277, The
enum generation uses strtoupper($value) to make $caseName which can collide for
symbols that differ only by case (e.g. "red" vs "RED"); update the logic in
AvroCodeGenerator (the loop building $caseName and calling $enum->addStmt) to
detect duplicate $caseName values before adding cases and handle them
deterministically—either throw a clear error listing the conflicting symbols or
disambiguate by appending a unique suffix/index or using a safer transformation
that preserves uniqueness (e.g., include original chars or an incremental
suffix); ensure the chosen approach prevents duplicate enum cases and that the
error/handling references the original $value strings so generated code remains
valid.
🤖 Augment PR SummarySummary: This PR introduces an Avro-to-PHP code generator for the PHP implementation. Changes:
Technical Notes: The generator returns a 🤖 Was this summary useful? React with 👍 or 👎 |
![augmentcode[bot]](https://avatars.githubusercontent.com/in/1027498?s=60&v=4){kind=small}
| private function avroPrimitiveTypeToPhp(AvroPrimitiveSchema $primitiveSchema): string | ||
| { | ||
| return match ($primitiveSchema->type()) { | ||
| AvroSchema::NULL_TYPE => 'null', |
There was a problem hiding this comment.
lang/php/lib/Generator/AvroCodeGenerator.php:299: Mapping Avro null to the standalone PHP type null will generate code that does not compile on PHP 8.1 (standalone null types are only allowed starting in PHP 8.2), which conflicts with the package’s php: ^8.1 requirement. This also means the record_with_all_primitive_types expectation currently codifies output that can’t be executed on PHP 8.1.
Severity: high
Other Locations
lang/php/test/Generator/AvroCodeGeneratorTest.php:593lang/php/test/Generator/AvroCodeGeneratorTest.php:601lang/php/test/Generator/AvroCodeGeneratorTest.php:612
🤖 Was this useful? React with 👍 or 👎, or 🚀 if it prevented an incident/outage.
|
|
||
| PHP; | ||
|
|
||
| $filename = $path.'/'.ucwords($name).'.php'; |
There was a problem hiding this comment.
lang/php/lib/Generator/AvroCodeGenerator.php:88: translate() uses the registry key ($name = schema fullname) to form the filename, while the generated class/enum name is based on the unqualified schema name(). For schemas with an Avro namespace, this can produce filenames containing dots / not matching the declared type name, which can break common autoloading expectations.
Severity: medium
🤖 Was this useful? React with 👍 or 👎, or 🚀 if it prevented an incident/outage.
| } | ||
|
|
||
| if ($field->hasDefaultValue()) { | ||
| $property->setDefault($this->buildDefault($field->defaultValue())); |
There was a problem hiding this comment.
lang/php/lib/Generator/AvroCodeGenerator.php:154: Default values are injected without considering the field schema, so defaults for non-scalar typed fields (notably enums and records) will likely emit invalid PHP (e.g., a string default for a property typed as a generated enum). This can cause a fatal error when the generated class is loaded.
Severity: high
Other Locations
lang/php/lib/Generator/AvroCodeGenerator.php:166
🤖 Was this useful? React with 👍 or 👎, or 🚀 if it prevented an incident/outage.
|
|
||
| foreach ($avroRecord->fields() as $field) { | ||
| $phpType = $this->avroTypeToPhp($field->type(), $phpNamespace); | ||
| $getter = $this->factory->method($field->name()) |
There was a problem hiding this comment.
lang/php/lib/Generator/AvroCodeGenerator.php:194: Generating property/method/parameter names directly from Avro field names can produce invalid PHP in edge cases (e.g., field name colliding with reserved keywords or with generated methods like jsonSerialize / __construct). When that happens, the generated file will fail to parse or will redeclare methods.
Severity: medium
Other Locations
lang/php/lib/Generator/AvroCodeGenerator.php:144lang/php/lib/Generator/AvroCodeGenerator.php:164lang/php/lib/Generator/AvroCodeGenerator.php:218
🤖 Was this useful? React with 👍 or 👎, or 🚀 if it prevented an incident/outage.
| $enum = $this->factory->enum($className)->setScalarType('string'); | ||
|
|
||
| foreach ($values as $value) { | ||
| $caseName = strtoupper($value); |
There was a problem hiding this comment.
lang/php/lib/Generator/AvroCodeGenerator.php:274: Enum case names are derived via strtoupper($value), which can collapse distinct Avro symbols that differ only by case (e.g., foo vs FOO) into duplicate PHP case identifiers. That would make the generated enum invalid.
Severity: medium
🤖 Was this useful? React with 👍 or 👎, or 🚀 if it prevented an incident/outage.
Code Review — PR #36: Add PHP Code Generator for Avro SchemasOverall this is a solid, well-structured addition with good test coverage. A few issues worth addressing before merge. Bugs1.
switch ($schema::class) {
case AvroRecordSchema::class:
// preferred
if ($schema instanceof AvroRecordSchema) { ... }2.
3. Enum symbol collision after $caseName = strtoupper($value);If an enum has symbols 4. Named type references fall through to When Avro resolves a schema reference by name (e.g. the second use of Design Concerns5. Stateful generator / non-reentrant
6. Triple field iteration in Fields are iterated three separate times: once for properties, once for the constructor, once for getters. Combining into a single pass would be cleaner and slightly more efficient (though not a correctness issue). 7. No namespace-aware path generation The Avro schema's own namespace (e.g. Minor / Style8. private function buildDefault(mixed $value): mixed
{
if (is_array($value)) {
return $this->factory->val($value);
}
return $value;
}PhpParser's 9. Each field's type is computed three times (property, constructor param, getter). Introduce a small helper or cache the result per field to avoid repetition. 10. The exception is thrown by Schema file changes (
|
| Severity | Issue |
|---|---|
| Bug | switch ($schema::class) won't handle subclasses |
| Bug | Enum symbol collision after strtoupper() |
| Design | Named type references may resolve to 'mixed' |
| Design | Stateful $registry — document or refactor |
| Minor | ucwords() → ucfirst() |
| Minor | Triple field iteration |
| Minor | Untested exception path |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 3 potential issues.
Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
|
|
||
| PHP; | ||
|
|
||
| $filename = $path.'/'.ucwords($name).'.php'; |
There was a problem hiding this comment.
Filename uses fullname instead of simple name
High Severity
The filename at line 88 is built from $name, which is the registry key — i.e., $schema->fullname(). For Avro schemas with a namespace (e.g., "namespace": "com.example"), fullname() returns "com.example.User", producing a filename like Com.example.User.php. Meanwhile, buildRecord and buildEnum derive the class name from $schema->name() (the simple name, "User"). This mismatch means the generated file won't match its class name, breaking autoloading and PSR-4 conventions for any namespaced Avro schema.
Additional Locations (1)
| $schema instanceof AvroArraySchema => 'list<'.$this->avroTypeToPhpDocInner($schema->items(), $phpNamespace).'>', | ||
| $schema instanceof AvroMapSchema => 'array<string, '.$this->avroTypeToPhpDocInner($schema->values(), $phpNamespace).'>', | ||
| $schema instanceof AvroRecordSchema, $schema instanceof AvroEnumSchema => '\\'.$phpNamespace.'\\'.ucwords($schema->name()), | ||
| $schema instanceof AvroUnionSchema => $this->unionToPhp($schema, $phpNamespace), |
There was a problem hiding this comment.
PHPDoc loses generic types for unions in nested contexts
Low Severity
avroTypeToPhpDocInner delegates to unionToPhp for union schemas, which in turn calls avroTypeToPhp — mapping arrays/maps to the bare 'array' string. This discards generic type information in PHPDoc. For example, a field typed as array<["null", {"type":"array","items":"int"}]> would produce list<null|array> instead of the correct list<null|list<int>>. The union branch here needs to call avroTypeToPhpDocInner on each member instead.
| $enum = $this->factory->enum($className)->setScalarType('string'); | ||
|
|
||
| foreach ($values as $value) { | ||
| $caseName = strtoupper($value); |
There was a problem hiding this comment.
Enum case names can collide with PHP reserved words
Medium Severity
strtoupper($value) on valid Avro enum symbols like "null", "true", "false", or "class" produces "NULL", "TRUE", "FALSE", "CLASS" — all reserved identifiers in PHP 8.1+ that cannot be used as enum case names. The generated PHP code would fail to compile for any Avro enum containing these common symbols.
2 participants
3708: To review by AI