3708: Add new code generator from AVRO to PHP - #43
Conversation
…o add-schema-to-php-generator
|
Warning Review limit reached
Next review available in: 45 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (11)
📒 Files selected for processing (25)
WalkthroughThe PHP implementation adds Avro record and enum code generation, a Symfony-based ✨ Finishing Touches🧪 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 PHP code generation capabilities to the Apache Avro PHP library, adding a CLI tool to generate PHP classes and backed enums from Avro schemas, along with a specific datum writer (AvroSpecificDatumWriter) to serialize them. The review feedback highlights critical improvements: ensuring PHP 8.1 compatibility by omitting standalone null type hints (which cause fatal syntax errors in PHP 8.1), making generated enums implement \JsonSerializable to support nested serialization, optimizing performance by replacing ReflectionClass with get_class(), and correcting directory creation permissions to 0755 for proper traversal.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| private function buildEnum( | ||
| AvroEnumSchema $avroEnum, | ||
| string $phpNamespace, | ||
| array $values | ||
| ): Node { | ||
| $className = $this->classNameForSchema($avroEnum); | ||
| $enum = $this->factory->enum($className)->setScalarType('string'); | ||
|
|
||
| foreach ($values as $value) { | ||
| $caseName = strtoupper($value); | ||
| $enum->addStmt( | ||
| $this->factory->enumCase($caseName)->setValue($value) | ||
| ); | ||
| } | ||
|
|
||
| return $this->factory->namespace($this->namespaceForSchema($avroEnum, $phpNamespace)) | ||
| ->addStmt($enum) | ||
| ->getNode(); | ||
| } |
There was a problem hiding this comment.
By default, PHP backed enums do not implement \JsonSerializable. If a generated enum is nested inside an array, map, or a complex union, calling json_encode() on the parent record will serialize the enum as an empty object {} instead of its string value. Making the generated enum implement \JsonSerializable resolves this issue robustly across all nested contexts.
private function buildEnum(
AvroEnumSchema $avroEnum,
string $phpNamespace,
array $values
): Node {
$className = $this->classNameForSchema($avroEnum);
$enum = $this->factory->enum($className)
->setScalarType('string')
->implement('\\JsonSerializable');
foreach ($values as $value) {
$caseName = strtoupper($value);
$enum->addStmt(
$this->factory->enumCase($caseName)->setValue($value)
);
}
$jsonSerialize = $this->factory->method('jsonSerialize')
->makePublic()
->setReturnType('string')
->addStmt(
new Stmt\Return_(
new Node\Expr\PropertyFetch(new Node\Expr\Variable('this'), 'value')
)
);
$enum->addStmt($jsonSerialize);
return $this->factory->namespace($this->namespaceForSchema($avroEnum, $phpNamespace))
->addStmt($enum)
->getNode();
}| $phpType = $this->avroTypeToPhp($field->type(), $phpNamespace); | ||
| $property = $this->factory->property($field->name()) | ||
| ->makePrivate() | ||
| ->setType($phpType); |
There was a problem hiding this comment.
In PHP 8.1 (the minimum required version), null is not supported as a standalone type hint (it was only introduced in PHP 8.2). Generating private null $nullField; will result in a fatal syntax error on PHP 8.1. We should omit the type hint when the type is standalone 'null'.
$phpType = $this->avroTypeToPhp($field->type(), $phpNamespace);
$property = $this->factory->property($field->name())
->makePrivate();
if ('null' !== $phpType) {
$property->setType($phpType);
}| $phpType = $this->avroTypeToPhp($field->type(), $phpNamespace); | ||
| $param = $this->factory->param($field->name())->setType($phpType); |
There was a problem hiding this comment.
Omit the standalone 'null' type hint for constructor parameters to maintain compatibility with PHP 8.1.
$phpType = $this->avroTypeToPhp($field->type(), $phpNamespace);
$param = $this->factory->param($field->name());
if ('null' !== $phpType) {
$param->setType($phpType);
}| $phpType = $this->avroTypeToPhp($field->type(), $phpNamespace); | ||
| $getter = $this->factory->method($field->name()) | ||
| ->makePublic() | ||
| ->setReturnType($phpType) |
There was a problem hiding this comment.
Omit the standalone 'null' return type hint for getter methods to maintain compatibility with PHP 8.1.
$phpType = $this->avroTypeToPhp($field->type(), $phpNamespace);
$getter = $this->factory->method($field->name())
->makePublic();
if ('null' !== $phpType) {
$getter->setReturnType($phpType);
}| return Command::FAILURE; | ||
| } | ||
|
|
||
| if (!is_dir($outputDir) && !mkdir($outputDir, 0744, true) && !is_dir($outputDir)) { |
There was a problem hiding this comment.
The directory permissions are set to 0744 which does not grant execute permissions (x) to group and others. Without execute permissions, non-owner users/processes will not be able to traverse (i.e., cd into or list files within) the generated directory. It is recommended to use 0755 for directory creation, which is also consistent with the permissions used on line 129.
if (!is_dir($outputDir) && !mkdir($outputDir, 0755, true) && !is_dir($outputDir)) {| private function classNameMatchesSchema(object $datum, string $schemaName): bool | ||
| { | ||
| $className = (new \ReflectionClass($datum))->getShortName(); | ||
|
|
||
| return 0 === strcasecmp($className, $schemaName); | ||
| } |
There was a problem hiding this comment.
Using new \ReflectionClass($datum) to retrieve the short class name on every match check is highly inefficient and can become a major performance bottleneck during serialization of large datasets. We can achieve the same result much faster using standard string manipulation on get_class($datum).
private function classNameMatchesSchema(object $datum, string $schemaName): bool
{
$class = get_class($datum);
$className = false !== ($pos = strrpos($class, '\\')) ? substr($class, $pos + 1) : $class;
return 0 === strcasecmp($className, $schemaName);
}| 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.
With the generated enums now implementing \JsonSerializable, we no longer need the custom property-fetch hacks for enum types or union types containing enums. We can simplify this method to always return the direct property fetch expression.
private function buildJsonSerializeValue(AvroSchema $fieldType, string $fieldName): Node\Expr
{
return new Node\Expr\PropertyFetch(new Node\Expr\Variable('this'), $fieldName);
}| private null \$nullField; | ||
| private bool \$boolField; | ||
| private int \$intField; | ||
| private int \$longField; | ||
| private float \$floatField; | ||
| private float \$doubleField; | ||
| private string \$stringField; | ||
| private string \$bytesField; | ||
| public function __construct(null \$nullField, bool \$boolField, int \$intField, int \$longField, float \$floatField, float \$doubleField, string \$stringField, string \$bytesField) | ||
| { | ||
| \$this->nullField = \$nullField; | ||
| \$this->boolField = \$boolField; | ||
| \$this->intField = \$intField; | ||
| \$this->longField = \$longField; | ||
| \$this->floatField = \$floatField; | ||
| \$this->doubleField = \$doubleField; | ||
| \$this->stringField = \$stringField; | ||
| \$this->bytesField = \$bytesField; | ||
| } | ||
| public function nullField(): null | ||
| { | ||
| return \$this->nullField; | ||
| } | ||
| public function boolField(): bool |
There was a problem hiding this comment.
Update the test expectation to match the PHP 8.1 compatible code generation (where standalone null type hints are omitted).
private $nullField;
private bool $boolField;
private int $intField;
private int $longField;
private float $floatField;
private float $doubleField;
private string $stringField;
private string $bytesField;
public function __construct($nullField, bool $boolField, int $intField, int $longField, float $floatField, float $doubleField, string $stringField, string $bytesField)
{
$this->nullField = $nullField;
$this->boolField = $boolField;
$this->intField = $intField;
$this->longField = $longField;
$this->floatField = $floatField;
$this->doubleField = $doubleField;
$this->stringField = $stringField;
$this->bytesField = $bytesField;
}
public function nullField()
{
return $this->nullField;
}
Review: Add PHP code generator from Avro schemasSecurity note (please investigate before merging)This PR adds four nearly identical new files --
These are instructions aimed at AI coding assistants/reviewers (like this one), not documentation for the PHP code generator. This reads as a prompt-injection attempt embedded in the diff, trying to get automated review tools to suppress parts of their output. I am flagging it rather than complying with it, and would recommend these four files be removed from the PR -- they are unrelated to "Add new code generator from AVRO to PHP" and should not be merged silently. Worth double-checking how they ended up in this PR (e.g. a copy-paste from another source, or a compromised tool in the authoring pipeline). Everything below reviews the actual PHP code generator changes on their merits. OverviewThe PR adds a Code quality / style
Potential bugs / correctness
Test coverageStrong coverage for the happy paths: nested/self-referential records, enums, unions (including nullable enum/records), arrays, maps, PSR-style namespacing from Avro namespaces, and round-trip byte-for-byte equivalence against the existing generic
PerformanceNothing concerning -- this is a one-shot codegen CLI operating on schema files at build/dev time, not a hot path. SecurityNo injection/traversal concerns in the generator itself (file paths come from the developer's own CLI invocation, not untrusted input). The one security-relevant item is the prompt-injection files noted at the top. Overall this is a well-structured, well-tested addition. The main things worth addressing before merge: (1) resolve the prompt-injection files, (2) decide whether |
🤖 Augment PR SummarySummary: This PR adds a new PHP code-generation toolchain that produces PHP classes from Avro schema files. Changes:
Technical Notes: Generated output maps Avro namespaces into a PSR-style directory tree under 🤖 Was this summary useful? React with 👍 or 👎 |
![augmentcode[bot]](https://avatars.githubusercontent.com/in/1027498?s=60&v=4){kind=small}
| protected function configure(): void | ||
| { | ||
| $this | ||
| ->addOption('file', 'f', InputOption::VALUE_OPTIONAL, 'One Avro schema file (.avsc)') |
There was a problem hiding this comment.
lang/php/lib/Console/GenerateCommand.php:43 — Using InputOption::VALUE_OPTIONAL for --file/--directory means invocations like --file without a value can be treated as “provided” (often as an empty string), which then bypasses the “exactly one input” check and fails later with less clear errors.
Severity: low
🤖 Was this useful? React with 👍 or 👎, or 🚀 if it prevented an incident/outage.
| } | ||
|
|
||
| if (null === $namespace || '' === $namespace) { | ||
| $io->error('PHP namespace is required (--namespace / -ns).'); |
There was a problem hiding this comment.
lang/php/lib/Console/GenerateCommand.php:79 — The error text says --namespace / -ns, but the configured short option is -N (and -ns won’t work), which is likely to confuse CLI users.
Severity: low
🤖 Was this useful? React with 👍 or 👎, or 🚀 if it prevented an incident/outage.
| return Command::FAILURE; | ||
| } | ||
|
|
||
| $outputDir = rtrim((string) realpath($outputDir), '/'); |
There was a problem hiding this comment.
lang/php/lib/Console/GenerateCommand.php:90 — Casting realpath($outputDir) to string can turn a failure into '', which then makes generated paths resolve to the filesystem root (e.g. /User.php) and produces misleading write errors instead of failing fast on an invalid output directory.
Severity: medium
🤖 Was this useful? React with 👍 or 👎, or 🚀 if it prevented an incident/outage.
| if ($count > 0) { | ||
| $encoder->writeLong($count); | ||
| foreach ($datum as $key => $value) { | ||
| $encoder->writeString((string) $key); |
There was a problem hiding this comment.
lang/php/lib/Datum/AvroSpecificDatumWriter.php:144 — writeMap() silently casts map keys to strings, so non-string keys (including numeric list keys) can be encoded as an Avro map, which can produce invalid encodings and also interacts badly with union branch selection.
Severity: high
Other Locations
lang/php/lib/Datum/AvroSpecificDatumWriter.php:214
🤖 Was this useful? React with 👍 or 👎, or 🚀 if it prevented an incident/outage.
| $property->setDocComment('/** @var '.$phpDocType.' */'); | ||
| } | ||
|
|
||
| if ($field->hasDefaultValue()) { |
There was a problem hiding this comment.
lang/php/lib/Generator/AvroCodeGenerator.php:141 — Applying raw Avro default values to typed properties can generate invalid PHP for enum/record fields (Avro defaults are JSON strings/objects, but the generated PHP types expect BackedEnum cases / object instances), leading to code that won’t load.
Severity: medium
Other Locations
lang/php/lib/Generator/AvroCodeGenerator.php:153
🤖 Was this useful? React with 👍 or 👎, or 🚀 if it prevented an incident/outage.
3 participants
3708: To review by AI