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

add mergeObjects() function #1897

Closed
wants to merge 1 commit into from
Closed

add mergeObjects() function #1897

wants to merge 1 commit into from

Conversation

Romaxx
Copy link

@Romaxx Romaxx commented Nov 16, 2018

Q A
Type feature
BC Break no

Summary

Copy link
Member

@alcaeus alcaeus left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for your contribution. Apart from the comments below, please note the following:

  • Please add tests as they are there for other aggregation pipeline operators. Take a look at AggregationOperatorsProviderTrait for guidance.
  • Just adding the operator to Expr doesn't help, you also need to expose it in the Operator class.

We'll have to see how to deal with the two different syntaxes for $mergeObjects: it can take a single argument (e.g. when used as accumulator in a $group stage) or 2 or more arguments (when used in other stages or operators). This also affects some other operators (e.g. $min and $max and currently isn't properly handled).

Please also note that I'm scheduling this for 2.1 as we've got a whole load of aggregation operators that need adding. You can always create the operator manually through the expression helper in the meantime.

*
* @see http://docs.mongodb.com/manual/reference/operator/aggregation/mergeObjects/
*
* @param Object1,Object2,...
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This typehint is wrong, please refer to other methods for guidance.

*
* @param Object1,Object2,...
*/
public function mergeObjects()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please look at other methods as to how to handle these arguments. Looking at the documentation for this operator, you can use $expression1, $expression2, ...$expressions and appropriate @param blocks. The add helper has a similar signature.

*/
public function mergeObjects()
{
$arrayarg = array();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's no need to copy the contents of func_get_args in a separate array, just pass them to the operator helper directly (again, see the add helper for example).

@@ -907,7 +907,23 @@ public function max($expression) : self
{
return $this->operator('$max', $expression);
}


Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are trailing spaces in this line which must not be there.

@alcaeus alcaeus added this to the 2.x milestone Dec 16, 2020
Base automatically changed from master to 2.3.x February 2, 2021 21:41
@alcaeus alcaeus removed this from the 2.x milestone Aug 5, 2021
@malarzm
Copy link
Member

malarzm commented Nov 4, 2023

mergeObjects was introduced with #2514

@malarzm malarzm closed this Nov 4, 2023
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.

3 participants