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

#6573 multiple "NEW" operator syntax #6574

Conversation

lzakrzewski
Copy link

In some cases, it will be good to use multiple NewObjectExpression.

For example, I used money library for money calculations and I would like to return the price of each item in my bucket as a Money value object.

            SELECT
                new Doctrine\Tests\Models\DDC6573\DDC6573Money(
                    i.priceAmount,
                    new Doctrine\Tests\Models\DDC6573\DDC6573Currency(i.priceCurrency)
                )
            FROM
                Doctrine\Tests\Models\DDC6573\DDC6573Item i

See: #6573

Right now I committed the only test.

@Majkl578
Copy link
Contributor

I'm wondering how the tests could be passing when this behavior is not supported now... 😕

@Ocramius
Copy link
Member

I'm wondering how the tests could be passing when this behavior is not supported now... 😕

Big WTF indeed. @lzakrzewski did you expect this? :O

@Bilge
Copy link
Contributor

Bilge commented Jul 23, 2017

Why is it so hard to understand? He wrote $this->markTestSkipped('Feature does not exist.'); at the head of test cases expected to fail, which skips those tests.

@Ocramius
Copy link
Member

Meh, totally missed it =_=

@lzakrzewski can you remove the "test skipped" blocks?

@lzakrzewski
Copy link
Author

@Ocramius @Majkl578 sorry for misleading you. "Test skipped" blocks removed.

@Majkl578
Copy link
Contributor

Yep, I must be blind... 😶
Anyway, I'd like to see this implemented - it may be a powerful tool for read-only layer separation to completely bypass full entity hydratation & management.
After a quick look: It's easy to support on parser level, but it will be more tricky to implement in hydrators since the code there assumes no DTOs could exist inside another DTOs. May even end up being rewritten/reimplented in order to have this. :/

@TNAJanssen
Copy link

TNAJanssen commented Sep 6, 2017

The parser is really easy to implement, is made it check for token type T_NEW.

The hydrator is a different story, I want to help but the issue also comes with the fact we don't know the nesting inside of the hydrator of somebody can help me with gathering this data I can continue to make an PR for this feature.

    if ($token['type'] === Lexer::T_NEW) {
        $expression = $this->NewObjectExpression();

        return $expression;
    }

@TNAJanssen
Copy link

#4687

@fesor
Copy link

fesor commented Sep 17, 2017

@TNAJanssen I implemented this but I not sure is this implementation is ok:

fesor@b48e6a4

  • In order to have this instantiation should be moved from Array/Object hydrator to AbstractHydrator.
  • also we need additional information to store hierarchy of arguments (in ResultsSetMapping)

@TNAJanssen
Copy link

@fesor indeed like that, do you already have it working?

@fesor
Copy link

fesor commented Sep 18, 2017

Yep, this commit contains all the implementation.

@TNAJanssen
Copy link

@fesor tests are also successful? I will test it tomorrow!

@fesor
Copy link

fesor commented Sep 19, 2017

@TNAJanssen tests successful but I didn't used test cases in this PR (Existing model already covers same cases). I also think that I need add few more test cases.

@Majkl578
Copy link
Contributor

Please create new PR for that and move discussion over there, it's not appropriate to discuss unrelated commits in this PR.

@fesor
Copy link

fesor commented Sep 19, 2017

Implementation for this PR: #6709

@Majkl578 Majkl578 added the DQL label Jan 1, 2018
Base automatically changed from master to old-prototype-3.x February 23, 2021 08:18
@greg0ire greg0ire closed this Aug 20, 2024
@greg0ire
Copy link
Member

Implemented in #11576

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.

7 participants