mattiabasone commented on code in PR #3624:
URL: https://github.com/apache/avro/pull/3624#discussion_r2688470178
##########
lang/php/lib/Schema/AvroField.php:
##########
@@ -82,35 +84,86 @@ class AvroField extends AvroSchema implements
AvroAliasedSchema
*/
private ?string $order;
private ?array $aliases;
+ private ?string $doc;
/**
- * @throws AvroSchemaParseException
+ * @param array<string> $aliases
* @todo Check validity of $default value
*/
- public function __construct(
- ?string $name,
+ private function __construct(
+ string $name,
string|AvroSchema $schema,
bool $isTypeFromSchemata,
bool $hasDefault,
mixed $default,
?string $order = null,
- mixed $aliases = null
+ ?array $aliases = null,
+ ?string $doc = null
) {
- if (!AvroName::isWellFormedName($name)) {
- throw new AvroSchemaParseException('Field requires a "name"
attribute');
- }
-
parent::__construct($schema);
$this->name = $name;
$this->isTypeFromSchemata = $isTypeFromSchemata;
$this->hasDefault = $hasDefault;
if ($this->hasDefault) {
$this->default = $default;
}
- self::checkOrderValue($order);
$this->order = $order;
- self::hasValidAliases($aliases);
$this->aliases = $aliases;
+
+ $this->doc = $doc;
+ }
+
+ /**
+ * @throws AvroSchemaParseException
+ */
+ public static function fromFieldDefinition(array $avro, ?string
$defaultNamespace, AvroNamedSchemata $schemata): self
Review Comment:
@martin-g I dicovered a bug when adding this method, basically one of the
tests ([this
one](https://github.com/mattiabasone/avro/blob/4eb807d64d1bc6bc6ca402531a0e66fa6aaf5eb1/lang/php/test/SchemaTest.php#L655-L663))
was failing with the error "MD5 is not a schema we know about.".
This happened because in this method I'm not passing the AvroNamedSchemata
as a reference, so my fear is that at some point we are not referring to the
same instance. This could be happening because [this
method](https://github.com/apache/avro/pull/3624/changes#diff-0198d22becf69eda13aa277ae38c37347f036df807d126e50c345fc2f5a98ab5L45)
was calling the `cloneWithNewSchema` function, which clones the schemata, but
then it was assigning the cloned schemata to the reference, overriding it.
Since all the PHP object are passed by reference by default, I think we
should do a cleanup of all these function which have as a parameter something
like this `?AvroNamedSchemata &$schemata = null` and change them to
`AvroNamedSchemata $schemata` if it's a strong requirment, instead of relying
on a reference which, at some point, it will be overwritten by someone changing
it from `null` to something else.
IMHO it's better to have an initialized empty AvroNamedSchemata that will be
updated during parsing, this is why I've changed the naming of the method to
`registerNamedSchema`
([reference](https://github.com/apache/avro/pull/3624/changes#diff-0198d22becf69eda13aa277ae38c37347f036df807d126e50c345fc2f5a98ab5R45))
Edit:
I did the cleanup in this commit
https://github.com/apache/avro/pull/3624/changes/377c963c98915d0f75a8f9e1ca1d48bd9785bda1
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
