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: Update custom mapping example #2654

Merged
merged 1 commit into from
Jun 28, 2024

Conversation

GromNaN
Copy link
Member

@GromNaN GromNaN commented Jun 25, 2024

Q A
Type doc
BC Break no
Fixed issues -

Summary

The previous example was a simplified copy of the date type. In order to present something more useful, the new example is inspired by the codecs tutorial.

Also initializing tests on documentation examples.

docs/en/reference/custom-mapping-types.rst Outdated Show resolved Hide resolved
docs/en/reference/custom-mapping-types.rst Outdated Show resolved Hide resolved
@GromNaN GromNaN force-pushed the doc-custom-mapping branch 2 times, most recently from 31708c8 to f21b657 Compare June 26, 2024 05:37
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 with a minor suggestion for checks

{
// This is called to convert a Mongo value to a PHP representation
return new \DateTime('@' . $value->sec);
$timeZone = new DateTimeZone($value['tz']);
Copy link
Member

Choose a reason for hiding this comment

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

Just a suggestion, but this could benefit from some value checks:

if (! is_array($value) || ! isset($value['utc'], $value['tz'])) {
    throw new Exception(); // TODO: throw correct exception class
}

The end result will be the same, but I feel like there can be more useful information in the exception message rather than letting PHP throw its type errors. Not sure if there's prior art for this in our type, so feel free to disregard and leave this as an exercise to the user.

Copy link
Member Author

Choose a reason for hiding this comment

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

Absolutely, it's easy to be sure of a property's value, especially with typing, but there can always be surprises with database content, especially when there's no schema (which is certainly very common).

@GromNaN GromNaN force-pushed the doc-custom-mapping branch 2 times, most recently from 827f613 to 021a38c Compare June 26, 2024 09:53
@SenseException
Copy link
Member

@GromNaN Please update your commit messages to more meaningful ones. Don't repeat messages and don't use just one word.

@GromNaN
Copy link
Member Author

GromNaN commented Jun 26, 2024

Please update your commit messages to more meaningful ones. Don't repeat messages and don't use just one word.

The commit history is not very meaningful. I squash and update the unique commit message before merging. This commits are just a raw history of my changes and fixes that help when reviewing a second time the PR.

The previous example was a simplified copy of the date type. In order to present something more useful, the new example is inspired by MongoDB's codec tutorial.
@GromNaN
Copy link
Member Author

GromNaN commented Jun 27, 2024

@SenseException I squashed the commits in this PR.

@GromNaN GromNaN enabled auto-merge (squash) June 27, 2024 11:48
@GromNaN GromNaN added this to the 2.9.0 milestone Jun 27, 2024
@GromNaN GromNaN merged commit 8b542f4 into doctrine:2.9.x Jun 28, 2024
17 of 18 checks passed
@GromNaN GromNaN deleted the doc-custom-mapping branch June 29, 2024 06:16
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