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

Undefined alias for Embeddable in 2.7.1 #8031

Closed
vicdelfant opened this issue Feb 19, 2020 · 16 comments
Closed

Undefined alias for Embeddable in 2.7.1 #8031

vicdelfant opened this issue Feb 19, 2020 · 16 comments
Labels
Milestone

Comments

@vicdelfant
Copy link

Bug Report

Q A
BC Break yes
Version 2.7.1

Summary

2.7.1 seems to (partially) break support for embedded properties, resulting in a "column not found" exception. Reverting to 2.7.0 without any other code changes fixes this. The cause seems to be the use of an undefined alias for fetching the embeddable's properties.

Current behavior

The a1_ alias is introduced without being defined, generating the following SQL:

SELECT o0_.invoice_id AS invoice_id_0, o0_.invoice_status AS invoice_status_1, a1_.code_number AS code_number_11, a1_.code_year AS code_year_12
FROM orders_invoice o0_
WHERE o0_.code_year = ?
ORDER BY o0_.code_number DESC LIMIT 1'

This results in the following exception:

An exception occurred while executing 'SELECT o0_.invoice_id AS invoice_id_0, o0_.invoice_status AS invoice_status_1, a1_.code_number AS code_number_11, a1_.code_year AS code_year_12 FROM orders_invoice o0_ WHERE o0_.code_year = ? ORDER BY o0_.code_number DESC LIMIT 1' with params [2020]:

  SQLSTATE[42S22]: Column not found: 1054 Unknown column 'a1_.code_number' in 'field list'

How to reproduce

Upgrade to 2.7.1 and use the following entity and embeddable. The entity and embeddable are trimmed for readability but you get the gist:

Entity

class Invoice
{
    /**
     * @ORM\Embedded(class="FQCN\Foo\Bar\InvoiceCode")
     *
     * @var InvoiceCode
     */
    private $code;
}

Embedded

/**
 * @ORM\Embeddable
 */
class InvoiceCode extends AbstractYearSequenceValue
{
    
}

/**
 * @ORM\Embeddable
 */
abstract class AbstractYearSequenceValue
{
    /**
     * @ORM\Column(type="integer", name="number", length=6)
     *
     * @var int
     */
    protected $number;

    /**
     * @ORM\Column(type="smallint", name="year", length=4)
     *
     * @var int
     */
    protected $year;
}

Expected behavior

The existing o0_ alias is re-used, resulting in the following SQL:

SELECT o0_.invoice_id AS invoice_id_0, o0_.invoice_status AS invoice_status_1, o0_.code_number AS code_number_11, o0_.code_year AS code_year_12
FROM orders_invoice o0_
WHERE o0_.code_year = ?
ORDER BY o0_.code_number DESC LIMIT 1'
@Exalyon
Copy link

Exalyon commented Feb 19, 2020

Hi, I have the same problem.

After some investigation, I found out that the mapping now adds an inherited key to the fieldMapping for the Embeddable who have a superclass. This result in the following code to think
the superclass is an entity which have a table whereas it is incorrect.

    $tableName = (isset($mapping['inherited']))
    ? $this->em->getClassMetadata($mapping['inherited'])->getTableName()
    : $class->getTableName();

This code is found in the SqlWalker at the line 1397.

@beberlei beberlei added the Bug label Feb 20, 2020
@beberlei beberlei added this to the 2.7.2 milestone Feb 20, 2020
@beberlei
Copy link
Member

I believe #8006 is at fault, its the only thing that is related to embeddables in 2.7.1

Looping in @malarzm

@malarzm
Copy link
Member

malarzm commented Feb 23, 2020

I've tried to create a failing test case (#8036) but it's not failing 👀 @vicdelfant @Exalyon could you please take a look and tell me what I'm doing wrong in the test?

@malarzm
Copy link
Member

malarzm commented Feb 23, 2020

All right, got it failing with repository's findBy 👍 Indeed #8006 is the culprit as the test is passing again once I revert my patch

@vicdelfant
Copy link
Author

You beat me to it @malarzm :) Thanks for looking into this!

@malarzm
Copy link
Member

malarzm commented Feb 23, 2020

One thing I've found out is that if AbstractYearSequenceValue would be mapped as a @ORM\MappedSuperclass then error wouldn't be raised and everything would work as it should. Also this would be correct mapping from ORM's perspective I believe. But then I've mapped myself abstract classes as entities so I guess there are reasons for such mappings :) I'll be looking into how we can fix this without reverting #8006

@jaroslavlibal
Copy link

It also breaks the proxies generation process for me and I end up with the ReflectionException in RuntimeReflectionService.php at line 75 - Property XXX does not exist.

Reverting the #8006 locally fixed the issue for me.

@malarzm
Copy link
Member

malarzm commented Feb 24, 2020

@jaroslavlibal with "proxy generation process" you mean casual proxy generation with provided command? Do you also have embeddables that inherit from other embeddables?

@jaroslavlibal
Copy link

@malarzm Yes, exactly - orm:generate-proxies command. And yes, we have embeddables that inherit from other embeddables (value objects).

A Club entity with the ClubOfficial $chairman embeddable property.
The ClubOfficial embeddable extends the ContactPerson embeddable.
The ContactPerson embeddable has the Address $address property, which is also an embeddable.

In RuntimeReflectionService.php line 75:      
Property App\Model\Address\ContactPerson::$chairman.address.street does not exist

@malarzm
Copy link
Member

malarzm commented Feb 24, 2020

@jaroslavlibal would you mind testing if #8036 fixes your issue as well? I couldn't come up with an easy-enough test for your case but I my patch should bring back status quo. Although having embeddable which contains another embeddable sounds weird and unsupported as according to docs embeddables can contain only columns 🤔

@jaroslavlibal
Copy link

jaroslavlibal commented Feb 25, 2020

@malarzm Unfortunately I have to inform that it haven't resolved my issue. I have probably overlooked (years ago) the information that embeddables can contain only columns; however the information itself was not entirely true as embeddables containing embeddables worked well until the current version 😟

We considered embeddables as value objects, which can contain another value objects (and columns).

The problem does not occur with embeddables which contains another embeddables, when no inheritance is used (eg. BillingInformation embeddable which contains Address $address embeddable property mentioned above).

@beberlei
Copy link
Member

@jaroslavlibal i am not sure about embeddables can't contain embeddables, I believe it was added at some point, so that is why its working for you. We should be able to fix it, or revert, so no worries :)

@beberlei
Copy link
Member

beberlei commented Mar 1, 2020

@jaroslavlibal i have reproduced your problem on top of @malarzm PR.

@beberlei
Copy link
Member

beberlei commented Mar 1, 2020

@jaroslavlibal there was already a workaround about nested and inheritence in the code that broke the right fix in #8006. I disabled that workaround in a new commit to the #8036 PR. Can you verify that fixes your problem?

@jaroslavlibal
Copy link

@beberlei Yes, pulling gh-8031-extending-embeddable 5f5b3a5 fixes my problem. Thanks for your effort!

beberlei added a commit that referenced this issue Mar 15, 2020
…8006 (#8036)

* Add test case

* Treat parent embeddables as mapped superclasses

* [GH-8031] Bugfix: Get working again on nested embeddables in inherited embeddables.

* Housekeeping: CS

* Update note on limitations

* [GH-8031] Verify assocations still do not work with Embeddables.

* Housekeeping: CS

Co-authored-by: Benjamin Eberlei <kontakt@beberlei.de>
@beberlei
Copy link
Member

Fixed in #8036

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

5 participants