Skip to content
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
34 changes: 34 additions & 0 deletions config/prod/services.yml
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,8 @@ services:
- ../../src/DataStore/YamlStore.php
- ../../src/DataStore/JsonDataStore.php
- ../../src/CloudApi/AccessTokenConnector.php
# SourceConfig is instantiated by the command with the SAS client.
- ../../src/SasApi/SourceConfig.php
- ../../src/Command/App/From/**
public: true
resource: ../../src
Expand All @@ -55,10 +57,23 @@ services:
- ../../src/Command/Api/ApiBaseCommand.php
- ../../src/Command/Api/ApiListCommand.php
- ../../src/Command/Api/ApiListCommandBase.php
# The Source config commands inherit from ConfigCommandBase instead.
- ../../src/Command/Source/**
- ../../src/Command/App/From/**
Acquia\Cli\Command\CommandBase:
abstract: true

# Source config commands share a common abstract base (which carries the
# same constructor as CommandBase plus the SAS client service).
Acquia\Cli\Command\Source\ConfigCommandBase:
abstract: true
parent: Acquia\Cli\Command\CommandBase
Acquia\Cli\Command\Source\:
resource: ../../src/Command/Source
parent: Acquia\Cli\Command\Source\ConfigCommandBase
exclude:
- ../../src/Command/Source/ConfigCommandBase.php

Acquia\Cli\EventListener\ExceptionListener:
tags:
# @see Symfony\Component\Console\ConsoleEvents
Expand All @@ -79,6 +94,9 @@ services:
acsf.credentials:
class: Acquia\Cli\AcsfApi\AcsfCredentials

sas.credentials:
class: Acquia\Cli\SasApi\SasCredentials

# AcquiaCloudApi services.
Acquia\Cli\Command\Api\ApiCommandFactory: ~
Acquia\Cli\Command\Api\ApiBaseCommand:
Expand Down Expand Up @@ -132,6 +150,22 @@ services:
arguments:
Acquia\Cli\ApiCredentialsInterface: '@acsf.credentials'

# Sites Aggregation Service (SAS) API services.
# SAS shares the Accounts authentication layer with the Cloud API, so it
# reuses the standard cloud credentials; only the base URI differs.
Acquia\Cli\SasApi\SasConnectorFactory:
arguments:
$config:
# @see https://symfony.com/doc/current/service_container/expression_language.html
key: '@=service("cloud.credentials").getCloudKey()'
secret: '@=service("cloud.credentials").getCloudSecret()'
accessToken: '@=service("cloud.credentials").getCloudAccessToken()'
accessTokenExpiry: '@=service("cloud.credentials").getCloudAccessTokenExpiry()'
$baseUri: '@=service("sas.credentials").getBaseUri()'
$accountsUri: '@=service("cloud.credentials").getAccountsUri()'
Acquia\Cli\SasApi\SasConnector:
alias: Acquia\Cli\SasApi\SasConnectorFactory

# Symfony services.
Acquia\Cli\Application:
arguments:
Expand Down
171 changes: 171 additions & 0 deletions src/Command/Source/ConfigCommandBase.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,171 @@
<?php

declare(strict_types=1);

namespace Acquia\Cli\Command\Source;

use Acquia\Cli\ApiCredentialsInterface;
use Acquia\Cli\CloudApi\ClientService;
use Acquia\Cli\Command\CommandBase;
use Acquia\Cli\DataStore\AcquiaCliDatastore;
use Acquia\Cli\DataStore\CloudDataStore;
use Acquia\Cli\Exception\AcquiaCliException;
use Acquia\Cli\Helpers\LocalMachineHelper;
use Acquia\Cli\Helpers\LoopHelper;
use Acquia\Cli\Helpers\SshHelper;
use Acquia\Cli\Helpers\TelemetryHelper;
use Acquia\Cli\SasApi\SasClientService;
use Acquia\Cli\SasApi\SourceConfig;
use Psr\Log\LoggerInterface;
use SelfUpdate\SelfUpdateManager;
use Symfony\Component\Console\Command\Command;
use Symfony\Component\Console\Input\InputInterface;
use Symfony\Component\Console\Input\InputOption;
use Symfony\Component\Console\Output\OutputInterface;

/**
* Base class for Source config commands.
*
* Both directions are thin triggers over the SAS API: they ask SAS to run a
* `drush source:config:*` command on the environment. Push (import) reads
* config from the site's git repository into the CMS; pull (export) does the
* reverse and returns the exported config for the command to write to disk.
* No push payload travels through these commands.
*/
abstract class ConfigCommandBase extends CommandBase
{
public function __construct(
LocalMachineHelper $localMachineHelper,
CloudDataStore $datastoreCloud,
AcquiaCliDatastore $datastoreAcli,
ApiCredentialsInterface $cloudCredentials,
TelemetryHelper $telemetryHelper,
string $projectDir,
ClientService $cloudApiClientService,
SshHelper $sshHelper,
string $sshDir,
LoggerInterface $logger,
SelfUpdateManager $selfUpdateManager,
private readonly SasClientService $sasClient,
) {
parent::__construct(
$localMachineHelper,
$datastoreCloud,
$datastoreAcli,
$cloudCredentials,
$telemetryHelper,
$projectDir,
$cloudApiClientService,
$sshHelper,
$sshDir,
$logger,
$selfUpdateManager,
);
}

protected function configure(): void
{
$this
->acceptEnvironmentId()
->acceptSiteInstanceId()
->addOption('force', 'f', InputOption::VALUE_NONE, 'Do not ask for confirmation');

@wimleers wimleers Aug 18, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

⚠️ No other command here has --force. -f is taken elsewhere by --factory-url (AuthAcsfLoginCommand.php:22) and --format (MakeDocsCommand.php:34), so no collision — but worth a deliberate call on a new flag versus --no-interaction semantics, and it needs to be consistent with the -n behaviour I flagged on line 119.

}

/**
* Trigger the config operation on the environment and return the decoded response.
*/
abstract protected function triggerOperation(SourceConfig $sourceConfig, string $environmentId): object;

/**
* A short verb phrase describing the operation, e.g. "Importing configuration".
*/
abstract protected function operationLabel(): string;

/**
* Handle a successfully completed operation.
*
* The default does nothing (push). Pull overrides this to fetch the
* exported payload and write it to disk.
*
* @infection-ignore-all ProtectedVisibility mutates this to private, which

@wimleers wimleers Aug 18, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Nine @infection-ignore-all annotations were added. The DUMP_DEPTH/DUMP_INDENT one is fair. This one and SasClientService.php:21 would both become observable via the executeCommand() tests above, and the SasConnectorFactory one is suppressing a true positive (see my comment on SasConnectorFactory.php:24). Worth rechecking which survive once the empty subclasses and dead branch are gone.

* is killed by the pull command overriding it, but Infection does not
* attribute the subclass test's coverage back to this base declaration.
*/
protected function onSuccess(SourceConfig $sourceConfig, string $operationId): int
{
$this->io->success($this->operationLabel() . ' completed successfully.');

return Command::SUCCESS;
}

protected function execute(InputInterface $input, OutputInterface $output): int
{
$this->setDirAndRequireProjectCwd($input);

$siteInstance = $this->determineSiteInstance($input);

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

The PR description says site identity is resolved from the git remote with no arguments needed. determineSiteInstance() (CommandBase.php:884-906) reads only --siteInstanceId and otherwise returns null, so line 106 throws unless that option is passed. acceptEnvironmentId() (line 69) registers an argument nothing reads.

CommandBase::determineEnvironment() (CommandBase.php:690-708) is the existing ladder: --siteInstanceIdenvironmentId argument → codebase environment → git-remote-matched application → prompt. It also normalises through EnvironmentTransformer, which would make commit 4768081 unnecessary.

All six execute() tests pass --siteInstanceId, which is why this wasn't caught.

if ($siteInstance === null) {
throw new AcquiaCliException(
'Could not determine a Source site instance. Run this command from a repository linked to an Acquia Cloud application, or pass --siteInstanceId.'
);
}

$environment = $siteInstance->environment;

if (!$input->getOption('force')) {
$answer = $this->io->confirm(
sprintf('%s on the %s environment?', $this->operationLabel(), $environment->name),
false,
);
if (!$answer) {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

⚠️ With -n and no --force, confirm(..., false) returns false and the command returns SUCCESS having done nothing. A CI job gets exit 0 and no sync.

DXBE-8: "failure exits non-zero with a plain-language error." Throw an exception when the session is non-interactive and --force wasn't given, naming --force.

That fixes the no-op locally. Exiting non-zero when the remote operation fails needs upstream first — point 2 of my review on acquia/sites-aggregation-service#971, since without a notification to poll the command can't know it failed.

return Command::SUCCESS;
}
}

$sourceConfig = new SourceConfig($this->sasClient->getClient());

$response = $this->triggerOperation($sourceConfig, $environment->id);
// @todo DXBE-20: Confirm the operation ID field name with the SAS team.
$operationId = $response->id ?? null;
Comment on lines +127 to +128

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

acquia/sites-aggregation-service#971 returns 202 Accepted with a Message and no id, so this throws an exception on every invocation. testExecuteThrowsWhenOperationIdMissing() is currently testing the production path.

Needs upstream first, then still changes here. Point 2 of my review on acquia/sites-aggregation-service#971 asks for config-sync to create a notification. If it does, the identifier comes off that notification rather than off the 202 body, so this block still has to be rewritten — it just stops being unfixable.

if (!is_string($operationId)) {
throw new AcquiaCliException('The SAS API response did not include an operation ID.');
}

$this->io->writeln(sprintf('%s submitted (operation %s). Waiting for it to complete...', $this->operationLabel(), $operationId));

if (!$this->waitForOperation($sourceConfig, $operationId)) {
return Command::FAILURE;
}

return $this->onSuccess($sourceConfig, $operationId);
}

/**
* Poll the operation until it leaves the in-progress states.
*
* @todo DXBE-20: Confirm the status field name and its values with the
* SAS team. Assumes a `status` field mirroring the task gateway's
* phases (pending/running/succeeded/failed).
*/
private function waitForOperation(SourceConfig $sourceConfig, string $operationId): bool
Comment on lines +145 to +149

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

GET /config-operation/{id} doesn't exist. acquia/sites-aggregation-service#971's description: "No notification record is created and no status polling is wired up, so the caller gets a 202 and no way to observe the outcome beyond the task log."

So getStatus(), waitForOperation() and the pending/running/succeeded/failed states are all guesses.

There is a real surface to aim at, though. SAS already exposes GET /sites/{siteId}/notifications, with status, progress and completed_at on each Notification — and Site::createBackup(), restoreBackup() and export() all create one. syncConfig() is the exception. So the shape is polling notifications once config-sync creates one, not a bespoke /config-operation/{id} resource.

Needs upstream first, then still changes here. Point 2 of my review on acquia/sites-aggregation-service#971 decides which: if config-sync creates a notification, this polls that instead of /config-operation/{id}; if fire-and-forget is the intended contract, this method comes out entirely. Either way the code changes.

{
$status = null;
$checkStatus = static function () use ($sourceConfig, $operationId, &$status): bool {
$response = $sourceConfig->getStatus($operationId);
$status = $response->status ?? 'unknown';
return !in_array($status, ['pending', 'running'], true);
};
$onDone = static function (): void {
};

// @infection-ignore-all The spinner message is transient (overwritten
// as the spinner advances) and never appears in the captured output,
// so its concatenation cannot be asserted by a test.
LoopHelper::getLoopy($this->output, $this->io, $this->operationLabel() . '...', $checkStatus, $onDone);

if ($status !== 'succeeded') {
$this->io->error(sprintf('%s ended with status: %s', $this->operationLabel(), $status));
}

return $status === 'succeeded';
}
}
112 changes: 112 additions & 0 deletions src/Command/Source/ConfigPullCommand.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,112 @@
<?php

declare(strict_types=1);

namespace Acquia\Cli\Command\Source;

use Acquia\Cli\Attribute\RequireAuth;
use Acquia\Cli\Exception\AcquiaCliException;
use Acquia\Cli\SasApi\SourceConfig;
use Symfony\Component\Console\Attribute\AsCommand;
use Symfony\Component\Console\Command\Command;
use Symfony\Component\Filesystem\Filesystem;
use Symfony\Component\Yaml\Yaml;

/**
* Trigger a Source config export on a site via the Sites Aggregation Service.
*
* Asks SAS to run a config export on the environment, which exports config
* from the CMS. The exported config comes back as a single YAML document keyed
* by collection, then config name; this command writes it out to
* .acquia/config/ as individual files, replacing whatever is there.
*/
#[RequireAuth]
#[AsCommand(name: 'source:config:pull', description: 'Export Source configuration from a site')]
final class ConfigPullCommand extends ConfigCommandBase
{
/**
* The directory (relative to the project root) config is written to.
*/
private const CONFIG_DIR = '.acquia/config';

@wimleers wimleers Aug 18, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

DXBE-8 requires a configurable target directory for both commands. A private const can't be configured — this wants an option defaulting to .acquia/config.

The option is needed here regardless. The push side additionally needs upstream first: the directory can't be communicated to SAS at all while acquia/sites-aggregation-service#971 takes no body — point 1 of my review on acquia/sites-aggregation-service#971. See my comment on SourceConfig::push().


/**
* The inline depth and indentation for dumped config YAML.
*
* @infection-ignore-all Increment/DecrementInteger on the depth is not
* observable: any depth beyond the config's actual nesting produces
* identical output, and the indent behavior is covered by the nested
* structure test. The values only need to be "deep enough".
*/
private const DUMP_DEPTH = 10;

private const DUMP_INDENT = 2;

protected function triggerOperation(SourceConfig $sourceConfig, string $environmentId): object
{
return $sourceConfig->pull($environmentId);
}

protected function operationLabel(): string
{
return 'Exporting configuration';
}

/**
* Fetch the exported payload and write it to .acquia/config/.
*
* The directory is wiped and rewritten so the local files mirror the
* remote state exactly — config removed in the CMS disappears locally too.
*/
protected function onSuccess(SourceConfig $sourceConfig, string $operationId): int
{
$yaml = $sourceConfig->getExportPayload($operationId);
$payload = Yaml::parse($yaml);

if (!is_array($payload)) {
throw new AcquiaCliException('The SAS API returned an invalid config payload.');
}

$this->writePayload($payload);

$this->io->success(sprintf('Configuration exported to %s.', self::CONFIG_DIR));

return Command::SUCCESS;
}

/**
* Wipe and rewrite .acquia/config/ from the payload.
*
* The payload maps collection names to config items. The default
* collection ("") writes to the config root; other collections write to
* dotted subdirectories (language.es becomes language/es).
*
* @param array<string, array<string, mixed>> $payload
*/
private function writePayload(array $payload): void
{
$configDir = $this->dir . '/' . self::CONFIG_DIR;
$filesystem = new Filesystem();

// Wipe the directory so the local files mirror the remote state.
$filesystem->remove($configDir);

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Pull is DXBE-8 scope, so not out of place — but acquia/sites-aggregation-service#971 adds only the import direction, so config-export and /payload have nothing to run against.

Not covered by my review on acquia/sites-aggregation-service#971 either — that asks about the push payload, not a config export endpoint. POST /sites/{siteId}/exports does exist, but it's a whole-site export (MysqlSiteExportAdapter), not CMS config. The export endpoint is DXBE-21, so this half is blocked on that landing first.

That matters because pull is the half that deletes files. testWritePayloadCreatesConfigDirWhenPayloadEmpty() shows it as a passing test: {} parses to [], passes the is_array() guard at line 65, wipes .acquia/config/ and writes nothing back. Any SAS-side bug returning an empty export destroys uncommitted local config, and the prompt says nothing about local deletion.

Suggest splitting pull into its own PR. If it stays: name the directory in the prompt, write to a temp dir and swap, and refuse to wipe on an empty payload.

$filesystem->mkdir($configDir);

foreach ($payload as $collection => $items) {
if (!is_array($items)) {
continue;
}
// The default collection ("") is the config root; other collections
// map their dotted name to a subdirectory (language.es -> language/es).
$collectionDir = $collection === ''
? $configDir
: $configDir . '/' . str_replace('.', '/', $collection);

foreach ($items as $name => $values) {
$filesystem->dumpFile(

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

⚠️ $name is a key from the decoded remote response, interpolated straight into a path, so ../ in a config name writes outside .acquia/config/ — arbitrary file write from a response body, and with dumpFile() overwriting, potential data loss outside the config directory too. Validate the name (basename(), or reject anything containing a separator) before building the path.

sprintf('%s/%s.yml', $collectionDir, $name),
Yaml::dump($values, self::DUMP_DEPTH, self::DUMP_INDENT),
);
}
}
}
}
30 changes: 30 additions & 0 deletions src/Command/Source/ConfigPushCommand.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,30 @@
<?php

declare(strict_types=1);

namespace Acquia\Cli\Command\Source;

use Acquia\Cli\Attribute\RequireAuth;
use Acquia\Cli\SasApi\SourceConfig;
use Symfony\Component\Console\Attribute\AsCommand;

/**
* Trigger a Source config import on a site via the Sites Aggregation Service.
*
* Asks SAS to run `drush source:config:import` on the environment, which
* reads config from the site's git repository and applies it to the CMS.
*/
#[RequireAuth]
#[AsCommand(name: 'source:config:push', description: 'Import deployed Source configuration on a site')]
final class ConfigPushCommand extends ConfigCommandBase
{
protected function triggerOperation(SourceConfig $sourceConfig, string $environmentId): object
{
return $sourceConfig->push($environmentId);

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

DXBE-8: "Push can point at an arbitrary prior Git commit to restore that state (revert path)." There's no option for it.

To be clear about the mechanism: the rollback in acquia/haas-drupal#2233 is dr checkpoint:create / checkpoint:revert, but that's automatic recovery inside one import — the checkpoint is made in step 4 and reverted only if that same import fails. It isn't a user-selectable restore point, and acquia/sites-aggregation-service#971 exposes no way to list checkpoints or revert to a chosen one.

The workable route is the payload: git show <ref>:.acquia/config/ locally, sent as --single-yaml. That needs upstream first — point 1 of my review on acquia/sites-aggregation-service#971 — but upstream alone doesn't deliver this. Once a body is accepted, this PR still has to add the --ref option, read the config out of that commit, and serialise it. Upstream unblocks it; it doesn't implement it.

}

protected function operationLabel(): string
{
return 'Importing configuration';
}
Comment on lines +26 to +29

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

operationLabel() is the whole of the push wording, and ConfigCommandBase.php:116 renders it as Importing configuration on the <env> environment?. Per acquia/sites-aggregation-tasks#261, confirming it puts the site in maintenance mode, backs up the database, checkpoints config, imports, and on failure reverts the checkpoint or restores the database. acquia/sites-aggregation-service#971 opens its risk section with "This triggers a destructive operation on a live site … The site is offline for the duration."

Someone pointing this at production can't tell from that sentence that the site goes down. EnvMirrorCommand.php:61 and PushDatabaseCommand.php:48 are the models — they name what gets overwritten. Please say that the site goes into maintenance mode and is unavailable while this runs, and that a database backup is taken first.

Anchoring here because the warning is push-specific: pull doesn't take the site offline. But the base's sprintf('%s on the %s environment?', ...) template only has room for a short label, so either that grows or the base needs a hook for the extra warning text.

}
16 changes: 16 additions & 0 deletions src/SasApi/SasClient.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
<?php

declare(strict_types=1);

namespace Acquia\Cli\SasApi;

use AcquiaCloudApi\Connector\Client;

/**
* Client for the Sites Aggregation Service (SAS) API.
*
* Response processing is inherited unchanged from the Cloud API client.
*/
class SasClient extends Client

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

class SasClient extends Client {}. AcsfClient exists because it overrides processResponse(); this overrides nothing. Use AcquiaCloudApi\Connector\Client directly — SasClientService is still the prophecy seam the tests need.

{
}
Loading
Loading