Improve code quality and increase PHPStan level#3634
Conversation
| foreach ($fieldSchema->fields() as $field) { | ||
| $field_name = $field->name(); | ||
| if (!$json_val = $default_value[$field_name]) { | ||
| if (!$json_val = $defaultValue[$field_name]) { |
There was a problem hiding this comment.
Does this work well when $defaultValue[$field_name] returns a falsy value like 0, false or '' ?
There was a problem hiding this comment.
uhm, it's wrong, nice catch.
lang/php/lib/IO/AvroStringIO.php
Outdated
| break; | ||
| case self::SEEK_CUR: | ||
| if (0 > $this->current_index + $whence) { | ||
| if (0 > $this->currentIndex + $whence) { |
There was a problem hiding this comment.
| if (0 > $this->currentIndex + $whence) { | |
| if (0 > $this->currentIndex + $offset) { |
?!
There was a problem hiding this comment.
Are you sure about this?
There was a problem hiding this comment.
Well, I have 0 experience with PHP but AFAIU $whence is used to tell from which position in the file to seek (beginning, current, end). In the other switch arms it checks whether the calculated position is a negative value. Just for SEEK_CUR it uses the $whence instead of $offset.
I am 99.9% certain this is a bug.
There was a problem hiding this comment.
Yup, it's a bug
| AvroSchema::isPrimitiveType($writersSchemaType) | ||
| && AvroSchema::isPrimitiveType($readersSchemaType) | ||
| ) { | ||
| return $writersSchemaType === $readersSchemaType; |
There was a problem hiding this 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.
There was a problem hiding this comment.
So, also the old check was wrong, right? Should be something like this:
if (
$writersSchemaType === $readersSchemaType
&& AvroSchema::isPrimitiveType($writersSchemaType)
&& AvroSchema::isPrimitiveType($readersSchemaType)
) {
return true;
}There was a problem hiding this comment.
This looks better but probably isPrimitiveType() is less expensive than === for complex schema types and should be first
There was a problem hiding this comment.
I've updated the check in 53f5894, wdyt?
|
Thank you, @mattiabasone ! |
2 participants
What is the purpose of the change
This PR improves the code quality and the strict type usage introduced in the previous PRs.
Verifying this change
This change is already covered by the existing test suite.
Documentation