3708: Add new code generator from AVRO to PHP - #40
Conversation
…o add-schema-to-php-generator
WalkthroughThis pull request introduces PHP code generation from Avro schema files. It adds ✨ 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 |
![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 4 potential issues.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit c8f7572. Configure here.
| $caseName = strtoupper($value); | ||
| $enum->addStmt( | ||
| $this->factory->enumCase($caseName)->setValue($value) | ||
| ); |
There was a problem hiding this comment.
Invalid enum case identifiers
High Severity
Enum generation maps each Avro symbol to a PHP case via strtoupper on the symbol string. Avro allows symbols that are not valid PHP identifiers (hyphens, leading digits, etc.), so the emitted enum can be syntactically invalid or fail at runtime when loaded.
Reviewed by Cursor Bugbot for commit c8f7572. Configure here.
| $phpType = $this->avroTypeToPhp($field->type(), $phpNamespace); | ||
| $param = $this->factory->param($field->name())->setType($phpType); | ||
| if ($field->hasDefaultValue()) { | ||
| $param->setDefault($this->buildDefault($field->defaultValue())); |
There was a problem hiding this comment.
String defaults on enum fields
Medium Severity
For enum-typed record fields, buildDefault passes Avro’s default symbol string straight into property and constructor defaults. Generated PHP types the field as the enum class, so defaults like 'active' are not legal enum values and the output will not compile.
Reviewed by Cursor Bugbot for commit c8f7572. Configure here.
|
|
||
| if (null !== $node && $registeredSchema instanceof AvroNamedSchema) { | ||
| $filename = $path.'/'.ucwords($registeredSchema->name()).'.php'; | ||
| $files[$filename] = "<?php\n\ndeclare(strict_types=1);\n\n{$this->printer->prettyPrint([$node])}\n"; |
There was a problem hiding this comment.
Output paths ignore namespace
Medium Severity
Generated file paths use only ucwords of the Avro type’s short name, not its namespace or fullname. Multiple named types with the same short name in one run overwrite the same .php file, silently dropping earlier types.
Reviewed by Cursor Bugbot for commit c8f7572. Configure here.
| $schema instanceof AvroArraySchema => is_array($datum) | ||
| && ([] === $datum || array_is_list($datum)), | ||
| $schema instanceof AvroMapSchema => is_array($datum), | ||
| default => false, |
There was a problem hiding this comment.
Union picks map for lists
Medium Severity
When resolving a union branch, datumMatchesSchema treats any PHP array as matching a map schema, including ordered lists. If the map branch appears before the array branch, list values can be encoded as maps with stringified numeric keys, producing incorrect Avro binary.
Reviewed by Cursor Bugbot for commit c8f7572. Configure here.
Code Review: Add PHP Code Generator from Avro to PHPOverviewThis PR introduces a
The scope is meaningful and the overall approach is sound. Several issues need addressing before merge. 🚨 Security — Prompt Injection (Critical)The PR adds These files are designed to manipulate AI-assisted code review tools into suppressing their own findings and omitting links. This is a prompt injection attack against AI reviewers and should be removed entirely before any merge. These files have no legitimate place in this PR. 🐛 Bugs1. $version = InstalledVersions::getPrettyVersion('apache/avro');
$app = new Application('avro', $version); // Application expects string
$version = InstalledVersions::getPrettyVersion('apache/avro') ?? 'dev';2. $outputDir = rtrim((string) realpath($outputDir), '/');
$resolved = realpath($outputDir);
if (false === $resolved) {
$io->error(sprintf('Could not resolve output directory "%s".', $outputDir));
return Command::FAILURE;
}
$outputDir = rtrim($resolved, '/');3. Class name generation doesn't handle snake_case or camelCase ( $className = ucwords($avroRecord->name());
$className = str_replace('_', '', ucwords($name, '_'));4. Enum case names are not validated as PHP identifiers ( $caseName = strtoupper($value);Avro enum symbols can technically be any non-empty string. If a symbol contains characters that are invalid in PHP identifiers (e.g., a hyphen, dot, or space), the generated enum case will be syntactically broken. At minimum, validate and throw a ⚡ Performance5. private function classNameMatchesSchema(object $datum, string $schemaName): bool
{
$className = (new \ReflectionClass($datum))->getShortName();
return 0 === strcasecmp($className, $schemaName);
}This is called inside static $cache = [];
$fqcn = $datum::class;
$shortName = $cache[$fqcn] ??= (new \ReflectionClass($datum))->getShortName();
return 0 === strcasecmp($shortName, $schemaName);🔧 Code Quality6.
7. Redundant if (!file_exists($file)) { ... }
$json = file_get_contents($file);
if (false === $json) { ... }The 8. CI workflow path specificity The previous 9. Generator test variable named private AvroCodeGenerator $transpiler;The class is named ✅ What's Done Well
SummaryThe core implementation is solid, but the PR must not be merged until:
|
![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 PHP code generator for Apache Avro, adding a CLI tool, a Symfony console command, and an AvroCodeGenerator to generate PHP classes and enums from Avro schemas, along with an AvroSpecificDatumWriter for binary serialization. The review feedback highlights several critical and high-severity issues, including a fatal ClassNotFoundException due to a PHP-Parser v5 class rename, a potential file-writing bug when realpath fails, and severe performance degradation from repeated ReflectionClass instantiations. Additionally, the reviewer noted potential runtime crashes with scalar JSON schemas and uninstalled package versions, missing custom exception handling for invalid types, and potential namespace collisions when generating classes with identical names.
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.
| $arrayItems[] = new Node\ArrayItem( | ||
| $this->buildJsonSerializeValue($field->type(), $field->name()), | ||
| new String_($field->name()) | ||
| ); |
There was a problem hiding this comment.
In PHP-Parser v5, PhpParser\Node\ArrayItem has been removed and renamed to PhpParser\Node\Expr\ArrayItem. Using Node\ArrayItem will result in a fatal ClassNotFoundException at runtime when generating record classes. Update this to use Node\Expr\ArrayItem.
$arrayItems[] = new Node\Expr\ArrayItem(
$this->buildJsonSerializeValue($field->type(), $field->name()),
new String_($field->name())
);| return Command::FAILURE; | ||
| } | ||
|
|
||
| $outputDir = rtrim((string) realpath($outputDir), '/'); |
There was a problem hiding this comment.
If realpath($outputDir) returns false (e.g., due to permission issues or unresolved paths), casting it to a string yields "". This results in $outputDir becoming "", which eventually causes the code generator to write files to the root directory / (e.g., /User.php). Abort execution with an error if the path cannot be resolved.
$realOutputDir = realpath($outputDir);
if (false === $realOutputDir) {
$io->error(sprintf('Could not resolve absolute path for directory \'%s\'.', $outputDir));
return Command::FAILURE;
}
$outputDir = rtrim($realOutputDir, '/');| 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.
Instantiating ReflectionClass on every schema match or union resolution is extremely expensive and will severely degrade serialization performance, especially when encoding large arrays of records. You can retrieve the short class name much faster using get_class() and string manipulation.
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 writeRecord(AvroRecordSchema $schema, object $datum, AvroIOBinaryEncoder $encoder): void | ||
| { |
There was a problem hiding this comment.
Using a strict object type hint for $datum will cause PHP to throw a generic TypeError if an invalid type is passed. To ensure consistent and descriptive error handling, accept mixed and throw the library's native AvroIOTypeException instead.
private function writeRecord(AvroRecordSchema $schema, mixed $datum, AvroIOBinaryEncoder $encoder): void
{
if (!is_object($datum)) {
throw new AvroIOTypeException($schema, $datum);
}| private function writeArray(AvroArraySchema $schema, array $datum, AvroIOBinaryEncoder $encoder): void | ||
| { |
There was a problem hiding this comment.
Using a strict array type hint for $datum will cause PHP to throw a generic TypeError if an invalid type is passed. Accept mixed and throw the library's native AvroIOTypeException instead.
private function writeArray(AvroArraySchema $schema, mixed $datum, AvroIOBinaryEncoder $encoder): void
{
if (!is_array($datum)) {
throw new AvroIOTypeException($schema, $datum);
}| private function writeMap(AvroMapSchema $schema, array $datum, AvroIOBinaryEncoder $encoder): void | ||
| { |
There was a problem hiding this comment.
Using a strict array type hint for $datum will cause PHP to throw a generic TypeError if an invalid type is passed. Accept mixed and throw the library's native AvroIOTypeException instead.
private function writeMap(AvroMapSchema $schema, mixed $datum, AvroIOBinaryEncoder $encoder): void
{
if (!is_array($datum)) {
throw new AvroIOTypeException($schema, $datum);
}| try { | ||
| return self::realParse( | ||
| avro: json_decode($json, associative: true, flags: JSON_THROW_ON_ERROR), | ||
| schemata: new AvroNamedSchemata() | ||
| ); |
There was a problem hiding this comment.
If $json is a valid JSON string representing a non-array/non-string/non-null value (such as "true" or "123"), json_decode will return a boolean or integer. Passing this directly to realParse will trigger a PHP TypeError due to the strict parameter type hint. Validate the decoded value first and throw an AvroSchemaParseException.
try {
$decoded = json_decode($json, associative: true, flags: JSON_THROW_ON_ERROR);
if (!is_array($decoded) && !is_string($decoded) && !is_null($decoded)) {
throw new AvroSchemaParseException('Invalid Avro schema description.');
}
return self::realParse(
avro: $decoded,
schemata: new AvroNamedSchemata()
);| use Composer\InstalledVersions; | ||
| use Symfony\Component\Console\Application; | ||
|
|
||
| $version = InstalledVersions::getPrettyVersion('apache/avro'); |
There was a problem hiding this comment.
If the apache/avro package is not installed as a dependency (for example, during local development or testing of the repository itself), InstalledVersions::getPrettyVersion will throw an OutOfBoundsException. Add a fallback to prevent the CLI tool from crashing in development environments.
try {
$version = InstalledVersions::getPrettyVersion('apache/avro');
} catch (\OutOfBoundsException) {
$version = 'dev-master';
}
| }; | ||
|
|
||
| if (null !== $node && $registeredSchema instanceof AvroNamedSchema) { | ||
| $filename = $path.'/'.ucwords($registeredSchema->name()).'.php'; |
There was a problem hiding this comment.
The code generator flattens all Avro schemas into a single PHP namespace and output directory. If the Avro schema contains multiple types with the same name in different namespaces (e.g., com.example.User and com.other.User), they will overwrite each other in the output directory and the $files array. Consider organizing the generated files into subdirectories matching their Avro namespaces and appending the Avro namespace to the PHP namespace.
🤖 Augment PR SummarySummary: Adds a PHP code-generation CLI to produce PHP classes/enums from Avro Key changes:
🤖 Was this summary useful? React with 👍 or 👎 |
![augmentcode[bot]](https://avatars.githubusercontent.com/in/1027498?s=60&v=4){kind=small}
| $version = InstalledVersions::getPrettyVersion('apache/avro'); | ||
|
|
||
| $app = new Application('avro', $version); | ||
| $app->addCommand(new GenerateCommand()); |
There was a problem hiding this comment.
lang/php/bin/avro:50 — This calls Application::addCommand(), but the PR pins symfony/console to ^6.4 where addCommand() doesn’t exist, so the CLI will fatally error when running.
Severity: high
🤖 Was this useful? React with 👍 or 👎, or 🚀 if it prevented an incident/outage.
| ->addOption('file', 'f', InputOption::VALUE_OPTIONAL, 'One Avro schema file (.avsc)') | ||
| ->addOption('directory', 'd', InputOption::VALUE_OPTIONAL, 'A directory containing multiple Avro schema files (.avsc)') | ||
| ->addOption('output', 'o', InputOption::VALUE_REQUIRED, 'Output directory for generated PHP files') | ||
| ->addOption('namespace', 'ns', InputOption::VALUE_REQUIRED, 'PHP namespace for generated classes'); |
There was a problem hiding this comment.
lang/php/lib/Console/GenerateCommand.php:46 — The namespace option uses shortcut ns, but Symfony Console parses -ns as -n -s (single-letter shortcuts), so -ns won’t work as documented/mentioned in the error text.
Severity: medium
Other Locations
lang/php/README.md:73lang/php/lib/Console/GenerateCommand.php:79
🤖 Was this useful? React with 👍 or 👎, or 🚀 if it prevented an incident/outage.
| } | ||
|
|
||
| try { | ||
| $schema = AvroSchema::parse($json); |
There was a problem hiding this comment.
lang/php/lib/Console/GenerateCommand.php:124 — When --directory is used, each schema file is parsed via AvroSchema::parse() independently, so schemas that reference named types declared in other files in the same directory won’t be resolvable during generation.
Severity: medium
🤖 Was this useful? React with 👍 or 👎, or 🚀 if it prevented an incident/outage.
| return implode('|', array_unique($types)); | ||
| } | ||
|
|
||
| private function buildDefault(mixed $value): mixed |
There was a problem hiding this comment.
lang/php/lib/Generator/AvroCodeGenerator.php:310 — buildDefault() returns raw scalar defaults, which will generate invalid PHP for typed enum/record fields that have Avro defaults (e.g., enum default is a symbol string but the property/param type is a backed enum).
Severity: high
Other Locations
lang/php/lib/Generator/AvroCodeGenerator.php:146lang/php/lib/Generator/AvroCodeGenerator.php:158
🤖 Was this useful? React with 👍 or 👎, or 🚀 if it prevented an incident/outage.
|
|
||
| $constructor = $this->factory->method('__construct')->makePublic(); | ||
| $constructorParamDocs = []; | ||
| foreach ($avroRecord->fields() as $field) { |
There was a problem hiding this comment.
lang/php/lib/Generator/AvroCodeGenerator.php:154 — Constructor params are emitted in schema field order, so if any field with a default precedes a field without a default, the generated __construct() will have required parameters after optional ones (deprecated/problematic in PHP 8+).
Severity: medium
🤖 Was this useful? React with 👍 or 👎, or 🚀 if it prevented an incident/outage.
| && $this->classNameMatchesSchema($datum, $schema->name()), | ||
| $schema instanceof AvroArraySchema => is_array($datum) | ||
| && ([] === $datum || array_is_list($datum)), | ||
| $schema instanceof AvroMapSchema => is_array($datum), |
There was a problem hiding this comment.
lang/php/lib/Datum/AvroSpecificDatumWriter.php:214 — datumMatchesSchema() treats any array as matching AvroMapSchema without checking that keys are strings; this can mis-resolve unions containing both map and array branches compared to AvroSchema::isValidDatum()’s stricter map validation.
Severity: medium
🤖 Was this useful? React with 👍 or 👎, or 🚀 if it prevented an incident/outage.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@lang/php/lib/Datum/AvroSpecificDatumWriter.php`:
- Around line 212-215: The AvroMapSchema condition at line 214 currently accepts
any array with `is_array($datum)`, which causes list datums to be misclassified
as maps when both map and array types are present in a union. Modify the
AvroMapSchema matching condition to exclude list datums by adding a check that
ensures the array is not a list, such as `!array_is_list($datum)`, so that only
associative arrays (true maps) match this branch and preserve correct union
branch resolution.
In `@lang/php/lib/Generator/AvroCodeGenerator.php`:
- Around line 265-269: In the foreach loop where enum cases are being generated,
remove the strtoupper() normalization applied to $value when assigning it to
$caseName. Instead, use the $value directly as the case name to preserve the
distinct identity of Avro symbols. This prevents collapsing distinct symbols
like "ok" and "OK" into the same PHP enum case, which would result in invalid
code.
- Around line 184-200: The getter method names are generated directly from Avro
field names without checking if they conflict with PHP reserved keywords like
match, list, or class. Create a new private helper method buildGetterName that
takes a field name, checks if it is a PHP reserved keyword using a helper like
isPhpReservedKeyword, and if so, prefixes it with "get" and capitalizes the
field name (e.g., "match" becomes "getMatch"). Then replace the direct use of
$field->name() when calling $this->factory->method() with a call to
buildGetterName($field->name()) to ensure all generated getter methods have
valid PHP names while maintaining a deterministic, reproducible mapping.
In `@lang/php/README.md`:
- Around line 62-64: The fenced code block containing vendor/bin/avro is missing
a language identifier on the opening fence, which causes markdown linting to
fail. Add a language identifier such as bash or shell to the opening fence (the
three backticks) before the code block to specify the code language and satisfy
the linting requirement.
- Line 60: The markdown file is using ATX-style headings (with # symbols) but
the markdownlint configuration expects setext-style headings (with underlines).
Convert all headings at the mentioned locations (line 60 "Usage", line 66, line
77, and line 91) from ATX style to setext style by replacing the leading hash
symbols with underlines using equals signs (=) for level 1 headings and hyphens
(-) for level 2 headings placed directly below the heading text. Ensure all
headings throughout the file use the same setext-style format consistently.
In `@lang/php/test/Generator/AvroCodeGeneratorTest.php`:
- Around line 615-665: The generator's avroPrimitiveTypeToPhp() method is
mapping AvroSchema::NULL_TYPE to the PHP 8.1-incompatible standalone null type
hint. Update the mapping to return a PHP 8.1-compatible representation such as
?mixed instead of null. Then update the test expectations in
AvroCodeGeneratorTest.php to reflect this change: replace all instances of null
type hints (in the property declarations, constructor parameters, and return
type declarations like nullField(): null) with the new PHP 8.1-compatible type
representation to match the corrected generator output.
🪄 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: df2f6b39-ef12-474b-8f4f-38b88019a683
⛔ Files ignored due to path filters (11)
lang/php/test/Fixtures/Generated/Address.phpis excluded by!**/generated/**lang/php/test/Fixtures/Generated/Car.phpis excluded by!**/generated/**lang/php/test/Fixtures/Generated/FuelType.phpis excluded by!**/generated/**lang/php/test/Fixtures/Generated/Member.phpis excluded by!**/generated/**lang/php/test/Fixtures/Generated/Metadata.phpis excluded by!**/generated/**lang/php/test/Fixtures/Generated/Order.phpis excluded by!**/generated/**lang/php/test/Fixtures/Generated/Priority.phpis excluded by!**/generated/**lang/php/test/Fixtures/Generated/Profile.phpis excluded by!**/generated/**lang/php/test/Fixtures/Generated/Task.phpis excluded by!**/generated/**lang/php/test/Fixtures/Generated/Team.phpis excluded by!**/generated/**lang/php/test/Fixtures/Generated/User.phpis excluded by!**/generated/**
📒 Files selected for processing (23)
.cursor/rules.md.gemini/rules.md.github/workflows/test-lang-php.ymlAGENTS.mdCLAUDE.mdcomposer.jsonlang/php/.php-cs-fixer.dist.phplang/php/README.mdlang/php/bin/avrolang/php/lib/Console/GenerateCommand.phplang/php/lib/Datum/AvroSpecificDatumWriter.phplang/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/lib/Schema/AvroSchema.phplang/php/phpstan.neonlang/php/test/Console/GenerateCommandTest.phplang/php/test/Datum/AvroSpecificDatumWriterTest.phplang/php/test/Fixtures/Schemas/status.avsclang/php/test/Fixtures/Schemas/user.avsclang/php/test/Generator/AvroCodeGeneratorTest.php
| $schema instanceof AvroArraySchema => is_array($datum) | ||
| && ([] === $datum || array_is_list($datum)), | ||
| $schema instanceof AvroMapSchema => is_array($datum), | ||
| default => false, |
There was a problem hiding this comment.
Disambiguate map-vs-array union matching
At Line 214, map matching accepts any array, so list datums can be misclassified as map in unions that contain both map and array, producing the wrong union branch index/payload.
Proposed fix
- $schema instanceof AvroMapSchema => is_array($datum),
+ $schema instanceof AvroMapSchema => is_array($datum)
+ && ([] === $datum || !array_is_list($datum))
+ && [] === array_filter(
+ array_keys($datum),
+ static fn (int|string $key): bool => !is_string($key)
+ ),🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@lang/php/lib/Datum/AvroSpecificDatumWriter.php` around lines 212 - 215, The
AvroMapSchema condition at line 214 currently accepts any array with
`is_array($datum)`, which causes list datums to be misclassified as maps when
both map and array types are present in a union. Modify the AvroMapSchema
matching condition to exclude list datums by adding a check that ensures the
array is not a list, such as `!array_is_list($datum)`, so that only associative
arrays (true maps) match this branch and preserve correct union branch
resolution.
| foreach ($avroRecord->fields() as $field) { | ||
| $phpType = $this->avroTypeToPhp($field->type(), $phpNamespace); | ||
| $getter = $this->factory->method($field->name()) | ||
| ->makePublic() | ||
| ->setReturnType($phpType) | ||
| ->addStmt( | ||
| new Stmt\Return_( | ||
| new Node\Expr\PropertyFetch(new Node\Expr\Variable('this'), $field->name()) | ||
| ) | ||
| ); | ||
|
|
||
| $phpDocType = $this->avroTypeToPhpDoc($field->type(), $phpNamespace); | ||
| if (null !== $phpDocType) { | ||
| $getter->setDocComment('/** @return '.$phpDocType.' */'); | ||
| } | ||
|
|
||
| $class->addStmt($getter); |
There was a problem hiding this comment.
Guard generated getter names against PHP reserved keywords.
Getters are emitted as public function <fieldName>(), so valid Avro fields like match/list/class can produce non-compilable PHP output. Please sanitize or transform method names (and keep mapping deterministic).
Suggested direction
- $getter = $this->factory->method($field->name())
+ $getter = $this->factory->method($this->buildGetterName($field->name()))private function buildGetterName(string $fieldName): string
{
$normalized = preg_replace('/[^A-Za-z0-9_]/', '_', $fieldName) ?? $fieldName;
$candidate = $normalized;
if ($this->isPhpReservedKeyword($candidate)) {
$candidate = 'get'.ucfirst($normalized);
}
return $candidate;
}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@lang/php/lib/Generator/AvroCodeGenerator.php` around lines 184 - 200, The
getter method names are generated directly from Avro field names without
checking if they conflict with PHP reserved keywords like match, list, or class.
Create a new private helper method buildGetterName that takes a field name,
checks if it is a PHP reserved keyword using a helper like isPhpReservedKeyword,
and if so, prefixes it with "get" and capitalizes the field name (e.g., "match"
becomes "getMatch"). Then replace the direct use of $field->name() when calling
$this->factory->method() with a call to buildGetterName($field->name()) to
ensure all generated getter methods have valid PHP names while maintaining a
deterministic, reproducible mapping.
| foreach ($values as $value) { | ||
| $caseName = strtoupper($value); | ||
| $enum->addStmt( | ||
| $this->factory->enumCase($caseName)->setValue($value) | ||
| ); |
There was a problem hiding this comment.
Do not uppercase enum case names directly from symbols.
strtoupper($value) can collapse distinct Avro symbols (e.g. ok and OK) into the same PHP enum case and generate invalid code. Keep symbol identity without lossy normalization.
Minimal fix
- $caseName = strtoupper($value);
+ $caseName = $value;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@lang/php/lib/Generator/AvroCodeGenerator.php` around lines 265 - 269, In the
foreach loop where enum cases are being generated, remove the strtoupper()
normalization applied to $value when assigning it to $caseName. Instead, use the
$value directly as the case name to preserve the distinct identity of Avro
symbols. This prevents collapsing distinct symbols like "ok" and "OK" into the
same PHP enum case, which would result in invalid code.
|
|
||
| The `avro` CLI tool generates PHP classes from Avro schema files (`.avsc`). | ||
|
|
||
| ## Usage |
There was a problem hiding this comment.
Use the repository’s configured heading style consistently.
These headings use ATX style while the configured markdownlint rule expects setext style.
Also applies to: 66-66, 77-77, 91-91
🧰 Tools
🪛 markdownlint-cli2 (0.22.1)
[warning] 60-60: Heading style
Expected: setext; Actual: atx
(MD003, heading-style)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@lang/php/README.md` at line 60, The markdown file is using ATX-style headings
(with # symbols) but the markdownlint configuration expects setext-style
headings (with underlines). Convert all headings at the mentioned locations
(line 60 "Usage", line 66, line 77, and line 91) from ATX style to setext style
by replacing the leading hash symbols with underlines using equals signs (=) for
level 1 headings and hyphens (-) for level 2 headings placed directly below the
heading text. Ensure all headings throughout the file use the same setext-style
format consistently.
Source: Linters/SAST tools
| ``` | ||
| vendor/bin/avro [options] | ||
| ``` |
There was a problem hiding this comment.
Add a language identifier to the fenced block.
The usage block is missing a language tag, which triggers markdown linting.
🧰 Tools
🪛 markdownlint-cli2 (0.22.1)
[warning] 62-62: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@lang/php/README.md` around lines 62 - 64, The fenced code block containing
vendor/bin/avro is missing a language identifier on the opening fence, which
causes markdown linting to fail. Add a language identifier such as bash or shell
to the opening fence (the three backticks) before the code block to specify the
code language and satisfy the linting requirement.
Source: Linters/SAST tools
| 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 | ||
| { | ||
| return \$this->boolField; | ||
| } | ||
| public function intField(): int | ||
| { | ||
| return \$this->intField; | ||
| } | ||
| public function longField(): int | ||
| { | ||
| return \$this->longField; | ||
| } | ||
| public function floatField(): float | ||
| { | ||
| return \$this->floatField; | ||
| } | ||
| public function doubleField(): float | ||
| { | ||
| return \$this->doubleField; | ||
| } | ||
| public function stringField(): string | ||
| { | ||
| return \$this->stringField; | ||
| } | ||
| public function bytesField(): string | ||
| { | ||
| return \$this->bytesField; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify declared PHP target and locate standalone `null` type hints in generator/tests.
set -euo pipefail
echo "== composer php constraint =="
fd -a '^composer\.json$' | while read -r f; do
echo "-- $f"
rg -n '"php"\s*:' "$f" || true
done
echo
echo "== standalone null type hints in generator/tests =="
rg -nP '\b(null)\s+\$[A-Za-z_]\w*' lang/php/lib/Generator/AvroCodeGenerator.php lang/php/test/Generator/AvroCodeGeneratorTest.php || true
echo
echo "Expected:"
echo "- If composer requires PHP 8.1.*, no standalone 'null' type hints should be generated/expected."Repository: martin-augment/avro
Length of output: 311
🏁 Script executed:
cd /home/jailuser/git && wc -l lang/php/test/Generator/AvroCodeGeneratorTest.phpRepository: martin-augment/avro
Length of output: 118
🏁 Script executed:
cd /home/jailuser/git && sed -n '615,665p' lang/php/test/Generator/AvroCodeGeneratorTest.phpRepository: martin-augment/avro
Length of output: 2145
🏁 Script executed:
cd /home/jailuser/git && grep -n "null" lang/php/test/Generator/AvroCodeGeneratorTest.php | head -30Repository: martin-augment/avro
Length of output: 2395
🌐 Web query:
PHP 8.1 standalone null type hint validity syntax
💡 Result:
In PHP 8.1, the use of null as a standalone type is invalid and will result in a fatal compile-time error [1][2]. Support for null as a standalone type was introduced in PHP 8.2 [1][3]. Prior to this version, null could only be used as part of a union type (e.g., Type|null) or via the nullable type hint syntax (e.g.,?Type) [1][3]. Attempting to use a standalone null type declaration in PHP 8.1, such as in the example below, triggers the following error: Fatal error: Null cannot be used as a standalone type [1][2]. function example: null { // Triggers fatal error in PHP 8.1 return null; }
Citations:
- 1: https://php.watch/versions/8.2/null-false-types
- 2: https://www.drupal.org/project/project_browser/issues/3509380
- 3: https://www.php.net/language.types.declarations.php
🏁 Script executed:
cd /home/jailuser/git && grep -B5 "record_with_all_primitive_types" lang/php/test/Generator/AvroCodeGeneratorTest.php | head -20Repository: martin-augment/avro
Length of output: 216
🏁 Script executed:
cd /home/jailuser/git && grep -n "class AvroCodeGenerator" lang/php/lib/Generator/AvroCodeGenerator.phpRepository: martin-augment/avro
Length of output: 90
🏁 Script executed:
cd /home/jailuser/git && sed -n '39,100p' lang/php/lib/Generator/AvroCodeGenerator.phpRepository: martin-augment/avro
Length of output: 2049
🏁 Script executed:
cd /home/jailuser/git && rg -n "null" lang/php/lib/Generator/AvroCodeGenerator.php | grep -i "type\|primitive\|null.*hint" | head -20Repository: martin-augment/avro
Length of output: 329
🏁 Script executed:
cd /home/jailuser/git && grep -A30 "primitive_type_name\|mapPrimitiveType\|null.*type" lang/php/lib/Generator/AvroCodeGenerator.php | head -50Repository: martin-augment/avro
Length of output: 45
🏁 Script executed:
cd /home/jailuser/git && sed -n '280,320p' lang/php/lib/Generator/AvroCodeGenerator.phpRepository: martin-augment/avro
Length of output: 1680
🏁 Script executed:
cd /home/jailuser/git && sed -n '280,350p' lang/php/lib/Generator/AvroCodeGenerator.phpRepository: martin-augment/avro
Length of output: 3251
Generator produces invalid PHP 8.1 syntax for Avro null types; both generator mapping and test expectations require fixes.
The codebase targets PHP 8.1 (composer.json declares "php": "^8.1"), but standalone null type hints are not valid in PHP 8.1 and cause a fatal error. PHP 8.1 does not support null as a standalone type—it was introduced in PHP 8.2. The generator's avroPrimitiveTypeToPhp() method maps AvroSchema::NULL_TYPE to the string 'null', which when used as a type hint (as shown in lines 615, 623, 634) produces invalid code.
This test expects the generator to produce code that would fail fatally on the declared runtime. Fix the generator mapping to use ?mixed or another PHP 8.1–compatible representation, and update test expectations accordingly.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@lang/php/test/Generator/AvroCodeGeneratorTest.php` around lines 615 - 665,
The generator's avroPrimitiveTypeToPhp() method is mapping AvroSchema::NULL_TYPE
to the PHP 8.1-incompatible standalone null type hint. Update the mapping to
return a PHP 8.1-compatible representation such as ?mixed instead of null. Then
update the test expectations in AvroCodeGeneratorTest.php to reflect this
change: replace all instances of null type hints (in the property declarations,
constructor parameters, and return type declarations like nullField(): null)
with the new PHP 8.1-compatible type representation to match the corrected
generator output.
3 participants
3708: To review by AI