PHPLIB-1635 BuilderEncoder accepts an instance of encoder instead of a class string #1608
Conversation
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| @@ -32,7 +34,7 @@ | |||
|
|
|||
| $encoded = []; | |||
| foreach ($value->getIterator() as $stage) { | |||
| $encoded[] = $this->encoder->encodeIfSupported($stage); | |||
| $encoded[] = $this->recursiveEncode($stage); | |||
Check notice
Code scanning / Psalm
MixedAssignment Note
There was a problem hiding this comment.
Shouldn't the return type be derived from @psalm-return on RecursiveEncode::recursiveEncode()? If not, does this need to be ignored in the baseline?
There was a problem hiding this comment.
That's already ignored in the baseline, but the first time, GitHub stills shows it.
There was a problem hiding this comment.
@alcaeus I also removed the interface and replaced the abstract class with a trait. That simplifies inheritance chain. This was useful for the constructor, but since only internal classes are instantiated by class name, I don't think we need them.
src/Builder/BuilderEncoder.php
Outdated
| @@ -67,7 +67,7 @@ public function canEncode(mixed $value): bool | |||
| return (bool) $this->getEncoderFor($value)?->canEncode($value); | |||
| } | |||
|
|
|||
| public function encode(mixed $value): stdClass|array|string|int | |||
| public function encode(mixed $value): mixed | |||
There was a problem hiding this comment.
Is mixed necessary here? I noticed it's absent on Encoder interface, which only specifies:
* @return mixed
* @psalm-return BSONType
I also noted that the original return type prior to this PR corresponds to the psalm-template implementation.
There was a problem hiding this comment.
Good point, that's what bothers me. There's no constraint on the return type of the custom encoders. So technically the BuilderEncoder could accept anything an Encoder would accept.
There was a problem hiding this comment.
I reverted to Type|stdClass|array|string|int, as this is expected.
| @@ -32,7 +34,7 @@ | |||
|
|
|||
| $encoded = []; | |||
| foreach ($value->getIterator() as $stage) { | |||
| $encoded[] = $this->encoder->encodeIfSupported($stage); | |||
| $encoded[] = $this->recursiveEncode($stage); | |||
There was a problem hiding this comment.
Shouldn't the return type be derived from @psalm-return on RecursiveEncode::recursiveEncode()? If not, does this need to be ignored in the baseline?
Fix PHPLIB-1635
Provides better dependency injection capability.