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 and add tests on ResolveTargetDocumentListener #2660

Merged
merged 2 commits into from
Jul 2, 2024

Conversation

GromNaN
Copy link
Member

@GromNaN GromNaN commented Jun 27, 2024

Q A
Type doc
BC Break no
Fixed issues -

Summary

  • Rewrote the introduction. The problem wasn't clear to me on first reading.
  • Modified the order in which the classes are presented, to make the explanations more logical. Highlight the module (namespace) of each class to make the purpose more explicit.
  • Added an usage example at the end, so that the result is more obvious.
  • Added tests

@GromNaN GromNaN requested a review from alcaeus June 27, 2024 13:33
@GromNaN GromNaN added this to the 2.9.0 milestone Jun 27, 2024
@GromNaN GromNaN force-pushed the doc-resolve-target-document branch from 011b109 to a60d1c7 Compare June 28, 2024 10:09
@GromNaN GromNaN force-pushed the doc-resolve-target-document branch from a60d1c7 to 8e6d157 Compare July 1, 2024 08:45
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.

The example we're using may not be the best considering how much code is necessary to explain this, but using these listeners is not necessary in most applications. This tutorial would do well with better examples, but I'm afraid that would be beyond the scope of the improvements we're planning to make. For the time being, the addition of tests and the better explanations are already a big improvement over the status quo.

I did leave a note about mentioning Sylius, as that's probably the most well-known practical usage of what we're explaining here. I'll let you decide whether you want to add that as an example of if you'd like to keep this abstract.


This functionality allows you to define relationships between different
documents without creating hard dependencies.
If you work with independent modules, you may encounter the problem of creating
Copy link
Member

Choose a reason for hiding this comment

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

In case you want to add an example, Sylius uses this concept extensively to allow for customising entity classes.

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 don't know Sylius to know what to write. I haven't found any reference to ResolveTargetDocumentListener or ResolveTargetEntityListener in the code or in the doc of Sylius.

}

An InvoiceSubjectInterface:
This class has de reference to the ``InvoiceSubjectInterface``. This interface
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
This class has de reference to the ``InvoiceSubjectInterface``. This interface
This class has a reference to an ``InvoiceSubjectInterface``. This interface

Copy link
Member Author

Choose a reason for hiding this comment

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

@GromNaN GromNaN merged commit e716dbe into doctrine:2.9.x Jul 2, 2024
17 of 18 checks passed
@GromNaN GromNaN mentioned this pull request Jul 2, 2024
@Steveb-p
Copy link
Contributor

Steveb-p commented Jul 2, 2024

The example we're using may not be the best considering how much code is necessary to explain this, but using these listeners is not necessary in most applications.

In applications yeah. In frameworks, it is really helpful to have this. I am using it often.

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.

3 participants