Skip to content

Symfony 5 parameter fix #295

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

Closed
wants to merge 2 commits into from
Closed
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
2 changes: 1 addition & 1 deletion Command/DoctrineCommand.php
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,7 @@ public static function configureMigrations(ContainerInterface $container, Config
$configuration->setMigrationsDirectory($dir);
} else {
// class Kernel has method getKernelParameters with some of the important path parameters
$pathPlaceholderArray = ['kernel.root_dir', 'kernel.cache_dir', 'kernel.logs_dir'];
$pathPlaceholderArray = ['kernel.project_dir', 'kernel.cache_dir', 'kernel.logs_dir'];
Copy link
Contributor

@dmaicher dmaicher Jan 17, 2020

Choose a reason for hiding this comment

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

I think this can be a BC break? The code below seems to replace/resolve parameter placeholders with kernel parameters. So now this will not work anymore if my Symfony 3.4 project relies on %kernel.root_dir% to be replaced here?

Copy link
Contributor

Choose a reason for hiding this comment

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

So I think for now we can just add kernel.project_dir to the array but also keep kernel.root_dir. There is a check below that ensures the kernel parameter exists.


foreach ($pathPlaceholderArray as $pathPlaceholder) {
if (! $container->hasParameter($pathPlaceholder) || ! preg_match('/\%' . $pathPlaceholder . '\%/', $dir)) {
Expand Down
2 changes: 1 addition & 1 deletion DependencyInjection/Configuration.php
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,7 @@ public function getConfigTreeBuilder() : TreeBuilder

$rootNode
->children()
->scalarNode('dir_name')->defaultValue('%kernel.root_dir%/DoctrineMigrations')->cannotBeEmpty()->end()
->scalarNode('dir_name')->defaultValue('%doctrine.migrations.dir%')->cannotBeEmpty()->end()
Copy link
Contributor

Choose a reason for hiding this comment

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

to stick with the naming of the other parameters I would name this one %doctrine_migrations.default_dir_name%

->scalarNode('namespace')->defaultValue('Application\Migrations')->cannotBeEmpty()->end()
->scalarNode('table_name')->defaultValue('migration_versions')->cannotBeEmpty()->end()
->scalarNode('column_name')->defaultValue('version')->end()
Expand Down
6 changes: 6 additions & 0 deletions DependencyInjection/DoctrineMigrationsExtension.php
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,12 @@ public function load(array $configs, ContainerBuilder $container) : void
{
$configuration = new Configuration();

if ($container->hasParameter('kernel.root_dir')) {
$container->setParameter('doctrine.migrations.dir', '%kernel.root_dir%/DoctrineMigrations');
} else {
$container->setParameter('doctrine.migrations.dir', '%kernel.project_dir%/src/Migrations');
Copy link
Member

Choose a reason for hiding this comment

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

Sorry for the late response.

As far as I can see this would still be a BC break for someone having an application that uses the old directory layout (i.e. they only updated their dependencies, but didn't adapt the directory structure established with Symfony Flex).

I think the only safe upgrade path is to deprecate not configuring this option and making it a mandatory one in the next major release.

Copy link
Contributor

Choose a reason for hiding this comment

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

Indeed @xabbuh is right 😢 This will break for people with the old directory structure that just move to Symfony 5 without adapting to the new structure.

Copy link
Member

Choose a reason for hiding this comment

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

I think the only safe upgrade path is to deprecate not configuring this option and making it a mandatory one in the next major release.

I'll defer this to @goetas, since the next major release will change the config format anyways. We may just want to ignore this for now and fix it properly in 3.0.

}

$config = $this->processConfiguration($configuration, $configs);

foreach ($config as $key => $value) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,7 @@ private function getContainer() : ContainerBuilder
'kernel.bundles' => [],
'kernel.cache_dir' => sys_get_temp_dir(),
'kernel.environment' => 'test',
'kernel.root_dir' => __DIR__ . '/../../', // src dir
'kernel.project_dir' => __DIR__ . '/../../', // src dir
]));
}
}