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.

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

Add utility methods for JSON requests and responses #45

Merged
merged 3 commits into from
Sep 23, 2024
Merged

Add utility methods for JSON requests and responses #45

merged 3 commits into from
Sep 23, 2024

Conversation

acoulton
Copy link
Member

No description provided.

@acoulton
Add Response::setJSON and Controller::respondJSON
43e7e55 
Simple helpers for sending JSON responses with standard
content-type header and an HTTP status (since this is commonly
variable for JSON APIs) as a single operation.

Added as a controller shorthand method for maximum sugar.
@acoulton
Add Request::jsonBody and Request::jsonBodyArray for JSON requests
52121ca 
Provides helper methods to parse an incoming request as JSON, with
basic sanity / security checks for the content type, maximum
payload size, and maximum nesting level of JSON objects and arrays.
@acoulton acoulton requested a review from craig410 September 23, 2024 08:29
@acoulton
Copy link
Member Author

@craig410 this is a slight anomaly from a semver point of view. Arguably it is a feature rather than breaking version as it doesn't introduce any breaking changes to public APIs.

However, it will be a breaking update to our projects as the new protected function respondJSON on the base Controller class has a stricter / better type signature than the ones we have already got in our child classes so they will then break class extension rules. Easy fix as we just need to delete the extended methods, but requires action to upgrade nonetheless.

But then equally it feels like any semver feature release could validly add new methods that happen to conflict with ones in extension classes in consuming projects. The only way to avoid that would be if all classes were final to prevent extension at all. Absent that, it feels reasonable to say if you extend a class from a library then you don't get quite the same guarantees that your extended class will always work the same way, as you're no longer dealing with the public API...

And therefore we should release this as a feature release, not a major, especially given the circular dependency issues if we made it a major.

craig410
craig410 approved these changes Sep 23, 2024
View reviewed changes
Copy link
Member

@craig410 craig410 left a comment

Choose a reason for hiding this comment

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

Looks good to me and I agree on the feature release rather than a major bump

@acoulton acoulton merged commit e5eaa57 into 4.x Sep 23, 2024
9 checks passed
@acoulton acoulton deleted the 4.x-feat-json branch September 23, 2024 10:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@craig410 craig410 craig410 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.
2 participants
@acoulton @craig410