Skip to content
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

Open
wants to merge 4 commits into
base: v1.x
Choose a base branch
from

Conversation

GromNaN
Copy link
Member

@GromNaN GromNaN commented Jan 23, 2025

Fix PHPLIB-1597

The string representation of a field path is simpler than using the FieldPath factory.

Before:

            Stage::group(
                _id: Expression::fieldPath('sizes'),
                averagePrice: Accumulator::avg(
                    Expression::numberFieldPath('price'),
                ),
            ),

After:

            Stage::group(
                _id: '$sizes'
                averagePrice: Accumulator::avg('$price'),
            ),

@GromNaN GromNaN requested a review from a team as a code owner January 23, 2025 14:05
@GromNaN GromNaN requested a review from jmikola January 23, 2025 14:05
@@ -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';
Copy link
Member

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.

Copy link
Member Author

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

@GromNaN GromNaN force-pushed the PHPLIB-1597 branch 2 times, most recently from bbe5e26 to 9f5a3fb Compare January 24, 2025 11:35
@GromNaN
Copy link
Member Author

GromNaN commented Jan 27, 2025

I had to change the order of conditions to please psalm. https://psalm.dev/r/4e4cd7b846

@GromNaN GromNaN enabled auto-merge (squash) January 27, 2025 14:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants