-
Notifications
You must be signed in to change notification settings - Fork 55
CLI-1668: added content type header #1946
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
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #1946 +/- ##
=========================================
Coverage 92.05% 92.06%
- Complexity 1894 1896 +2
=========================================
Files 122 122
Lines 6959 6966 +7
=========================================
+ Hits 6406 6413 +7
Misses 553 553 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
Try the dev build for this PR: https://acquia-cli.s3.amazonaws.com/build/pr/1946/acli.phar |
|
This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Please remove the |
41459fc to
cefb157
Compare
- Add Content-Type: application/json header for POST/PUT/PATCH requests - Send empty JSON body for requests without data to satisfy API requirements - Resolves Content-Type header must exist errors in API commands
cefb157 to
4eee6d7
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR adds Content-Type header support for POST/PUT/PATCH API requests that don't include body data. The changes ensure that these requests send an empty JSON body with the appropriate Content-Type header to satisfy API requirements.
Key Changes:
- Modified
addPostParamsToClient()to detect when no data is present and automatically add empty JSON body with Content-Type header for POST/PUT/PATCH requests - Added comprehensive test coverage for the new functionality, including tests for case-insensitive method comparison and verification that empty JSON is not added when data is present
- Added new tests to verify help text for pre-release and deprecated commands
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| src/Command/Api/ApiBaseCommand.php | Implements logic to add empty JSON body and Content-Type header for POST/PUT/PATCH requests without data |
| tests/phpunit/src/Commands/Api/ApiCommandTest.php | Adds three new tests for POST request scenarios and two new tests for verifying help text on pre-release and deprecated commands |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Co-authored-by: Copilot <[email protected]>
danepowell
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This smells wrong to me. Why would the api:environments:database-copy command work but api:site-instances:database:copy fail? The parameters look largely the same to me.
I think if you answer that question, you'll find the acquia-spec.json is broken, and this actually needs to be fixed in cx-api-spec repo, not with a code change here.
For instance, I notice the request body is marked as required in the environments endpoint but not in the site-instances endpoint.
|
@danepowell the command will result in such error which can we further understood by user at-least. I think we should make argument --source_environment_id compulsory/required argument to run this command . because source will be required for such calls , please check below commands for reference below command is missing source as required argument and hence resulting in error of missing request type but as per spec it was not marked required we have not marked source type as required this is my understanding of the issue where i feel source type is required even-though marked as false in required and it also make sense logically though i am not sure can you please guide on final way ? ..... I would definitely check at cx side tomorrow to confirm on the spec body required false side |
|
closing as not required |

Motivation
Fixes #NNN
Proposed changes
Alternatives considered
Testing steps
./bin/acli ckc