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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implement named arguments for NEW DTO syntax #11116

Open
wants to merge 2 commits into
base: 3.2.x
Choose a base branch
from

Conversation

Zuruuh
Copy link

@Zuruuh Zuruuh commented Dec 12, 2023

I love using DTOs but it can get pretty annoying to understand what's going on when using a ton of arguments, having to keep track of what is what, in what order, etc... Using named arguments should really help a lot with that I think 馃憤
Small note but I also made it that trailing commas don't crash the lexer which is a nice qol feature to have imo. I can remove it / move it to another PR if needed 馃憣

Now the following DQL works as expected

SELECT NEW CustomerDTO(
  email: e.email,
  name: u.name,
  city: a.address,
) FROM ...

Closes #9182

@derrabus
Copy link
Member

Thank you. Your new tests are failing, however.

@Zuruuh
Copy link
Author

Zuruuh commented Dec 13, 2023

yeah you're right, I only tested locally with a php version above 8 (and it worked fine), but it seems like ReflectionClass::newInstanceArgs didn't work with an associative array before the introduction of named params in php, so it will require a bit more work to handle php 7.x versions

@Zuruuh Zuruuh force-pushed the named-arguments-for-new-keyword branch from 1660aa6 to 06fc20f Compare December 13, 2023 12:54
@Zuruuh
Copy link
Author

Zuruuh commented Dec 13, 2023

Tested locally with php 7.1, tests should pass fine now 馃憤

@Zuruuh Zuruuh requested a review from greg0ire December 13, 2023 18:54
@greg0ire
Copy link
Member

This probably needs to be updated as well:

NewObjectExpression ::= "NEW" AbstractSchemaName "(" NewObjectArg {"," NewObjectArg}* ")"
NewObjectArg ::= ScalarExpression | "(" Subselect ")"

*
* @return mixed
*/
public function NewObjectArg()
public function NewObjectArg(bool $namedArgAlreadyParsed = false)
Copy link
Member

Choose a reason for hiding this comment

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

The Parser is neither final nor internal. This means this is a breaking change for extending classes. You can obtain the new argument with func_get_args() without changing the signature.

Suggested change
public function NewObjectArg(bool $namedArgAlreadyParsed = false)
public function NewObjectArg(/* bool $namedArgAlreadyParsed = false */)

@Zuruuh Zuruuh force-pushed the named-arguments-for-new-keyword branch from 8d64606 to d9a2dd1 Compare December 13, 2023 23:54
@@ -584,6 +584,16 @@ And then use the ``NEW`` DQL keyword :

Note that you can only pass scalar expressions to the constructor.

The ``NEW`` operator also supports named arguments, similarly to php 8.0 :
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
The ``NEW`` operator also supports named arguments, similarly to php 8.0 :
The ``NEW`` operator also supports named arguments:

$query = $em->createQuery('SELECT NEW CustomerDTO(email: e.email, name: c.name, address: a.city) FROM Customer c JOIN c.email e JOIN c.address a');
$users = $query->getResult(); // array of CustomerDTO

Note that you cannot pass unnamed arguments after named ones, just like in php.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
Note that you cannot pass unnamed arguments after named ones, just like in php.
Note that you must not pass ordered arguments after named ones.

$obj = $class->newInstanceArgs($args);
} else {
$constructor = $class->getConstructor();
$unnamedArgs = [];
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
$unnamedArgs = [];
$orderedArgs = [];

/** @var Node */
public $innerExpression;

/** @var ?string */
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
/** @var ?string */
/** @var string|null */

{
$namedArgAlreadyParsed = func_get_args()[0] ?? false;
Copy link
Member

Choose a reason for hiding this comment

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

Do we actually need this parameter? Couldn't NewObjectExpression() just check if the kind of argument it receives is supported at its position?

Copy link
Author

Choose a reason for hiding this comment

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

I think this should work (but idk how to feel abt it): when parsing a NewObjectExpression, we parse the current one, consume the tokens, then parse the next one (if there is any) without consuming it, and throw if it's not named while the current one is. It would work without the need of the parameter, but i feel like it's kinda doing too much ? also we're parsing twice all the arguments while it's not really needed (idk if it's really impactant in terms of perf). wdyt ?

Copy link
Author

Choose a reason for hiding this comment

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

Hello @derrabus, I didn't want to ping earlier since it was holiday season; what do you think about the approach describe above ? did you have something else in mind ?

@Zuruuh Zuruuh requested a review from derrabus December 14, 2023 22:36
@Zuruuh Zuruuh force-pushed the named-arguments-for-new-keyword branch from c06a583 to c683c30 Compare February 6, 2024 20:47
@Zuruuh Zuruuh changed the base branch from 2.18.x to 2.19.x February 6, 2024 20:48
@Zuruuh
Copy link
Author

Zuruuh commented Feb 6, 2024

just saw #11208, I will update the mr to target 3.1 馃憤

@Zuruuh Zuruuh force-pushed the named-arguments-for-new-keyword branch from 064ab47 to 11021dc Compare February 7, 2024 00:16
@Zuruuh Zuruuh changed the base branch from 2.19.x to 3.1.x February 7, 2024 00:17
@Zuruuh Zuruuh requested a review from greg0ire February 7, 2024 00:52
@Zuruuh
Copy link
Author

Zuruuh commented Feb 7, 2024

Update done

  • dropped the custom implementation for php versions prior to 8.0 since the lib requires >=8.1
  • Implemented the changes that would have been breaking in src/Query/Parser.php since the class is now finalized

greg0ire
greg0ire previously approved these changes Feb 22, 2024
@Zuruuh Zuruuh requested review from norkunas and greg0ire March 5, 2024 12:51
@greg0ire greg0ire changed the base branch from 3.1.x to 3.2.x March 5, 2024 20:14
@greg0ire greg0ire removed the request for review from norkunas March 5, 2024 20:21
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.

None yet

4 participants