Skip to content

Conversation

@kalindi-adhiya
Copy link
Contributor

Motivation

Fixes #NNN

Proposed changes

Alternatives considered

Testing steps

  1. Follow the contribution guide to set up your development environment or download a pre-built acli.phar for this PR.
  2. If running from source, clear the kernel cache to pick up new and changed commands: ./bin/acli ckc
  3. Check for regressions: (add specific steps for this pr)
  4. Check new functionality: (add specific steps for this pr)

@kalindi-adhiya kalindi-adhiya added the bug Something isn't working label Dec 19, 2025
@codecov
Copy link

codecov bot commented Dec 19, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 92.06%. Comparing base (e43385f) to head (b3da9d9).
⚠️ Report is 2 commits behind head on main.

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.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@github-actions
Copy link

Try the dev build for this PR: https://acquia-cli.s3.amazonaws.com/build/pr/1946/acli.phar

curl -OL https://acquia-cli.s3.amazonaws.com/build/pr/1946/acli.phar
chmod +x acli.phar

@acquia-stalebot-platauto
Copy link

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 stale label to avoid it being closed. Thank you for your contributions. More info: https://github.com/acquia/devops-github-administration/blob/main/docs/operations_related_to_repositories.md#acquia-stale-bot

- 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
@kalindi-adhiya kalindi-adhiya marked this pull request as ready for review January 5, 2026 12:39
Copilot AI review requested due to automatic review settings January 5, 2026 12:39
Copy link
Contributor

Copilot AI left a 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 <175728472+Copilot@users.noreply.github.com>
Copy link
Contributor

@danepowell danepowell left a 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.

@kalindi-adhiya
Copy link
Contributor Author

kalindi-adhiya commented Jan 5, 2026

@danepowell the command will result in such error which can we further understood by user at-least.
I
Screenshot 2026-01-06 at 12 34 51 AM

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

kalindi.adhiya@Mac16,8-KalindiAdhiya cli % ./bin/acli api:environments:database-copy -h
Description:
  Copies a database to this environment.

Usage:
  api:environments:database-copy [options] [--] <environmentId> <name> <source>
  api:environments:database-copy 12-d314739e-296f-11e9-b210-d663bd873d93 "db_name" "14-0c7e79ab-1c4a-424e-8446-76ae8be7e851"
  api:environments:database-copy myapp.dev "db_name" "14-0c7e79ab-1c4a-424e-8446-76ae8be7e851"

Arguments:
  environmentId         The environment identifier. The identifier is a compound key consisting of the internal database ID of the environment and the application UUID. You may also use an environment alias or UUID.
  name                  The name of the database to copy to the selected environment.
  source                The ID of the environment to copy the database from.

Options:
      --task-wait       Wait for this task to complete
  -h, --help            Display help for the given command. When no command is given display help for the list command
  -q, --quiet           Do not output any message
  -V, --version         Display this application version
      --ansi|--no-ansi  Force (or disable --no-ansi) ANSI output
  -n, --no-interaction  Do not ask any interactive question
  -v|vv|vvv, --verbose  Increase the verbosity of messages: 1 for normal output, 2 for more verbose output and 3 for debug

Help:
  For more help, see https://cloudapi-docs.acquia.com/ or https://dev.acquia.com/api-documentation/acquia-cloud-site-factory-api for acsf commands.

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

kalindi.adhiya@Mac16,8-KalindiAdhiya cli % ./bin/acli api:site-instances:database:copy -h
Description:
Copy a database from one environment to another.

Usage:
api:site-instances:database:copy [options] [--] <environmentId> <siteId>
api:site-instances:database:copy myapp.dev

Arguments:
environmentId                                      Environment unique identifier You may also use an environment alias or UUID.
siteId                                             Site unique identifier

Options:
   --source_environment_id=SOURCE_ENVIRONMENT_ID  source_environment_id
   --task-wait                                    Wait for this task to complete
-h, --help                                         Display help for the given command. When no command is given display help for the list command
-q, --quiet                                        Do not output any message
-V, --version                                      Display this application version
   --ansi|--no-ansi                               Force (or disable --no-ansi) ANSI output
-n, --no-interaction                               Do not ask any interactive question
-v|vv|vvv, --verbose                               Increase the verbosity of messages: 1 for normal output, 2 for more verbose output and 3 for debug

Help:
For more help, see https://cloudapi-docs.acquia.com/ or https://dev.acquia.com/api-documentation/acquia-cloud-site-factory-api for acsf commands.

This endpoint is pre-release and therefore unsupported and may be changed or removed without notice.

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

@kalindi-adhiya
Copy link
Contributor Author

closing as not required

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

Labels

bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants