Add relationships - #7
Conversation
9f01777 to
e55b47c
Compare
74d1380 to
efa32f6
Compare
There was a problem hiding this comment.
Pull request overview
Adds MongoDB ODM relationship support (EmbedOne, EmbedMany, ReferenceOne, ReferenceMany) to the maker workflow, including source manipulation utilities and tests.
Changes:
- Introduces relation modeling classes and extends
ClassSourceManipulatorto generate ODM relation attributes/properties (including constructor initialization for collections). - Updates
MakeDocumentinteractive flow to prompt for relation details and write inverse-side mappings when requested. - Adds unit snapshot tests and functional maker tests to exercise relationship generation.
Reviewed changes
Copilot reviewed 21 out of 21 changed files in this pull request and generated 10 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/MongoDB/snapshots/reference_one.php | Snapshot for generated ReferenceOne property/attribute output. |
| tests/MongoDB/snapshots/reference_many.php | Snapshot for generated ReferenceMany collection + constructor init. |
| tests/MongoDB/snapshots/multiple_collections.php | Snapshot ensuring multiple collections are all initialized once in constructor. |
| tests/MongoDB/snapshots/field.php | Snapshot for adding a basic ODM field. |
| tests/MongoDB/snapshots/embed_one.php | Snapshot for generated EmbedOne property/attribute output. |
| tests/MongoDB/snapshots/embed_many.php | Snapshot for generated EmbedMany collection + constructor init. |
| tests/MongoDB/ClassSourceManipulatorTest.php | Adds snapshot-based unit tests for field + relation generation. |
| tests/Maker/MakeDocumentTest.php | Adds functional scenarios for creating documents with relations (currently with TODO assertions). |
| src/MongoDB/StaticReflectionService.php | Adds replacement reflection service for disconnected metadata loading. |
| src/MongoDB/RelationReferenceOne.php | Adds relation mapping object for ReferenceOne metadata. |
| src/MongoDB/RelationReferenceMany.php | Adds relation mapping object for ReferenceMany metadata. |
| src/MongoDB/RelationEmbedOne.php | Adds relation mapping object for EmbedOne metadata. |
| src/MongoDB/RelationEmbedMany.php | Adds relation mapping object for EmbedMany metadata. |
| src/MongoDB/MongoDBHelper.php | Refactors metadata discovery/autocomplete to use metadata factory and supports disconnected mode. |
| src/MongoDB/DocumentRelation.php | Introduces interactive relation model used by the maker flow. |
| src/MongoDB/ClassSourceManipulator.php | Adds APIs to generate ODM relation attributes/properties and init collections in constructors. |
| src/MongoDB/BaseRelation.php | Adds base relation value object used by relation types. |
| src/MongoDB/BaseCollectionRelation.php | Adds base class for collection relations (uses custom return type Collection). |
| src/Maker/MakeDocument.php | Extends maker prompts + generation logic to handle relations and inverse-side updates. |
| phpstan.neon.dist | Ignores class-not-found errors for snapshot fixtures. |
| phpcs.xml.dist | Relaxes an alignment sniff rule (likely due to new formatting). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| public function getOwningRelation(): RelationReferenceOne|RelationEmbedOne | ||
| { | ||
| return match ($this->getType()) { | ||
| self::REFERENCE_ONE => new RelationReferenceOne( | ||
| propertyName: $this->owningProperty, | ||
| targetClassName: $this->inverseClass, | ||
| targetPropertyName: $this->inverseProperty, | ||
| isSelfReferencing: $this->isSelfReferencing, | ||
| mapInverseRelation: $this->mapInverseRelation, | ||
| isOwning: true, | ||
| isNullable: $this->isNullable, | ||
| ), |
There was a problem hiding this comment.
$this->inverseProperty is a typed property that is not always initialized (e.g. when mapInverseRelation is false). Accessing it here will trigger a fatal error (Typed property ... must not be accessed before initialization). Fix by making $inverseProperty nullable (and defaulting to null) and/or only passing targetPropertyName when it is actually set.
| public function setMapInverseRelation(bool $mapInverseRelation): void | ||
| { | ||
| if ($mapInverseRelation && isset($this->inverseProperty)) { | ||
| throw new Exception('Cannot set setMapInverseRelation() to true when the inverse relation property is set.'); | ||
| } | ||
|
|
||
| $this->mapInverseRelation = $mapInverseRelation; | ||
| } |
There was a problem hiding this comment.
The guard condition is inverted: setting mapInverseRelation to true when inverseProperty is set should be valid, while setting it to false after inverseProperty is set is what creates an inconsistent state. Adjust the condition (and exception message) accordingly.
| $attributeOptions = ['targetDocument' => new ClassNameValue($typeHint, $targetClass)]; | ||
|
|
||
| // Add inversedBy or mappedBy if provided | ||
| if (isset($options['inversedBy'])) { | ||
| $attributeOptions['inversedBy'] = $options['inversedBy']; | ||
| } | ||
|
|
||
| if (isset($options['mappedBy'])) { | ||
| $attributeOptions['mappedBy'] = $options['mappedBy']; | ||
| } |
There was a problem hiding this comment.
ReferenceOne should not be configured with both inversedBy and mappedBy at the same time (they represent owning vs inverse concerns). Consider validating the $options combination and throwing an InvalidArgumentException (or picking one deterministically) to avoid generating invalid ODM mapping.
| foreach ($this->registry->getManagers() as $dm) { | ||
| assert($dm instanceof DocumentManager); | ||
| $cmf = $dm->getMetadataFactory(); | ||
| assert($cmf instanceof AbstractClassMetadataFactory); | ||
|
|
There was a problem hiding this comment.
Relying on assert() for runtime type safety is fragile because assertions may be disabled, which could lead to hard failures later. Prefer a real runtime guard (e.g. if (! $dm instanceof DocumentManager) { continue; }) and similarly for the metadata factory type.
| /** @return string[] */ | ||
| public function getParentClasses(string $class): array | ||
| { | ||
| return []; | ||
| } |
There was a problem hiding this comment.
These ReflectionService methods currently return hard-coded values ([] / true) that don't reflect the real class structure. This can cause incorrect metadata behavior in disconnected mode (e.g. treating non-existent methods as present, ignoring inheritance). Implement them using ReflectionClass (and matching visibility checks) to return accurate results.
| public function hasPublicMethod(string $class, string $method): bool | ||
| { | ||
| return true; | ||
| } |
There was a problem hiding this comment.
These ReflectionService methods currently return hard-coded values ([] / true) that don't reflect the real class structure. This can cause incorrect metadata behavior in disconnected mode (e.g. treating non-existent methods as present, ignoring inheritance). Implement them using ReflectionClass (and matching visibility checks) to return accurate results.
| yield 'it_creates_document_with_reference_one_relation' => [ | ||
| self::createMakeDocumentTest() | ||
| ->run(static function (MakerTestRunner $runner): void { |
There was a problem hiding this comment.
These newly added functional scenarios don't assert that the generated document code actually contains the expected relationship mapping (they contain TODO markers). Add concrete assertions (e.g., verify the generated files contain the expected ODM attributes, properties, and constructor initialization) so relation support is exercised end-to-end.
| // TODO: Add relation assertions | ||
| }), | ||
| ]; |
There was a problem hiding this comment.
These newly added functional scenarios don't assert that the generated document code actually contains the expected relationship mapping (they contain TODO markers). Add concrete assertions (e.g., verify the generated files contain the expected ODM attributes, properties, and constructor initialization) so relation support is exercised end-to-end.
There are 4 types of relationships: EmbedOne, EmbedMany, ReferenceOne, ReferenceMany