Skip to content

Use DSN-like strings to define credentials #572

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 5 commits into from
Closed

Use DSN-like strings to define credentials #572

wants to merge 5 commits into from

Conversation

javiereguiluz
Copy link
Member

@javiereguiluz
Copy link
Member Author

I need someone to explain me why I needed to make this change --> f3d8340 to make this work. Thanks!

@stof
Copy link
Member

stof commented May 30, 2017

Well, your second commit is because what you defined was not a URL-based DSN for SQLite. It was only a path.
If you want to use it in url, use sqlite://%kernel.project_dir%/...

@javiereguiluz
Copy link
Member Author

@stof that was the problem! I'm an idiot 😊

However, if I change that, I now see this:

Symfony\Component\DependencyInjection\Exception\EnvParameterException:
Incompatible use of dynamic environment variables "DATABASE_URL" found in parameters.

symfony-demo/vendor/symfony/symfony/src/Symfony/Component/DependencyInjection/Dumper/PhpDumper.php:182
symfony-demo/vendor/symfony/symfony/src/Symfony/Component/HttpKernel/Kernel.php:743
symfony-demo/vendor/symfony/symfony/src/Symfony/Component/HttpKernel/Kernel.php:569
symfony-demo/vendor/symfony/symfony/src/Symfony/Component/HttpKernel/Kernel.php:119
symfony-demo/vendor/symfony/symfony/src/Symfony/Bundle/FrameworkBundle/Test/KernelTestCase.php:157
symfony-demo/tests/AppBundle/Command/AddUserCommandTest.php:104
symfony-demo/tests/AppBundle/Command/AddUserCommandTest.php:41

@stof
Copy link
Member

stof commented May 30, 2017

hmm, this message is not clear

@voronkovich
Copy link
Contributor

@javiereguiluz, dumper throws an exception because you don't use the DATABASE_URL variable in the test environment. See https://github.com/symfony/symfony/blob/v3.3.0/src/Symfony/Component/DependencyInjection/Dumper/PhpDumper.php#L182

@voronkovich
Copy link
Contributor

@javiereguiluz, see @dmaicher's comment #557 (comment)

@javiereguiluz
Copy link
Member Author

@voronkovich thanks for your comments. That was indeed the error and the proposed fix solved the issue. Thanks!

phpunit.xml.dist Outdated
@@ -10,6 +10,7 @@
<php>
<ini name="error_reporting" value="-1" />
<server name="KERNEL_DIR" value="app/" />
<env name="DATABASE_URL" value="sqlite://%kernel.project_dir%/var/data/blog_test.sqlite"/>
Copy link
Member

Choose a reason for hiding this comment

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

I don't think you can use a parameter here, in an actual env var

Copy link
Contributor

@dmaicher dmaicher May 30, 2017

Choose a reason for hiding this comment

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

Actually you are right but it still works 😄

Really weird what happens now when dumping the connection within a test to see the config:

  -_params: array:9 [
    "url" => "sqlite://%kernel.project_dir%/var/data/blog_test.sqlite"
    "host" => "%kernel.project_dir%"
    "port" => null
    "user" => "root"
    "password" => null
    "driver" => "pdo_sqlite"
    "driverOptions" => []
    "defaultTableOptions" => []
    "path" => "var/data/blog_test.sqlite"
  ]

This seems to be more correct:

<env name="DATABASE_URL" value="sqlite:///var/data/blog_test.sqlite" />

==>

  -_params: array:9 [
    "url" => "sqlite:///var/data/blog_test.sqlite"
    "host" => "localhost"
    "port" => null
    "user" => "root"
    "password" => null
    "driver" => "pdo_sqlite"
    "driverOptions" => []
    "defaultTableOptions" => []
    "path" => "var/data/blog_test.sqlite"
  ]

Copy link
Contributor

Choose a reason for hiding this comment

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

@javiereguiluz FYI see above

Copy link
Member Author

@javiereguiluz javiereguiluz May 30, 2017

Choose a reason for hiding this comment

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

@dmaicher you are right. I fixed that in 464e786 but I think Christophe is right and container params don't work here. I partially fixed what you say ... but I'm going to test your full solution now. Thanks!

Copy link
Contributor

Choose a reason for hiding this comment

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

@javiereguiluz
Copy link
Member Author

Thank you all for your help making this mergeable!

javiereguiluz added a commit that referenced this pull request May 31, 2017
… (voronkovich)

This PR was merged into the master branch.

Discussion
----------

Override DATABASE_URL by setting default parameter's value

This approach is more flexible than we used in #572

Commits
-------

9ea6fc8 Override DATABASE_URL by setting default parameter's value
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants