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

doc: Review custom collections and repository docs #2653

Merged
merged 2 commits into from
Jun 25, 2024

Conversation

GromNaN
Copy link
Member

@GromNaN GromNaN commented Jun 24, 2024

Q A
Type doc
BC Break no
Fixed issues -

@GromNaN GromNaN requested a review from alcaeus June 24, 2024 12:52
@@ -3,9 +3,6 @@
Custom Collections
==================

.. note::
This feature was introduced in version 1.1
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We are v2.9, no need to mention such an old version.

Comment on lines 136 to 139
public function createFrom(array $elements): static
{
return new static($this->eventDispatcher, $elements);
}
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Implementing this method was missing from the doc.

class SectionCollection extend ArrayCollection
{
public function __construct(
private EventDispatcherInterface $eventDispatcher,
Copy link
Member Author

@GromNaN GromNaN Jun 24, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The doc doesn't say how to inject the dependency when creating a new SectionCollection.

I think, it must be done when creating the Application instance:

#[Document]
class Application
{
    public function __construct(
        #[EmbedMany(
            collectionClass: SectionCollection::class,
            targetDocument: Section::class,
        )]
        public Collection $sections;
    ) {}
}
$application = new Application(
    sections: new SectionCollection($eventDispatcher),
);

Or keeping the SectionCollection class internal:

#[Document]
class Application
{
    #[EmbedMany(
        collectionClass: SectionCollection::class,
        targetDocument: Section::class,
    )]
    public Collection $sections;
    public function __construct(
        EventDispatcherInterface $eventDispatcher,
    ) {
        $this->sections = new SectionCollection($eventDispatcher);
    }
}
$application = new Application($eventDispatcher);

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, instantiating the class for new documents is totally on user and was left as an exercise ;) Feel free to propose these 2 ways! Although in first one you should pass a SectionCollection to Application's ctor

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've chosen to keep only the first example. The 2nd is just a syntax variation. Since it's an advanced example, it's intended for developers who will be able to find the alternative if needed.

@@ -41,11 +40,6 @@ owning document's class.
Custom Collection Classes
-------------------------

.. note::
You may want to check `malarzm/collections <https://github.com/malarzm/collections>`_
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This package is not compatible with doctrine/collection v2. Consider restoring a link once @malarzm's package is updated.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No need to remove just yet, the package will be compatible soon ™️ :D

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On a more serious note, feel free ;)

Comment on lines +165 to +168
return match ($collectionClass) {
SectionCollection::class => new SectionCollection([], $this->eventDispatcher),
default => new $collectionClass(),
};
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The match operator is perfect for this use-case.

@GromNaN GromNaN marked this pull request as ready for review June 24, 2024 13:02
@GromNaN GromNaN changed the title Review custom collections and repository docs doc: Review custom collections and repository docs Jun 24, 2024
@@ -41,11 +40,6 @@ owning document's class.
Custom Collection Classes
-------------------------

.. note::
You may want to check `malarzm/collections <https://github.com/malarzm/collections>`_
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No need to remove just yet, the package will be compatible soon ™️ :D

class SectionCollection extend ArrayCollection
{
public function __construct(
private EventDispatcherInterface $eventDispatcher,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, instantiating the class for new documents is totally on user and was left as an exercise ;) Feel free to propose these 2 ways! Although in first one you should pass a SectionCollection to Application's ctor

@@ -41,11 +40,6 @@ owning document's class.
Custom Collection Classes
-------------------------

.. note::
You may want to check `malarzm/collections <https://github.com/malarzm/collections>`_
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On a more serious note, feel free ;)

Copy link
Member

@alcaeus alcaeus left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, but fine with waiting for @malarzm to update the collection classes in order to keep the docs in.

@GromNaN GromNaN merged commit df1cd7a into doctrine:2.9.x Jun 25, 2024
17 of 18 checks passed
@GromNaN GromNaN deleted the doc-factory branch June 25, 2024 10:07
@GromNaN GromNaN added this to the 2.9.0 milestone Jun 27, 2024
alcaeus added a commit that referenced this pull request Sep 6, 2024
* 2.9.x: (24 commits)
  Fix typo in code example (#2670)
  Label PRs about GH actions with "CI" (#2632)
  Review basic mapping (#2668)
  Fix wording (#2667)
  Add native type to private properties and final classes (#2666)
  Review and add tests on `ResolveTargetDocumentListener` (#2660)
  Remove soft-delete-cookbook (#2657)
  doc: Remove wakeup and clone cookbook (#2663)
  Modernize generated code for Hydrators (#2665)
  Add tests for introduction (#2664)
  doc: Review mapping ORM and ODM cookbook (#2658)
  doc: Review cookbook on blending ORM and ODM (#2656)
  doc: Review and test validation cookbook (#2662)
  Update custom mapping example (#2654)
  doc: Review Simple Search Engine Cookbook (#2659)
  doc: Add cookbook about embedding referenced documents using $lookup (#2655)
  doc: Add type to properties (#2652)
  doc: Review custom collections and repository docs (#2653)
  doc: Review Getting Started (#2650)
  Move annotations-reference to attributes-reference (#2651)
  ...
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