Throw SerializationError over client safe Error when failing to serialize leaf types
#903
Conversation
…erialize scalars Resolves #570
Codecov Report
@@ Coverage Diff @@
## master #903 +/- ##
==========================================
- Coverage 94.11% 94.10% -0.01%
==========================================
Files 117 117
Lines 9679 9675 -4
==========================================
- Hits 9109 9105 -4
Misses 570 570
Continue to review full report at Codecov.
|
spawnia
changed the title
SerializationError over client safe Error when failing to serialize scalarsSerializationError over client safe Error when failing to serialize leaf values
spawnia
changed the title
SerializationError over client safe Error when failing to serialize leaf valuesSerializationError over client safe Error when failing to serialize leaf types
|
I let the
|
| 'String cannot represent value: ' . Utils::printSafe($value) | ||
| ); | ||
| } | ||
|
|
||
| return (string) $value; | ||
| } | ||
|
|
||
| /** | ||
| * @param mixed $value |
There was a problem hiding this comment.
I'd keep those mixed so they can be converted to (mixed $value) on php 8 automatically.
With this change we wouldn't be able to distinguish intentional mixed versus forgotten type. Also, consumers can @inheritDoc
There was a problem hiding this comment.
The parent method still has the explicit mixed annotation.
There was a problem hiding this comment.
But the thing with php is to specify types even in implementations but that's related to previous discussion on disabling missing type checks.
We can have @inheritdoc here so it explicitly says we want mixed here.
With this change it's ambiguous: is it forgotten or inherited? What if parent type narrows, should it be narrowed here as well?
There was a problem hiding this comment.
Good tooling I use assumes the parent PHPDoc is inherited, unless specifically overwritten. Perhaps we can document and reify that assumption as part of the contribution guidelines?
There was a problem hiding this comment.
Do as you think is the best but personally I don't like trading automated checks for a piece of text few people read 🤷🏾♀️
There was a problem hiding this comment.
I am fairly confident we will keep this project moving towards stronger types without duplicating code.
# Conflicts: # CHANGELOG.md # src/Type/Definition/BooleanType.php # src/Type/Definition/FloatType.php # src/Type/Definition/IDType.php # src/Type/Definition/IntType.php # src/Type/Definition/StringType.php # tests/Executor/TestClasses/ComplexScalar.php
Resolves #570