-
Notifications
You must be signed in to change notification settings - Fork 2.5k
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
Implement named arguments for NEW DTO syntax #11116
Conversation
Thank you. Your new tests are failing, however. |
yeah you're right, I only tested locally with a php version above 8 (and it worked fine), but it seems like |
1660aa6
to
06fc20f
Compare
Tested locally with php 7.1, tests should pass fine now 👍 |
This probably needs to be updated as well: orm/docs/en/reference/dql-doctrine-query-language.rst Lines 1690 to 1691 in 2b91edc
|
lib/Doctrine/ORM/Query/Parser.php
Outdated
* | ||
* @return mixed | ||
*/ | ||
public function NewObjectArg() | ||
public function NewObjectArg(bool $namedArgAlreadyParsed = false) |
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.
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.
public function NewObjectArg(bool $namedArgAlreadyParsed = false) | |
public function NewObjectArg(/* bool $namedArgAlreadyParsed = false */) |
8d64606
to
d9a2dd1
Compare
@@ -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 : |
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.
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. |
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.
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 = []; |
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.
$unnamedArgs = []; | |
$orderedArgs = []; |
/** @var Node */ | ||
public $innerExpression; | ||
|
||
/** @var ?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.
/** @var ?string */ | |
/** @var string|null */ |
lib/Doctrine/ORM/Query/Parser.php
Outdated
{ | ||
$namedArgAlreadyParsed = func_get_args()[0] ?? false; |
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.
Do we actually need this parameter? Couldn't NewObjectExpression()
just check if the kind of argument it receives is supported at its position?
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 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 ?
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.
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 ?
c06a583
to
c683c30
Compare
just saw #11208, I will update the mr to target 3.1 👍 |
064ab47
to
11021dc
Compare
Update done
|
This was taken over by #11575 |
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
Closes #9182