-
Notifications
You must be signed in to change notification settings - Fork 1
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
Support Embeddables #2
Comments
Thank you for identifying this issue. I've created a commit to fix the problem. You can test the solution on the master branch. If everything checks out, I'll proceed with publishing a stable version right after. Feel free to provide any feedback or further observations. Thanks again for your contribution to improving the project. |
Updated to dev version and it generated correct code for embeddables' access methods. Everything seems great so far, thank you! |
One issue I found is sometimes we need to initialize embeddable property in entity's constructor: It can depend from case to case, but I think if property is not nullable, and embeddable's constructor does not have any arguments, it's instantiation can be safely added to entity's __construct: #[ORM\Embeddable]
class MyEmbed
{
/// ... properties ...
public function __construct() // or no __construct at all
{
// no args - so we can generate new-statement safely
}
}
#[ORM\Entity]
class MyEntity
{
#[ORM\Embedded]
private MyEmbed $embed; // not nullable - it SHOULD be initialized within __construct
/*
* Getters / Setters (auto-generated)
*/
public function __construct()
{
$this->embed = new MyEmbed();
}
/// ... getters / setters ...
} That raises further question, though: what if MyEmbed::__construct has optional arguments? Then we can not be 100% sure that it should be instantiated without them. Normally, one would need to use EntityInitializerInterface to initialize the embeddable manually if they have to pass constructor args to it. So we can probably use this fact to decide do we put it the constructor or not. So my current suggestion is, while rendering entity's __construct method:
then output If it is only A and B, but there are required arguments, would be nice to emit a warning: "{propertyName} is missing an initializer". In all other cases, omit the initializer. PS. This is kinda opinionated, I know -- this feature is not critical, and developer can always use |
Another issue: trying to convert an embeddable class itself to generated methods, getting "No class found". On a side note, I had "No class found" also when trying to generate methods for a trait, that is used to hold a common set of fields for two different entities (can not use class inheritance there). Would be nice to fix this too. |
Results in:
It seems that embeddables are not supported currently, but neither are they skipped, and it fails here:
doctrine-entities-generator-bundle/src/EntityGenerator/EntityGenerator.php
Lines 113 to 115 in 4cccef6
because hasField is:
return isset($this->fieldMappings[$fieldName]) || isset($this->embeddedClasses[$fieldName])
The text was updated successfully, but these errors were encountered: