Symfony - `storage_filename_strategy` configuration not evaluated

[MaSpeng]

The Symfony integration as of version 2.0 of this bundle does not evaluate the storage_filename_strategy and connections.<connection_name>.storage.filename_strategy configuration.

The service db_tools.storage will be configured with strategies resolved from the deprecated configuration path db_tools.storage.filename_strategy at

DbToolsBundle/src/Bridge/Symfony/DependencyInjection/DbToolsExtension.php

Lines 88 to 109 in d0cb632

	$strategies = [];
	foreach (($config['storage']['filename_strategy'] ?? []) as $connectionName => $strategyId) {
	// Default is handled directly by the storage service.
	if ($strategyId !== null && $strategyId !== 'default' && $strategyId !== 'datetime') {
	if ($container->hasDefinition($strategyId)) {
	$strategies[$connectionName] = new Reference($strategyId);
	} elseif (\class_exists($strategyId)) {
	if (!\is_subclass_of($strategyId, FilenameStrategyInterface::class)) {
	throw new InvalidArgumentException(\sprintf('"db_tools.connections.%s.filename_strategy": class "%s" does not implement "%s"', $connectionName, $strategyId, FilenameStrategyInterface::class));
	}
	$serviceId = '.db_tools.filename_strategy.' . \sha1($strategyId);
	$container->setDefinition($serviceId, (new Definition())->setClass($strategyId));
	$strategies[$connectionName] = new Reference($serviceId);
	} else {
	throw new InvalidArgumentException(\sprintf('"db_tools.connections.%s.filename_strategy": class or service "%s" does not exist or is not registered in container', $connectionName, $strategyId));
	}
	break;
	}
	}
	if ($strategies) {
	$container->getDefinition('db_tools.storage')->setArgument(1, $strategies);
	}

Due to the initialization of the configuration object at

DbToolsBundle/src/Bridge/Symfony/DependencyInjection/DbToolsExtension.php

Line 115 in d0cb632

return new DbToolsConfiguration(true, false);

its not possible anymore to use this configuration path.

The configuration at db_tools.storage_filename_strategy and db_tools.connections.<connection_name>.storage_filename_strategy does not seem to be taken into account at all.

---

For anyone facing the same issue, as temporary a workaround, it is possible to update the definition of the service `db_tools.storage` through a custom compiler pass.

<?php

declare(strict_types=1);

namespace App\DependencyInjection\Compiler;

use Symfony\Component\DependencyInjection\Compiler\CompilerPassInterface;
use Symfony\Component\DependencyInjection\ContainerBuilder;
use Symfony\Component\DependencyInjection\Exception\LogicException;
use Symfony\Component\DependencyInjection\Reference;

class DbToolsCompilerPass implements CompilerPassInterface
{
    public function process(ContainerBuilder $container): void
    {
        /**
         * @var array<int, array{
         *      storage_filename_strategy?: 'default'|'datetime'|string,
         *      connections: array<string, array{
         *          storage_filename_strategy?: 'default'|'datetime'|string,
         *      }
         *  } $dbToolsConfigurations
         */
        $dbToolsConfigurations = $container->getExtensionConfig('db_tools');
        if (count($dbToolsConfigurations) > 1) {
            throw new LogicException('More than one db tool configuration defined.');
        }

        $dbToolsConfiguration = $dbToolsConfigurations[0];
        if ([] === $dbToolsConfiguration) {
            return;
        }

        $storageFilenameStrategies = [];

        $defaultStorageFilenameStrategy = $this->retrieveStorageFilenameStrategyFromConfiguration(
            $dbToolsConfiguration,
        );
        if (null !== $defaultStorageFilenameStrategy) {
            $storageFilenameStrategies['default'] = $defaultStorageFilenameStrategy;
        }

        if (array_key_exists('connections', $dbToolsConfiguration)) {
            foreach ($dbToolsConfiguration['connections'] as $connectionName => $connectionConfiguration) {
                $storageFilenameStrategy = $this->retrieveStorageFilenameStrategyFromConfiguration(
                    $connectionConfiguration,
                );
                if (null === $storageFilenameStrategy) {
                    continue;
                }

                $storageFilenameStrategies[$connectionName] = $storageFilenameStrategy;
            }
        }

        if ($storageFilenameStrategies === []) {
            return;
        }

        $dbToolsStorageDefinition = $container->getDefinition('db_tools.storage');

        $dbToolsStorageDefinition->replaceArgument(1, $storageFilenameStrategies);
    }

    /**
     * @param array{storage_filename_strategy?: 'default'|'datetime'|string} $configuration
     *
     * @return Reference|null
     */
    private function retrieveStorageFilenameStrategyFromConfiguration(array $configuration): ?Reference
    {
        if (!array_key_exists('storage_filename_strategy', $configuration)
            || in_array($configuration['storage_filename_strategy'], ['default', 'datetime'])
        ) {
            return null;
        }

        return new Reference($configuration['storage_filename_strategy']);
    }
}

[SimonMellerin]

Thanks a lot for the feedback, I'll try to fix it within the week.

[SimonMellerin]

Fixed and shipped in version 2.0.1.

@MaSpeng I let you confirm this is ok for you close this issue.

Just out of curiosity, what filename strategy do you implement? Do you think its generic enough to be added to the lib? or it's too specific to your use case?
Could your use case have been covered by a generic pattern-based filename strategy?

[MaSpeng]

@SimonMellerin Thanks for the new release, the new version is partially working:

The following configuration is working now: ✅

db_tools:
  connections:
    default:
      storage_filename_strategy: App\DbTools\Storage\CustomFilenameStrategy

Whats still not working as expected is: ❌

db_tools:
  storage_filename_strategy: App\DbTools\Storage\CustomFilenameStrategy

I would expect the top level configuration to be used for all available connections until i specify it explicitly for a connection, but it does not have any effect.

I would also expect that this configuration: ❌

db_tools:
  storage_filename_strategy: App\DbTools\Storage\CustomFilenameStrategy

  connections:
    not_default_connection:
      storage_filename_strategy: App\DbTools\Storage\AnotherCustomFilenameStrategy

would use the top most configured storage file name strategy for all connections except the override for the not_default_connection.

---

Our custom file name strategy is rather simple, we just need the backup to be stored as {filename}.{extension}, we actually want this backup to be overwritten by the next execution.

A pattern based filename strategy would solve this by using the available arguments of the \MakinaCorpus\DbToolsBundle\Storage\FilenameStrategyInterface::generateFilename function, for example {connectionName}.{extension}.

A more flexible approach using a set of predefined placeholder in conjunction with a pattern to be able to use sub directories would be more flexible indeed.

[SimonMellerin]

Thank you for your vigilance! Should be good with version 2.0.1

We may consider to add this kind of pattern based filename strategy. Seems a nice way to easily implement a simple custom strategy.

cheers

[MaSpeng]

@SimonMellerin Thanks for the new release, the new version is partially working:

The following configuration is working, the connection specific configured strategy is used for the connection: ✅

db_tools:
  connections:
    default:
      storage_filename_strategy: App\DbTools\Storage\CustomFilenameStrategy

The following configuration is also working, the connection specific configured strategy is used for the connection, the top level configured strategy is used for all other configured connections: ✅

db_tools:
  storage_filename_strategy: App\DbTools\Storage\CustomFilenameStrategy

  connections:
    default:
      ...
    not_default_connection:
      storage_filename_strategy: App\DbTools\Storage\AnotherCustomFilenameStrategy

The following configuration is not working as expected: ❌

db_tools:
  storage_filename_strategy: App\DbTools\Storage\CustomFilenameStrategy

I would expect this storage filename strategy to be applied for all auto discovered connections (default in most cases), but it is required to add a connection configuration to make it work as seen in the second working configuration example above.

This is happening as the fallback to the top most configured strategy is done inside the foreach loop at:

DbToolsBundle/src/Bridge/Symfony/DependencyInjection/DbToolsExtension.php

Line 91 in 3b8d305

$strategyId = $connectionConfig['storage_filename_strategy'] ?? $config['storage_filename_strategy'] ?? null;

As a result, when the developer is relying on the automatic discovery of the connections based on the configured doctrine connections, the top level strategy will not be applied to any of those.