Skip to content

CLI-1729: Restore /translation API handling and _links removal logic in CLI#1962

Merged
danepowell merged 1 commit intoacquia:mainfrom
jigar-shah-acquia:CLI-1729
Feb 12, 2026
Merged

CLI-1729: Restore /translation API handling and _links removal logic in CLI#1962
danepowell merged 1 commit intoacquia:mainfrom
jigar-shah-acquia:CLI-1729

Conversation

@jigar-shah-acquia
Copy link
Contributor

Motivation

This PR restores the special handling for /translation API responses that was unintentionally removed in PR #1961 (CLI-1711).

Previously, the CLI stripped _links fields from responses returned by /translation endpoints before displaying output to users. The removal of this logic caused _links to reappear in CLI output, resulting in:

  • Unexpected output changes
  • Confusion for customers
  • Internal stakeholder concerns

Fixes #CLI-1729


Proposed changes

  • Reintroduce conditional handling for API paths starting with /translation.
  • Restore logic in ApiBaseCommand::mungeResponse() to:
    • Remove top-level _links
    • Remove nested _links within response objects
  • Reintroduce PHPUnit test coverage similar to the previous testTranslationResponse() test.
  • Ensure no behavior changes for non-translation endpoints.

End-user impact

  • _links will no longer appear in CLI output for /translation endpoints.
  • Output formatting for other endpoints remains unchanged.
  • No manual user action required.

Alternatives considered

  1. Leave _links in output

    • Rejected because it changes historical CLI behavior and creates inconsistent output formatting.
  2. Introduce a global _links stripping mechanism

    • Rejected because _links are valid and expected in other API responses.
    • The behavior should remain scoped only to /translation endpoints.

The chosen solution restores the original scoped behavior without impacting other commands.


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:
    ./bin/acli ckc
  3. Check for regressions
    Run existing API commands unrelated to /translation (e.g., environment or site instance commands).
    Verify _links fields are still present where previously expected.
    Run full PHPUnit suite:
vendor/bin/phpunit

Confirm no unrelated test failures.

  1. Check restored functionality
    Execute a /translation API command.
    Confirm:
    _links are not displayed in CLI output.
    Nested _links are also removed.
    Run PHPUnit tests and verify:
    Translation-specific test(s) pass.
    Response formatting matches expected structure.

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 restores the special handling for /translation API endpoints that was unintentionally removed in a previous change. The restoration ensures that _links fields are stripped from translation API responses before being displayed to users, maintaining historical CLI behavior and preventing confusion.

Changes:

  • Reintroduced conditional check for paths starting with /translation in ApiBaseCommand::execute()
  • Restored mungeResponse() method to remove _links from both top-level and nested response objects
  • Added comprehensive test coverage with four new test cases validating the _links removal logic

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.

File Description
src/Command/Api/ApiBaseCommand.php Restored the /translation path check and mungeResponse() method to strip _links fields from translation API responses
tests/phpunit/src/Commands/Api/ApiCommandTest.php Added four new test cases covering various scenarios for _links removal logic including edge cases for path matching

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@codecov
Copy link

codecov bot commented Feb 12, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 92.29%. Comparing base (5b44af7) to head (61bf8df).
⚠️ Report is 1 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff            @@
##               main    #1962   +/-   ##
=========================================
  Coverage     92.28%   92.29%           
- Complexity     1910     1916    +6     
=========================================
  Files           122      122           
  Lines          6987     6995    +8     
=========================================
+ Hits           6448     6456    +8     
  Misses          539      539           

☔ 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/1962/acli.phar

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

@danepowell danepowell merged commit e4c3d97 into acquia:main Feb 12, 2026
19 of 20 checks passed
@jfrank-nih
Copy link

jfrank-nih commented Feb 17, 2026

@jigar-shah-acquia, @danepowell: From the NCI viewpoint, this is a mistake. We can work with having the notification ID put directly into the JSON object and rewrite our own code to handle it. But we think it's a better idea to have this be standardized across all API endpoints, whether they are "translation" or not. From the end user perspective it is better to have a single standard.

Also, I will note that the ACLI command api:codebases:bulk-code-switch:start specifically lists --task-wait as an option, but it throws an error if you try to use it. acli itself is assuming that the links object is there.

In CommandBase.php line 2438:
                                                                  
  JSON object must contain the _links.notification.href property 

@danepowell
Copy link
Contributor

I hear you James, thanks for the feedback. I've included it on our ongoing email threads and we'll get back to you there. Cheers.

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants