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

Updated the test case for DDC6558 after review comments #6578

Closed
wants to merge 2 commits into from

Conversation

jeroenvrooij
Copy link

No description provided.

@lcobucci
Copy link
Member

lcobucci commented Aug 6, 2017

@jeroenvrooij DDC6558Employee is not mapped, leading JoinedSubclassPersister to not find the phoneNumber property. Adding it there makes your test to pass.

@Ocramius I'd close this one as invalid...

@jeroenvrooij
Copy link
Author

@lcobucci what do you mean with 'is not mapped'? I added the '@entity' annotation. In the original PR I also added the inheritance annotations to the Employee, but that lead to other issues and @Ocramius noted that that was an invalid mapping. See #6558 for some background info and a link to the other PR.

If you think multilevel inheritance is working properly can you please show me a brief example? We'd love to work with it, but we cannot find any docs or anybody to tell us how to do it. The implementations we tried for ourselves all had their flaws (hence the created issue ;))

@lcobucci
Copy link
Member

lcobucci commented Aug 7, 2017

@jeroenvrooij it's not mapped as a discriminator. I've made your tests pass by adding it there.

@jeroenvrooij
Copy link
Author

@lcobucci adding a discriminator on Employee is not valid. To quote @Ocramius:

Only one discriminator column is allowed at top level - this usage is invalid and should probably be reported by the schema validation

Check: #6559

@lcobucci
Copy link
Member

lcobucci commented Aug 7, 2017

@jeroenvrooij that's not what I meant, that specific class is not listed in your discriminator map (at the root class).
All classes of the inheritance tree should be there, even if they're abstract.

I'm replying from my mobile so it's quite difficult to send you snippets, however if you just add that class to your map the test will pass.

@lcobucci
Copy link
Member

lcobucci commented Aug 7, 2017

I'm now on my laptop... this makes your tests to pass:

/**
 * @Entity
 * @InheritanceType("JOINED")
 * @DiscriminatorColumn(name="discr", type="string")
 * @DiscriminatorMap({"manager" = "DDC6558Manager", "staff" = "DDC6558Staff", "developer" = "DDC6558Developer", "employee" = "DDC6558Employee"})
 */
abstract class DDC6558Person
{
    /** @Id @Column(type="integer") @GeneratedValue */
    public $id;
}

Note that I've added "employee" = "DDC6558Employee" to your discriminator map.

@jeroenvrooij
Copy link
Author

@lcobucci Thanks for the explanation, I now get what you mean :). And indeed: if I add the employee in the discriminator map of the root class then the test passes. At the moment I cannot quickly test it in our application because we already re-structured the design (we could not wait any longer). But I think that all scenarios (persisting, fetching, etc) will now work, awesome!

(You could even add "foo" = "DDC6558Employee" to the mapping, the key doesn't seem to matter. I suppose that is due to the fact that the Employee is abstract and will never be instantiated.)

We really struggled to get this set up, mainly because it is not in the Doctrine docs and we couldn't find any conclusive info on the web. I cannot imagine that we are the only ones facing this, so would this be something that should be added to the docs? If so, I would love to help making that happen :).

@Ocramius
Copy link
Member

@jeroenvrooij rather than documentation, this should be part of the schema validation tools.

The validator should have a piece of logic dedicated to detecting problems in the DiscriminatorMap

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.

4 participants