martin-g commented on code in PR #3634:
URL: https://github.com/apache/avro/pull/3634#discussion_r2729197863
##########
lang/php/lib/DataFile/AvroDataIOReader.php:
##########
@@ -227,12 +211,82 @@ private function skipSync(): bool
/**
* Reads the block header (which includes the count of items in the block
* and the length in bytes of the block)
- * @returns int length in bytes of the block.
+ * @return int length in bytes of the block.
*/
private function readBlockHeader(): string|int
Review Comment:
In what cases does this function return a string ?
Update the `@return` at line 214
##########
lang/php/lib/Avro.php:
##########
@@ -49,42 +49,42 @@ class Avro
* self::GMP_BIGINTEGER_MODE on 32-bit platforms that have GMP available,
* and to self::PHP_BIGINTEGER_MODE otherwise.
*/
- private static int $biginteger_mode;
+ private static int $bigIntegerMode;
/**
* Wrapper method to call each required check.
*/
- public static function checkPlatform()
+ public static function checkPlatform(): void
{
self::check64Bit();
}
/**
- * @returns bool true if the PHP GMP extension is used and false otherwise.
+ * @return bool true if the PHP GMP extension is used and false otherwise.
* @internal Requires Avro::check64Bit() (exposed via
Avro::checkPlatform())
* to have been called to set Avro::$biginteger_mode.
Review Comment:
```suggestion
* to have been called to set Avro::$bigIntegerMode.
```
##########
lang/php/lib/IO/AvroStringIO.php:
##########
@@ -147,21 +126,21 @@ public function seek($offset, $whence = self::SEEK_SET):
bool
if (0 > $offset) {
throw new AvroIOException('Cannot seek before beginning of
file.');
}
- $this->current_index = $offset;
+ $this->currentIndex = $offset;
break;
case self::SEEK_CUR:
- if (0 > $this->current_index + $whence) {
+ if (0 > $this->currentIndex + $whence) {
Review Comment:
```suggestion
if (0 > $this->currentIndex + $offset) {
```
?!
##########
lang/php/lib/Protocol/AvroProtocol.php:
##########
@@ -28,6 +28,14 @@
/**
* Avro library for protocols
+ * @phpstan-import-type AvroSchemaDefinitionArray from AvroSchema
+ * @phpstan-import-type AvroProtocolMessageDefinitionArray from
AvroProtocolMessage
+ * @phpstan-type AvroProtocolDefinitionArray array{
+ * types: AvroSchemaDefinitionArray,
Review Comment:
```suggestion
* types?: AvroSchemaDefinitionArray,
```
`realParse()` below treats it as optional
##########
lang/php/lib/AvroDebug.php:
##########
@@ -59,37 +59,34 @@ public static function debug($format, $args, $debug_level =
self::DEBUG1)
* or more verbose than than the current debug level
* and false otherwise.
*/
- public static function isDebug(int $debug_level = self::DEBUG1): bool
+ public static function isDebug(int $debugLevel = self::DEBUG1): bool
{
- return self::DEBUG_LEVEL >= $debug_level;
+ return self::DEBUG_LEVEL >= $debugLevel;
}
/**
- * @param string $str
* @param string $joiner string used to join
- * @returns string hex-represented bytes of each byte of $str
+ * @return string hex-represented bytes of each byte of $str
* joined by $joiner
*/
- public static function hexString($str, $joiner = ' ')
+ public static function hexString(string $str, string $joiner = ' '): string
{
return implode($joiner, self::hexArray($str));
}
/**
- * @param string $str
Review Comment:
This parameter is still there
##########
lang/php/lib/Schema/AvroSchema.php:
##########
@@ -657,7 +665,7 @@ public static function isValidDatum(AvroSchema
$expected_schema, $datum): bool
case self::RECORD_SCHEMA:
case self::ERROR_SCHEMA:
case self::REQUEST_SCHEMA:
- if (!($expected_schema instanceof AvroRecordSchema)) {
+ if (!$expected_schema instanceof AvroRecordSchema) {
Review Comment:
Are you sure about this change ?
My AI friend says this is a wrong change.
##########
lang/php/lib/Schema/AvroRecordSchema.php:
##########
@@ -47,23 +55,25 @@ public function __construct(
);
}
- if (AvroSchema::REQUEST_SCHEMA === $schema_type) {
- parent::__construct($schema_type, $name);
+ if (AvroSchema::REQUEST_SCHEMA === $schemaType) {
+ parent::__construct($schemaType, $name);
} else {
- parent::__construct($schema_type, $name, $doc, $schemata,
$aliases);
+ parent::__construct($schemaType, $name, $doc, $schemata, $aliases);
}
[$x, $namespace] = $name->nameAndNamespace();
Review Comment:
```suggestion
[, $namespace] = $name->nameAndNamespace();
```
##########
lang/php/lib/DataFile/AvroDataIOWriter.php:
##########
@@ -67,52 +67,49 @@ class AvroDataIOWriter
/**
* @var string sync marker
*/
- private string $sync_marker;
+ private string $syncMarker;
public function __construct(
AvroIO $io,
- AvroIODatumWriter $datum_writer,
- string|AvroSchema|null $writers_schema = null,
+ AvroIODatumWriter $datumWriter,
+ string|AvroSchema|null $writersSchema = null,
string $codec = AvroDataIO::NULL_CODEC
) {
$this->io = $io;
- $this->datum_writer = $datum_writer;
+ $this->datumWriter = $datumWriter;
$this->encoder = new AvroIOBinaryEncoder($this->io);
$this->buffer = new AvroStringIO();
- $this->buffer_encoder = new AvroIOBinaryEncoder($this->buffer);
- $this->block_count = 0;
+ $this->bufferEncoder = new AvroIOBinaryEncoder($this->buffer);
+ $this->blockCount = 0;
$this->metadata = [];
- if ($writers_schema) {
+ if ($writersSchema) {
if (!AvroDataIO::isValidCodec($codec)) {
throw new AvroDataIOException(
sprintf('codec %s is not supported', $codec)
);
}
- $this->sync_marker = self::generateSyncMarker();
+ $this->syncMarker = self::generateSyncMarker();
$this->metadata[AvroDataIO::METADATA_CODEC_ATTR] = $this->codec =
$codec;
- $this->metadata[AvroDataIO::METADATA_SCHEMA_ATTR] = (string)
$writers_schema;
+ $this->metadata[AvroDataIO::METADATA_SCHEMA_ATTR] = (string)
$writersSchema;
Review Comment:
```suggestion
$this->metadata[AvroDataIO::METADATA_SCHEMA_ATTR] = (string)
$writersSchema;
if ($writersSchema instanceof AvroSchema) {
$this->datumWriter->writersSchema = $writersSchema;
} else {
$this->datumWriter->writersSchema =
AvroSchema::parse($writersSchema);
}
```
isn't this needed ?
##########
lang/php/lib/AvroDebug.php:
##########
@@ -59,37 +59,34 @@ public static function debug($format, $args, $debug_level =
self::DEBUG1)
* or more verbose than than the current debug level
* and false otherwise.
*/
- public static function isDebug(int $debug_level = self::DEBUG1): bool
+ public static function isDebug(int $debugLevel = self::DEBUG1): bool
{
- return self::DEBUG_LEVEL >= $debug_level;
+ return self::DEBUG_LEVEL >= $debugLevel;
}
/**
- * @param string $str
* @param string $joiner string used to join
- * @returns string hex-represented bytes of each byte of $str
+ * @return string hex-represented bytes of each byte of $str
* joined by $joiner
*/
- public static function hexString($str, $joiner = ' ')
+ public static function hexString(string $str, string $joiner = ' '): string
{
return implode($joiner, self::hexArray($str));
}
/**
- * @param string $str
- * @returns string[] array of hex representation of each byte of $str
+ * @return string[] array of hex representation of each byte of $str
*/
- public static function hexArray($str)
+ public static function hexArray(string $str): array
{
return self::bytesArray($str);
}
/**
- * @param string $str
Review Comment:
This parameter is still there
##########
lang/php/lib/Datum/AvroIODatumReader.php:
##########
@@ -490,18 +499,18 @@ public function readDefaultValue(AvroSchema
$field_schema, mixed $default_value)
case AvroSchema::UNION_SCHEMA:
return $this->readDefaultValue(
/** @phpstan-ignore method.notFound */
- $field_schema->schemaByIndex(0),
- $default_value
+ $fieldSchema->schemaByIndex(0),
+ $defaultValue
);
case AvroSchema::ENUM_SCHEMA:
case AvroSchema::FIXED_SCHEMA:
- return $default_value;
+ return $defaultValue;
case AvroSchema::RECORD_SCHEMA:
$record = [];
/** @phpstan-ignore method.notFound */
- foreach ($field_schema->fields() as $field) {
+ foreach ($fieldSchema->fields() as $field) {
$field_name = $field->name();
- if (!$json_val = $default_value[$field_name]) {
+ if (!$json_val = $defaultValue[$field_name]) {
Review Comment:
Does this work well when `$defaultValue[$field_name]` returns a falsy value
like `0`, `false` or `''` ?
##########
lang/php/lib/Datum/AvroIODatumWriter.php:
##########
@@ -103,39 +103,39 @@ private function writeValidatedData(AvroSchema
$writers_schema, mixed $datum, Av
return;
case AvroSchema::BYTES_TYPE:
- $this->writeBytes($writers_schema, $datum, $encoder);
+ $this->writeBytes($writersSchema, $datum, $encoder);
return;
case AvroSchema::ARRAY_SCHEMA:
- $this->writeArray($writers_schema, $datum, $encoder);
+ $this->writeArray($writersSchema, $datum, $encoder);
return;
case AvroSchema::MAP_SCHEMA:
- $this->writeMap($writers_schema, $datum, $encoder);
+ $this->writeMap($writersSchema, $datum, $encoder);
return;
case AvroSchema::FIXED_SCHEMA:
- $this->writeFixed($writers_schema, $datum, $encoder);
+ $this->writeFixed($writersSchema, $datum, $encoder);
return;
case AvroSchema::ENUM_SCHEMA:
- $this->writeEnum($writers_schema, $datum, $encoder);
+ $this->writeEnum($writersSchema, $datum, $encoder);
return;
case AvroSchema::RECORD_SCHEMA:
case AvroSchema::ERROR_SCHEMA:
case AvroSchema::REQUEST_SCHEMA:
- $this->writeRecord($writers_schema, $datum, $encoder);
+ $this->writeRecord($writersSchema, $datum, $encoder);
return;
case AvroSchema::UNION_SCHEMA:
- $this->writeUnion($writers_schema, $datum, $encoder);
+ $this->writeUnion($writersSchema, $datum, $encoder);
return;
default:
throw new AvroException(sprintf(
'Unknown type: %s',
- $writers_schema->type
+ $writersSchema->type
Review Comment:
```suggestion
$writersSchema->type()
```
##########
lang/php/lib/Datum/AvroIODatumReader.php:
##########
@@ -43,124 +43,126 @@
class AvroIODatumReader
{
public function __construct(
- private ?AvroSchema $writers_schema = null,
- private ?AvroSchema $readers_schema = null
+ private ?AvroSchema $writersSchema = null,
+ private ?AvroSchema $readersSchema = null
) {
}
- public function setWritersSchema(AvroSchema $readers_schema): void
+ public function setWritersSchema(AvroSchema $schema): void
{
- $this->writers_schema = $readers_schema;
+ $this->writersSchema = $schema;
}
public function read(AvroIOBinaryDecoder $decoder): mixed
{
- if (is_null($this->readers_schema)) {
- $this->readers_schema = $this->writers_schema;
+ if (is_null($this->readersSchema)) {
+ $this->readersSchema = $this->writersSchema;
}
return $this->readData(
- $this->writers_schema,
- $this->readers_schema,
+ $this->writersSchema,
+ $this->readersSchema,
$decoder
);
}
public function readData(
- AvroSchema $writers_schema,
- AvroSchema $readers_schema,
+ AvroSchema $writersSchema,
+ AvroSchema $readersSchema,
AvroIOBinaryDecoder $decoder
): mixed {
// Schema resolution: reader's schema is a union, writer's schema is
not
if (
- $readers_schema instanceof AvroUnionSchema
- && AvroSchema::UNION_SCHEMA === $readers_schema->type()
- && AvroSchema::UNION_SCHEMA !== $writers_schema->type()
+ $readersSchema instanceof AvroUnionSchema
+ && AvroSchema::UNION_SCHEMA === $readersSchema->type()
+ && AvroSchema::UNION_SCHEMA !== $writersSchema->type()
) {
- foreach ($readers_schema->schemas() as $schema) {
- if (self::schemasMatch($writers_schema, $schema)) {
- return $this->readData($writers_schema, $schema, $decoder);
+ foreach ($readersSchema->schemas() as $schema) {
+ if (self::schemasMatch($writersSchema, $schema)) {
+ return $this->readData($writersSchema, $schema, $decoder);
}
}
- throw new AvroIOSchemaMatchException($writers_schema,
$readers_schema);
+ throw new AvroIOSchemaMatchException($writersSchema,
$readersSchema);
}
- return match ($writers_schema->type()) {
+ return match ($writersSchema->type()) {
AvroSchema::NULL_TYPE => $decoder->readNull(),
AvroSchema::BOOLEAN_TYPE => $decoder->readBoolean(),
AvroSchema::INT_TYPE => $decoder->readInt(),
AvroSchema::LONG_TYPE => $decoder->readLong(),
AvroSchema::FLOAT_TYPE => $decoder->readFloat(),
AvroSchema::DOUBLE_TYPE => $decoder->readDouble(),
AvroSchema::STRING_TYPE => $decoder->readString(),
- AvroSchema::BYTES_TYPE => $this->readBytes($writers_schema,
$readers_schema, $decoder->readBytes()),
- AvroSchema::ARRAY_SCHEMA => $this->readArray($writers_schema,
$readers_schema, $decoder),
- AvroSchema::MAP_SCHEMA => $this->readMap($writers_schema,
$readers_schema, $decoder),
- AvroSchema::UNION_SCHEMA => $this->readUnion($writers_schema,
$readers_schema, $decoder),
- AvroSchema::ENUM_SCHEMA => $this->readEnum($writers_schema,
$readers_schema, $decoder),
- AvroSchema::FIXED_SCHEMA => $this->readFixed($writers_schema,
$readers_schema, $decoder),
+ AvroSchema::BYTES_TYPE => $this->readBytes($writersSchema,
$readersSchema, $decoder->readBytes()),
+ AvroSchema::ARRAY_SCHEMA => $this->readArray($writersSchema,
$readersSchema, $decoder),
+ AvroSchema::MAP_SCHEMA => $this->readMap($writersSchema,
$readersSchema, $decoder),
+ AvroSchema::UNION_SCHEMA => $this->readUnion($writersSchema,
$readersSchema, $decoder),
+ AvroSchema::ENUM_SCHEMA => $this->readEnum($writersSchema,
$readersSchema, $decoder),
+ AvroSchema::FIXED_SCHEMA => $this->readFixed($writersSchema,
$readersSchema, $decoder),
AvroSchema::RECORD_SCHEMA,
AvroSchema::ERROR_SCHEMA,
- AvroSchema::REQUEST_SCHEMA => $this->readRecord($writers_schema,
$readers_schema, $decoder),
+ AvroSchema::REQUEST_SCHEMA => $this->readRecord($writersSchema,
$readersSchema, $decoder),
default => throw new AvroException(sprintf(
"Cannot read unknown schema type: %s",
- $writers_schema->type()
+ $writersSchema->type()
)),
};
}
/**
* @throws AvroSchemaParseException
- * @return bool true if the schemas are consistent with
- * each other and false otherwise.
+ * @return bool true if the schemas are consistent with each other and
false otherwise.
*/
public static function schemasMatch(
- AvroSchema $writers_schema,
- AvroSchema $readers_schema
+ AvroSchema $writersSchema,
+ AvroSchema $readersSchema
): bool {
- $writers_schema_type = $writers_schema->type;
- $readers_schema_type = $readers_schema->type;
+ $writersSchemaType = $writersSchema->type;
+ $readersSchemaType = $readersSchema->type;
- if (AvroSchema::UNION_SCHEMA === $writers_schema_type ||
AvroSchema::UNION_SCHEMA === $readers_schema_type) {
+ if (AvroSchema::UNION_SCHEMA === $writersSchemaType ||
AvroSchema::UNION_SCHEMA === $readersSchemaType) {
return true;
}
- if (AvroSchema::isPrimitiveType($writers_schema_type)) {
- return true;
+ if (
+ AvroSchema::isPrimitiveType($writersSchemaType)
+ && AvroSchema::isPrimitiveType($readersSchemaType)
+ ) {
+ return $writersSchemaType === $readersSchemaType;
Review Comment:
This is wrong! It break type promotion, e.g. writer=Schema::Int and
reader=Schema::Long. This is allowed by the Avro spec and there is code for it
below (line 186+) but this check for equality returns early and does not allow
it.
##########
lang/php/lib/Protocol/AvroProtocol.php:
##########
@@ -28,6 +28,14 @@
/**
* Avro library for protocols
+ * @phpstan-import-type AvroSchemaDefinitionArray from AvroSchema
+ * @phpstan-import-type AvroProtocolMessageDefinitionArray from
AvroProtocolMessage
+ * @phpstan-type AvroProtocolDefinitionArray array{
+ * types: AvroSchemaDefinitionArray,
+ * protocol: string,
+ * namespace: string,
+ * messages: array<string, AvroProtocolMessageDefinitionArray>
Review Comment:
```suggestion
* messages?: array<string, AvroProtocolMessageDefinitionArray>
```
`realParse()` below treats it as optional
##########
lang/php/lib/Protocol/AvroProtocolMessage.php:
##########
@@ -29,13 +29,18 @@
use Apache\Avro\Schema\AvroSchema;
use Apache\Avro\Schema\AvroSchemaParseException;
+/**
+ * @phpstan-import-type AvroSchemaDefinitionArray from AvroSchema
+ * @phpstan-type AvroProtocolMessageDefinitionArray array{request:
AvroSchemaDefinitionArray, response: string}
Review Comment:
```suggestion
* @phpstan-type AvroProtocolMessageDefinitionArray array{request:
AvroSchemaDefinitionArray, response?: string}
```
##########
lang/php/lib/AvroDebug.php:
##########
@@ -100,40 +97,35 @@ public static function bytesArray($str, $format = 'x%02x')
}
/**
- * @param string $str
Review Comment:
This parameter is still there
##########
lang/php/lib/AvroDebug.php:
##########
@@ -59,37 +59,34 @@ public static function debug($format, $args, $debug_level =
self::DEBUG1)
* or more verbose than than the current debug level
* and false otherwise.
*/
- public static function isDebug(int $debug_level = self::DEBUG1): bool
+ public static function isDebug(int $debugLevel = self::DEBUG1): bool
{
- return self::DEBUG_LEVEL >= $debug_level;
+ return self::DEBUG_LEVEL >= $debugLevel;
}
/**
- * @param string $str
Review Comment:
This parameter is still there
##########
lang/php/lib/DataFile/AvroDataIOReader.php:
##########
@@ -72,71 +72,58 @@ public function __construct(
}
$this->codec = $codec;
- $this->block_count = 0;
+ $this->blockCount = 0;
// FIXME: Seems unsanitary to set writers_schema here.
// Can't constructor take it as an argument?
- $this->datum_reader->setWritersSchema(
+ $this->datumReader->setWritersSchema(
AvroSchema::parse($this->metadata[AvroDataIO::METADATA_SCHEMA_ATTR])
);
}
/**
* @throws AvroException
* @throws AvroIOException
- * @return array of data from object container.
+ * @return list<mixed> of data from object container.
* @internal Would be nice to implement data() as an iterator, I think
*/
public function data(): array
{
$data = [];
$decoder = $this->decoder;
while (true) {
- if (0 == $this->block_count) {
+ if (0 == $this->blockCount) {
if ($this->isEof()) {
break;
}
if ($this->skipSync()) {
+ /** @phpstan-ignore if.alwaysFalse */
Review Comment:
Why static analysis is disabled here ?
##########
lang/php/lib/AvroDebug.php:
##########
@@ -100,40 +97,35 @@ public static function bytesArray($str, $format = 'x%02x')
}
/**
- * @param string $str
* @param string $joiner string to join bytes of $str
- * @returns string of bytes of $str represented in decimal format
+ * @return string of bytes of $str represented in decimal format
* @uses decArray()
*/
- public static function decString($str, $joiner = ' ')
+ public static function decString(string $str, string $joiner = ' '): string
{
return implode($joiner, self::decArray($str));
}
/**
- * @param string $str
- * @returns string[] array of bytes of $str represented in decimal format
('%3d')
+ * @return string[] array of bytes of $str represented in decimal format
('%3d')
*/
- public static function decArray($str)
+ public static function decArray(string $str): array
{
return self::bytesArray($str, '%3d');
}
/**
- * @param string $str
Review Comment:
This parameter is still there
##########
lang/php/lib/AvroDebug.php:
##########
@@ -100,40 +97,35 @@ public static function bytesArray($str, $format = 'x%02x')
}
/**
- * @param string $str
* @param string $joiner string to join bytes of $str
- * @returns string of bytes of $str represented in decimal format
+ * @return string of bytes of $str represented in decimal format
* @uses decArray()
*/
- public static function decString($str, $joiner = ' ')
+ public static function decString(string $str, string $joiner = ' '): string
{
return implode($joiner, self::decArray($str));
}
/**
- * @param string $str
- * @returns string[] array of bytes of $str represented in decimal format
('%3d')
+ * @return string[] array of bytes of $str represented in decimal format
('%3d')
*/
- public static function decArray($str)
+ public static function decArray(string $str): array
{
return self::bytesArray($str, '%3d');
}
/**
- * @param string $str
* @param string $format one of 'ctrl', 'hex', or 'dec'.
* See {@link self::asciiArray()} for more
description
- * @param string $joiner
Review Comment:
This parameter is still there
##########
lang/php/lib/AvroDebug.php:
##########
@@ -100,40 +97,35 @@ public static function bytesArray($str, $format = 'x%02x')
}
/**
- * @param string $str
* @param string $joiner string to join bytes of $str
- * @returns string of bytes of $str represented in decimal format
+ * @return string of bytes of $str represented in decimal format
* @uses decArray()
*/
- public static function decString($str, $joiner = ' ')
+ public static function decString(string $str, string $joiner = ' '): string
{
return implode($joiner, self::decArray($str));
}
/**
- * @param string $str
- * @returns string[] array of bytes of $str represented in decimal format
('%3d')
+ * @return string[] array of bytes of $str represented in decimal format
('%3d')
*/
- public static function decArray($str)
+ public static function decArray(string $str): array
{
return self::bytesArray($str, '%3d');
}
/**
- * @param string $str
* @param string $format one of 'ctrl', 'hex', or 'dec'.
* See {@link self::asciiArray()} for more
description
- * @param string $joiner
- * @returns string of bytes joined by $joiner
+ * @return string of bytes joined by $joiner
* @uses asciiArray()
*/
- public static function asciiString($str, $format = 'ctrl', $joiner = ' ')
+ public static function asciiString(string $str, string $format = 'ctrl',
string $joiner = ' '): string
{
return implode($joiner, self::asciiArray($str, $format));
}
/**
- * @param string $str
Review Comment:
This parameter is still there
##########
lang/php/lib/AvroDebug.php:
##########
@@ -100,40 +97,35 @@ public static function bytesArray($str, $format = 'x%02x')
}
/**
- * @param string $str
* @param string $joiner string to join bytes of $str
- * @returns string of bytes of $str represented in decimal format
+ * @return string of bytes of $str represented in decimal format
* @uses decArray()
*/
- public static function decString($str, $joiner = ' ')
+ public static function decString(string $str, string $joiner = ' '): string
{
return implode($joiner, self::decArray($str));
}
/**
- * @param string $str
Review Comment:
This parameter is still there
--
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]
