-
-
Notifications
You must be signed in to change notification settings - Fork 421
drop annotation support with entities #1126
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
Conversation
composer.json
Outdated
@@ -42,7 +42,8 @@ | |||
"sort-packages": true | |||
}, | |||
"conflict": { | |||
"doctrine/orm": "<2.10" | |||
"doctrine/orm": "<2.10", | |||
"doctrine/doctrine-bundle": "<2.4" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
2.4 introduces support for attributes... this allows use to remove some of our attribute conditionals. or at the very least - we would only need to verify the bundle is installed..
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
side note - we already have doctrine-bundle: ^2.4
as a dev dependency.
8e958a8
to
b09fdd8
Compare
if (!$this->doesEntityUseAttributeMapping($entityClassDetails->getFullName())) { | ||
throw new RuntimeCommandException(sprintf('Only attribute mapping is supported by make:entity, but the <info>%s</info> class uses a different format. If you would like this command to generate the properties & getter/setter methods, add your mapping configuration, and then re-run this command with the <info>--regenerate</info> flag.', $entityClassDetails->getFullName())); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is the one that we need to keep and determine if
- AttributeMapping is not used
- If somehow - AttributeMapping was being used but the class had
@ORM\Entity..
type annotations.
Then we would throw this exception..
{ | ||
return $this->doesClassUseDriver($className, AnnotationDriver::class); | ||
} | ||
|
||
public function doesClassUsesAttributes(string $className): bool | ||
{ | ||
return $this->doesClassUseDriver($className, AttributeDriver::class); | ||
} | ||
|
||
public function isDoctrineSupportingAttributes(): bool |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
After we remove that one if statement we talked about in this PR, is this used anymore?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It won't be when we're all said and done, but make:user
&& make:reset-password
are still calling on this.. they need to be refactored after this PR && the CSM one.
|
||
return $this->doctrineHelper->isClassAnnotated($className); | ||
} | ||
|
||
/** @legacy Drop when Annotations are no longer supported */ | ||
private function doesEntityUseAttributeMapping(string $className): bool |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we're keeping this, remove the comment above
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We shouldn't need to keep this. But for the same reasons above, we'll need to remove it in a final "clean up" PR.
@@ -579,10 +579,10 @@ private function addSingularRelation(BaseRelation $relation): void | |||
|
|||
$this->addGetter( | |||
$relation->getPropertyName(), | |||
$relation->getCustomReturnType() ?: $typeHint, | |||
$relation->getCustomReturnType() ?? $typeHint, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit pick, but I don't think ??
is needed here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe it still is because the return type for BaseRelation::getCustomReturnType(): ?string
.
Before, we just had a "string" type hint instead of an actual return type. Now, we have to watch out for uninitialized properties. Side note: we're doing a lot of cleanup in CSM in the respective PR.
f3024b0
to
e70cc83
Compare
Thanks Jesse! |
This PR was squashed before being merged into the 1.0-dev branch. Discussion ---------- [csm] strict typing && legacy code removal - drops CSM annotation support - cleans up legacy CSM code - implements strict typing - [x] Depends on #1126 Commits ------- ffc80c4 [csm] strict typing && legacy code removal
No description provided.