10000 fixing nullable handing in "normalizer" for nullable properties, fix condition for nullable properties by tecbird · Pull Request #841 · janephp/janephp · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

fixing nullable handing in "normalizer" for nullable properties, fix condition for nullable properties #841

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 7 commits into from
Apr 18, 2025
Merged

fixing nullable handing in "normalizer" for nullable properties, fix condition for nullable properties #841

merged 7 commits into from
Apr 18, 2025

Conversation

tecbird
Copy link
Contributor
@tecbird tecbird commented Jan 17, 2025

OpenAPI 3.0.x

code currently not working to set "null" in nullable properties

type: string nullable: true

@tecbird
Copy link
Contributor Author
tecbird commented Jan 17, 2025 •
edited
Loading

Example for "deactivated" that has "nullable" (null|DateTime) value:

without my fixes:

if ($object->isInitialized('deactivated') && null !== $object->getDeactivated()) { $data['deactivated'] = $object->getDeactivated()->format('Y-m-d\TH:i:sP'); }

with my fixes:
if ($object->isInitialized('deactivated')) { $data['deactivated'] = $object->getDeactivated()?->format('Y-m-d\TH:i:sP'); }

So i'm able to set "deactivated" to null via api call

@the-devbear the-devbear mentioned this pull request Mar 25, 2025
Open
@Korbeil
Copy link
Member
Korbeil commented Apr 4, 2025

Hey @tecbird, thanks for your change ! Could you rebase your change onto next branch pls ?
Also, I'm pretty sure we are missing some fixtures update and if you could add a regression test 🙏

@Fahl-Design
Copy link
Contributor

Hey @Korbeil, I pushed a rebase and will add a test soon ✌️

@Fahl-Design
Copy link
Contributor

Hey @Korbeil, I added a test case for a nullable date-time property. Sorry for that massive diff but as you can see there ware many generated normalizers affected (most in issue-445 based PictureparkSwagger.json and github)

Korbeil
Korbeil approved these changes Apr 17, 2025
View reviewed changes
Copy link
Member
@Korbeil Korbeil left a comment

Choose a reason for hiding this comment

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

Thanks for your changes ! 🙏

@Korbeil
Copy link
Member
Korbeil commented Apr 17, 2025

Mmmmm I cannot fixes conflicts since I do not have rights to push to your branch, could you fix them please ? 🙏
Also, could you add a note about your changes in CHANGELOG please ? 🙏

@Fahl-Design
Copy link
Contributor

Hey @Korbeil I rebased it and added a changelog entry ✌️

Korbeil
Korbeil reviewed Apr 18, 2025
View reviewed changes
CHANGELOG.md Outdated
@@ -22,6 +22,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
- [CI] [GH#850](https://github.com/janephp/janephp/pull/850) Update GH actions/cache to v4
- [OpenApi] [GH#845](https://github.com/janephp/janephp/pull/845) Content */* breaks generated Endpoint PHP class
- [JsonSchema] [GH#846](https://github.com/janephp/janephp/pull/846) Cast integer data to bool for boolean fields when integer is in data
- [JsonSchema] [GH#841](https://github.com/janephp/janephp/pull/841) Fix "nullable" property handing in generated normalizers
Copy link
Member

Choose a reason for hiding this comment

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

Please put it under the "Unreleased" section 🙏

Copy link
Contributor

Choose a reason for hiding this comment

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

Fixed ✌️

@Korbeil
Copy link
Member
Korbeil commented Apr 18, 2025

Thanks @Fahl-Design & @tecbird for your contribution ! 🙏

@Korbeil Korbeil merged commit a5adff9 into janephp:next Apr 18, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Fahl-Design Fahl-Design Fahl-Design left review comments

@Korbeil Korbeil Korbeil approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@tecbird @Korbeil @Fahl-Design
0