Skip to content

Conversation

Matthew-Kilpatrick
Copy link

Description

Symfony serializer supports a Context attribute which can provide details for how to normalize a property. Specifically, a datetime_format key can be provided to specify how to format a DateTime field. This PR handles a datetime_format value of Y-m-d and sets the format to date, rather than the current incorrect value of date-time.

I don't believe there are any issues this PR will close, though this was brought up in a comment at #2145 (comment)

What type of PR is this? (check all applicable)

  • Bug Fix
  • Feature
  • Refactor
  • Deprecation
  • Breaking Change
  • Documentation Update
  • CI

Checklist

  • I have made corresponding changes to the documentation (docs/)
  • [] I have made corresponding changes to the changelog (CHANGELOG.md)

Uncertainties

  • The most suitable place I could find to place this was alongside the Symfony constraint annotation readers (SymfonyConstraintAnnotationReader), and making this a more generic SymfonyAnnotationReader class. The class was documented as @internal so I don't believe this would be a breaking change. If you'd rather I go down a different route, please give me some pointers of where/how you'd like this implemented.
  • I'm torn whether this should be classed as a feature (from the bundle perspective, functionality to parse this field doesn't yet exist) or bug (because from a user perspective, the bundle is generating the wrong format) - and as such, haven't updated the changelog yet.
  • I don't think any documentation changes should be required for this, so have checked this off on the checklist.

@Matthew-Kilpatrick Matthew-Kilpatrick changed the title Handle Symfony serializer datetime_format context for date formatting fix: Handle Symfony serializer datetime_format context for date formatting Sep 8, 2025
@Matthew-Kilpatrick Matthew-Kilpatrick force-pushed the symfony-context-datetime-format branch 4 times, most recently from d5b6deb to 14924a4 Compare September 13, 2025 18:25
@Matthew-Kilpatrick Matthew-Kilpatrick force-pushed the symfony-context-datetime-format branch from 14924a4 to 29aab02 Compare September 13, 2025 18:28
@DjordyKoert
Copy link
Collaborator

Thanks for opening a PR!

I think we should expand the $context argument (which is passed to various describers) based on whatever is set in the #[Context].

Then the DateTimePropertyDescriber can for example set a different format based on the passed context.

Add values from the Symfony Context attribute to property context, for use in property describers.
@Matthew-Kilpatrick Matthew-Kilpatrick force-pushed the symfony-context-datetime-format branch from eed3d6e to 982ffa2 Compare September 20, 2025 16:43
@Matthew-Kilpatrick
Copy link
Author

Thanks for your feedback, that does seem like a much better solution than what I originally went with!

As far as I could tell, there's no cases yet where the $context value passed to the describers has property/method-level context, and the current behaviour seemed to pass $model->getSerializationContext(). So, I've done some slight refactoring so that ObjectModelDescriber instantiates an array of property context data, which gets passed by reference to AnnotationsReader, so that annotation readers can update the context here. This then gets passed to describeProperty in ObjectModelDescriber, where it's merged with the $model->getSerializationContext() (I originally tried instantiating it to this in describe, but some of the JMS tests fail as I think the context is updated).

I've kept the processing for checking if the Context attribute exists in the SymfonyAnnotationReader class which I renamed from SymfonyConstraintAnnotationReader in my previous commit as this feels like the best place for this to live, but if you'd like this to go somewhere else, please let me know!

The alternative I considered which would have required less refactoring was to store to the _context property of the OA\Schema passed to the describers & annotation readers - although, this felt a lot messier (I'd rather use the intended parameter getting passed around).

I've pushed a new commit with these changes to help keep track of my changes, but can squash everything down into a single commit once you're happy with everything before merging!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants