Skip to content

PHPLIB-1679 Call DocumentCodec::encode() only on objects #1687

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 3 commits into
base: v2.0
Choose a base branch
from

Conversation

GromNaN
Copy link
Member

@GromNaN GromNaN commented May 9, 2025

Fix PHPLIB-1679

  • Check if the input document is an object before calling DocumentCodec::encode()
  • Never validate the value returned by DocumentCodec::encode() as the type MongoDB\BSON\Document is enforced.

@GromNaN GromNaN requested a review from alcaeus May 9, 2025 14:00
@GromNaN GromNaN requested a review from a team as a code owner May 9, 2025 14:00
@GromNaN GromNaN changed the title PHPLIB-1679 Call DocumentCodec::encode() only on objects PHPLIB-1679 Call DocumentCodec::encode() only on objects May 9, 2025
@GromNaN GromNaN changed the base branch from v2.x to v2.0 May 9, 2025 14:00
@GromNaN
Copy link
Member Author

GromNaN commented May 9, 2025

I should add tests, probably in CodecCollectionFunctionalTest.

@@ -306,6 +308,10 @@
// $args[0] was already validated above. Since DocumentCodec::encode will always return a Document
// instance, there is no need to re-validate the returned value here.
if ($codec) {
if (! is_object($args[0])) {

Check notice

Code scanning / Psalm

MixedArrayAccess Note

Cannot access array value on mixed variable $args
@@ -306,6 +308,10 @@
// $args[0] was already validated above. Since DocumentCodec::encode will always return a Document
// instance, there is no need to re-validate the returned value here.
if ($codec) {
if (! is_object($args[0])) {
throw UnsupportedValueException::invalidEncodableValue($args[0]);

Check notice

Code scanning / Psalm

MixedArrayAccess Note

Cannot access array value on mixed variable $args
@@ -341,6 +347,10 @@
}

if ($codec) {
if (! is_object($args[1])) {

Check notice

Code scanning / Psalm

MixedArrayAccess Note

Cannot access array value on mixed variable $args
@@ -341,6 +347,10 @@
}

if ($codec) {
if (! is_object($args[1])) {
throw UnsupportedValueException::invalidEncodableValue($args[1]);

Check notice

Code scanning / Psalm

MixedArrayAccess Note

Cannot access array value on mixed variable $args
@alcaeus
Copy link
Member

alcaeus commented May 12, 2025

I don't think we should add the error checks here. The template annotation on the DocumentCodec requires an object type as input for encode, but the idea is that the codecs accept any value and throw the invalidEncodableValue exception themselves if they cannot encode the value - that is unless one uses encodeIfSupported.

I wonder if we should instead document the parameter as mixed in the encode and decode methods in DocumentCodec (and potentially in Encoder and Decoder as well) as the methods can be called with the wrong input type but will simply throw an exception if it's not of the expected type. Not sure if we can represent that with static analysis annotations.

@GromNaN
Copy link
Member Author

GromNaN commented May 12, 2025

What I understand is that calling DocumentCodec::encode() with something other than an object of the class it can encode is an error: the DocumentCodec implementation must throw an exception at runtime in this case. But by no means a normal use case. UnsupportedValueException is a child of LogicException, meaning that's an error in the code.

LogicException that represents error in the program logic. This kind of exception should directly lead to a fix in your code.

The @psalm-param ObjectType $value annotation clearly shows the intention. I don't see why we should make static analysis less capable of detecting errors by not specifying the expected value type.

Throwing the error before or inside DocumentCodec::encode() will result in the same exception to be thrown.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants