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

Allow named Arguments to be passed to Dto #11575

Merged
merged 1 commit into from
Oct 12, 2024

Conversation

eltharin
Copy link
Contributor

@eltharin eltharin commented Aug 19, 2024

Allow to change argument order or use variadic argument in dto constructor.

When Dto implements (empty) interface WithNamedArguments, ObjectHydrator will pass an associated key<>value argument to Dto constructor.
Dto can use named arguments or a variadic argument to put values in their properties.

Final Goal (to do after) is to allow select NEW ItemDto(item.*) FROM App\Entity\Item as item

updated with with the nested New operator PR.

@greg0ire greg0ire changed the base branch from 3.2.x to 3.3.x August 19, 2024 20:30
@eltharin eltharin marked this pull request as draft August 19, 2024 20:44
@eltharin eltharin marked this pull request as ready for review August 20, 2024 13:24
docs/en/reference/dql-doctrine-query-language.rst Outdated Show resolved Hide resolved
docs/en/reference/dql-doctrine-query-language.rst Outdated Show resolved Hide resolved
docs/en/reference/dql-doctrine-query-language.rst Outdated Show resolved Hide resolved
phpcs.xml.dist Outdated Show resolved Hide resolved
src/Internal/Hydration/AbstractHydrator.php Outdated Show resolved Hide resolved
tests/Tests/ORM/Functional/NewOperatorTest.php Outdated Show resolved Hide resolved
tests/Tests/ORM/Functional/NewOperatorTest.php Outdated Show resolved Hide resolved
docs/en/reference/dql-doctrine-query-language.rst Outdated Show resolved Hide resolved
docs/en/reference/dql-doctrine-query-language.rst Outdated Show resolved Hide resolved
docs/en/reference/dql-doctrine-query-language.rst Outdated Show resolved Hide resolved
src/WithNamedArguments.php Outdated Show resolved Hide resolved
@eltharin eltharin force-pushed the named_arguments branch 5 times, most recently from 8be75b6 to f6ac22b Compare August 26, 2024 17:35
@eltharin eltharin requested a review from derrabus August 27, 2024 10:04
docs/en/reference/dql-doctrine-query-language.rst Outdated Show resolved Hide resolved
docs/en/reference/dql-doctrine-query-language.rst Outdated Show resolved Hide resolved
docs/en/reference/dql-doctrine-query-language.rst Outdated Show resolved Hide resolved
docs/en/reference/dql-doctrine-query-language.rst Outdated Show resolved Hide resolved
docs/en/reference/dql-doctrine-query-language.rst Outdated Show resolved Hide resolved
docs/en/reference/dql-doctrine-query-language.rst Outdated Show resolved Hide resolved
@greg0ire

This comment was marked as resolved.

@eltharin eltharin force-pushed the named_arguments branch 2 times, most recently from 4a1cd17 to c4b9991 Compare September 15, 2024 17:23
Note that you can only pass scalar expressions or other Data Transfer Objects to the constructor.

If you use your data-transfer objects for multiple queries but not necessarily with the same parameters,
you can use named arguments or variadic arguments, add the ``named`` keyword in your DQL query before your DTO :
Copy link
Member

Choose a reason for hiding this comment

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

Here you mentioned named arguments and variadic arguments. Is it necessary to add the NAMED keyword to use variadic arguments?

Copy link
Contributor Author

@eltharin eltharin Sep 18, 2024

Choose a reason for hiding this comment

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

variadic argument can be used without named keyword but the array will be "indexed" while with namedit will be associated

docs/en/reference/dql-doctrine-query-language.rst Outdated Show resolved Hide resolved
docs/en/reference/dql-doctrine-query-language.rst Outdated Show resolved Hide resolved
src/Exception/DuplicateFieldException.php Outdated Show resolved Hide resolved
src/Exception/NoMatchingPropertyException.php Outdated Show resolved Hide resolved
src/Query/Parser.php Outdated Show resolved Hide resolved
src/Query/Parser.php Outdated Show resolved Hide resolved
@eltharin eltharin force-pushed the named_arguments branch 2 times, most recently from 17b88d4 to 8334d0f Compare September 18, 2024 20:46
greg0ire
greg0ire previously approved these changes Sep 19, 2024
SenseException
SenseException previously approved these changes Sep 19, 2024
@greg0ire greg0ire added this to the 3.3.0 milestone Sep 20, 2024
@greg0ire
Copy link
Member

@derrabus please give this another review.

@eltharin
Copy link
Contributor Author

eltharin commented Oct 2, 2024

waiting for #11619 to push EBNF update

@eltharin eltharin dismissed stale reviews from SenseException and greg0ire via c5fd47d October 8, 2024 07:05
@eltharin eltharin force-pushed the named_arguments branch 2 times, most recently from c5fd47d to 5388eb8 Compare October 8, 2024 07:35
.. code-block:: php

<?php
$query = $em->createQuery('SELECT NEW NAMED CustomerDTO(name: c.name, value: CONCAT(a.city, ' ' , a.zip)) FROM Customer c JOIN c.address a');
Copy link
Member

Choose a reason for hiding this comment

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

I would prefer if we have only this syntax available CustomerDTO(name: c.name, value: CONCAT(a.city, ' ' , a.zip)) and not the two above that implicitly map the name or the AS naming.

This would in my opinion also make the NAMED keyword obsolete, which I would like to get rid of as well.

The goal should be that named arguments syntax looks exactly the same as in PHP itself, because the NEW className somewhat implies that this is what DQL supports.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Disable implicity map means that we have to write all the field's names for our queries, instead of add a keyword "named".

Remove AS keyword means for those who actually have a named array as result (with AS keyword) to rewrite all the names while in my case just have to add NEW NAMED myDTO(...) and it's done.

Forcing to write: name: c.name, lastname: c.lastname, age: c.age, birthdate: c.birthdate, ... is in my opinion a waste of time for the few fields to rename.

For those as me who have helpers to select fields to add in queries, remove NAMED keyword means to be obliged to name ALL the fields.

As I said in descritpion, final goal is to be able to write c.* and have all the fields by the names,

That's why the first version of this PR have only AS keyword and not the 'PHP syntax'

Copy link
Member

Choose a reason for hiding this comment

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

@eltharin I discussed with @greg0ire and @derrabus at our Doctrine core team meetup in person, and I guess the main thing is that we want to avoid adding 2 syntaxes (implicit and AS naming, plus name double colon). We considered both and your arguments and then we would rather remove the PHP-style named arguments syntax and only keep the examples 1+2, so the following should be allowed:

  • SELECT NEW NAMED CustomerDTO(a.city, c.name) FROM Customer c JOIN c.address a')
  • SELECT NEW NAMED CustomerDTO(c.name, CONCAT(a.city, ' ' , a.zip) AS value) FROM Customer c JOIN c.address a

and this syntax should be removed again:

  • SELECT NEW NAMED CustomerDTO(name: c.name, value: CONCAT(a.city, ' ' , a.zip)) FROM Customer c JOIN c.address a

Everything else is 👍 from me.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

thank you to all three of you (and other mainteners) for being so open-minded and for your work.

Allow to change argument order or use variadic argument in dto constructor using new named keyword
@greg0ire greg0ire merged commit 19d9244 into doctrine:3.3.x Oct 12, 2024
87 checks passed
@greg0ire
Copy link
Member

Thanks @eltharin !

@beberlei
Copy link
Member

@eltharin 💯 thanks

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.

5 participants