-
Notifications
You must be signed in to change notification settings - Fork 264
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
PHPLIB-1597 Accept a $-prefixed string anywhere an expression is accepted #1571
base: v1.x
Are you sure you want to change the base?
Conversation
generator/config/expressions.php
Outdated
@@ -45,6 +46,11 @@ | |||
$expressions = []; | |||
$resolvesToInterfaces = []; | |||
foreach ($bsonTypes as $name => $acceptedTypes) { | |||
// an expression can be a string with a $-prefixed field name | |||
if (! in_array('string', $acceptedTypes)) { | |||
$acceptedTypes[] = 'string'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Assuming this is the only code change in this PR, what about this change allows $-prefixed strings specifically? It looks like you're just accepting any kind of string whenever an expression is expected.
Side note: I couldn't confirm if this was really the only code change in the PR because GitHub's PR view is not correctly labeling all generated files. In the future it'd be preferable to segregate changes to generated files into their own commit.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added validation of $string
, but that's something the server would do.
Failed to optimize pipeline :: caused by :: $abs only supports numeric types, not string
bbe5e26
to
9f5a3fb
Compare
I had to change the order of conditions to please psalm. https://psalm.dev/r/4e4cd7b846 |
Fix PHPLIB-1597
The string representation of a field path is simpler than using the
FieldPath
factory.Before:
After: