Skip to content
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

Update CreateDatabaseDoctrineCommand.php #798

Closed
wants to merge 1 commit into from

Conversation

themizzi
Copy link

@themizzi themizzi commented Apr 5, 2018

Setting the path dynamically, which is useful to switch between in memory and path based SQLite dbs, with an env var requires setting the key path on the params. Isset returns true in this case even if the path is empty. !empty is a better check with this in mind.

Setting the path dynamically, which is useful to switch between in memory and path based SQLite dbs, with an env var requires setting the key `path` on the params. Isset returns true in this case even if the path is empty. `!empty` is a better check with this in mind.
@@ -72,7 +72,7 @@ protected function execute(InputInterface $input, OutputInterface $output)
}
}

$hasPath = isset($params['path']);
$hasPath = !empty($params['path']);
Copy link
Member

Choose a reason for hiding this comment

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

Could the same not be archived with "isset($params['path']) && $params['path'] !== ''" ?

That way we can avoid empty() :)

Copy link
Member

@Ocramius Ocramius left a comment

Choose a reason for hiding this comment

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

This kind of change requires test additions

@kimhemsoe
Copy link
Member

@themizzi Are you able to provide the needed changes?

@ostrolucky ostrolucky self-assigned this Apr 6, 2019
@ostrolucky ostrolucky added Bug and removed Bug labels Apr 6, 2019
@alcaeus alcaeus assigned alcaeus and unassigned ostrolucky and themizzi Apr 7, 2019
@alcaeus
Copy link
Member

alcaeus commented Apr 7, 2019

After taking another look, we've decided not to merge this. An empty path is very much valid in this context, which is why we use isset to check this. The correct solution would be to not pass an empty string, which can be an issue when dealing with environment variables.

Luckily, Symfony 4.3 will ship with a default: env var processor which allows you to fall back to a default value from a parameter, or receive null instead of an empty string. If you don't want to wait for Symfony 4.3 or can't upgrade when it is released, you can create an env var processor that does what Symfony 4.3 will ship.

@alcaeus alcaeus closed this Apr 7, 2019
@alcaeus alcaeus added the ⭐️ EUFOSSA Hackathon https://symfony.com/blog/the-symfony-and-api-platform-hackathon-is-coming label Apr 7, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
⭐️ EUFOSSA Hackathon https://symfony.com/blog/the-symfony-and-api-platform-hackathon-is-coming Won't Fix
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants