Skip to content

Conversation

@p4veI
Copy link
Contributor

@p4veI p4veI commented Feb 24, 2022

I was thinking if we could use dotenv loader for the dev/test run setup, I know I can set env vars in IDE but I consider this better.

closes #267

@staabm
Copy link
Owner

staabm commented Feb 24, 2022

good catch. this will close #267

@staabm
Copy link
Owner

staabm commented Feb 25, 2022

running composer test with this PR I get:

$ composer test
> phpunit -c tests/default/config/phpunit.xml
PHPUnit 9.5.16 by Sebastian Bergmann and contributors.

Error in bootstrap script: Error:
mysqli object is already closed
PHP Warning:  Undefined array key "DBA_HOST" in C:\dvl\Workspace\phpstan-dba\tests\ReflectorFactory.php on line 29
PHP Warning:  Undefined array key "DBA_USER" in C:\dvl\Workspace\phpstan-dba\tests\ReflectorFactory.php on line 30
PHP Warning:  Undefined array key "DBA_PASSWORD" in C:\dvl\Workspace\phpstan-dba\tests\ReflectorFactory.php on line 31
PHP Warning:  Undefined array key "DBA_DATABASE" in C:\dvl\Workspace\phpstan-dba\tests\ReflectorFactory.php on line 32
PHP Warning:  Undefined array key "DBA_MODE" in C:\dvl\Workspace\phpstan-dba\tests\ReflectorFactory.php on line 33
PHP Warning:  Undefined array key "DBA_REFLECTOR" in C:\dvl\Workspace\phpstan-dba\tests\ReflectorFactory.php on line 34
PHP Warning:  mysqli::__construct(): (HY000/1045): Access denied for user ''@'192.168.50.51' (using password: NO) in C:\dvl\Workspace\phpstan-dba\tests\ReflectorFactory.php on line 54
Script phpunit -c tests/default/config/phpunit.xml handling the phpunit event returned with error code 1
Script @phpunit was called via test

did you intentionally not put the defaults we had before into the .env.dist file?

@p4veI
Copy link
Contributor Author

p4veI commented Feb 27, 2022

@staabm sort of, I originally wanted to keep the defaults in the reflector factory, so it's only required for other connections and modes, but it I could use the default to null with default postgres instances..

maybe I'll default everything to null so it's more clear the defined env is required...

Probably could add this to library docs as well and describe the dev setup...

@staabm
Copy link
Owner

staabm commented Feb 27, 2022

Whether the consumer of phpstan-dba uses dotenv (or a different impl) or no dotenv at all is not subject of phpstan-dba.

Its the choice of the user

@p4veI
Copy link
Contributor Author

p4veI commented Feb 28, 2022

I've cleaned up the setup - now if you don't have IDE env vars setup it should default to what you have in .env, hope that's what you were suggesting.


require_once __DIR__.'/../../../vendor/autoload.php';

if (false === getenv('GITHUB_ACTION')) {
Copy link
Owner

@staabm staabm Feb 28, 2022

Choose a reason for hiding this comment

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

I don't think we need this IF-case arround the loading.


another question: does the dotenv class automatically load the .env.dist file now?
I tried running this PR locally and I am still getting errors

$ composer test
> phpunit -c tests/default/config/phpunit.xml
PHPUnit 9.5.16 by Sebastian Bergmann and contributors.

Error in bootstrap script: RuntimeException:
Unknown mode:
PHP Warning:  Undefined array key "DBA_HOST" in C:\dvl\Workspace\phpstan-dba\tests\ReflectorFactory.php on line 29
PHP Warning:  Undefined array key "DBA_USER" in C:\dvl\Workspace\phpstan-dba\tests\ReflectorFactory.php on line 30
PHP Warning:  Undefined array key "DBA_PASSWORD" in C:\dvl\Workspace\phpstan-dba\tests\ReflectorFactory.php on line 31
PHP Warning:  Undefined array key "DBA_DATABASE" in C:\dvl\Workspace\phpstan-dba\tests\ReflectorFactory.php on line 32
PHP Warning:  Undefined array key "DBA_MODE" in C:\dvl\Workspace\phpstan-dba\tests\ReflectorFactory.php on line 33
PHP Warning:  Undefined array key "DBA_REFLECTOR" in C:\dvl\Workspace\phpstan-dba\tests\ReflectorFactory.php on line 34

does it work for you locally?

Copy link
Owner

Choose a reason for hiding this comment

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

ahh I get it now. I need to copy the .env.dist file to .env once.. it works now

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeah, the .dist file is supposed to be like a template for the envs..

But it's commonly used so some defaults / test setup can be blaced in dist and setup from the existing file..

Copy link
Owner

Choose a reason for hiding this comment

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

I see. I am used to a process where the .dist file is automtically picked up, without the need to copy it a priori.

but thats fine.

thanks for the work you put into it here. I will merge it.

@staabm staabm merged commit 91ceba1 into staabm:main Feb 28, 2022
@staabm
Copy link
Owner

staabm commented Feb 28, 2022

thank you

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

tests: extract database access credentials into dot-env files

3 participants