Skip to content

Re-add common PDO tests to Firebird test suite #4112

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 1 commit into from

Conversation

cmb69
Copy link
Member

@cmb69 cmb69 commented May 4, 2019

These have been inadvertently dropped when changing the test suite to
not require ext/interbase anymore, so we add them back.

We also change the required environment variable names to match the
usual PDO names. Particularly, we replace PDO_FIREBIRD_TEST_HOSTNAME
and _DATABASE with the more flexible PDO_FIREBIRD_TEST_DSN.

These have been inadvertently dropped when changing the test suite to
not require ext/interbase anymore, so we add them back.

We also change the required environment variable names to match the
usual PDO names.  Particularly, we replace `PDO_FIREBIRD_TEST_HOSTNAME`
and `_DATABASE` with the more flexible `PDO_FIREBIRD_TEST_DSN`.
define('PDO_FIREBIRD_TEST_PASSWORD', getenv('PDO_FIREBIRD_TEST_PASSWORD') ?: 'phpfi');
define('PDO_FIREBIRD_TEST_HOSTNAME', getenv('PDO_FIREBIRD_TEST_HOSTNAME') ?: 'localhost');
define('PDO_FIREBIRD_TEST_DATABASE', getenv('PDO_FIREBIRD_TEST_DATABASE') ?: '');
define('PDO_FIREBIRD_TEST_USER', getenv('PDO_FIREBIRD_TEST_USER') ?: 'SYSDBA');
Copy link
Contributor

Choose a reason for hiding this comment

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

How about using the following code snippets:

......
 getenv('PDO_FIREBIRD_TEST_USER') ?? 'SYSDBA';
......

It's the PHP 7 syntax.

Copy link
Member

Choose a reason for hiding this comment

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

@peter279k we can’t as getenv() returns false, not null

Copy link
Contributor

@peter279k peter279k May 4, 2019

Choose a reason for hiding this comment

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

Ok, using the ?: syntax to detect condition if it is false.

$dbh = new PDO(PDO_FIREBIRD_TEST_DSN, PDO_FIREBIRD_TEST_USER, PDO_FIREBIRD_TEST_PASS) or die;
Copy link
Contributor

Choose a reason for hiding this comment

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

If the DB connection is failed, I think we need to output some notification message via die function.

It can make use have note that the DB connection status message.

Copy link
Member

Choose a reason for hiding this comment

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

PDO already yields an error message, die simply prevents further execution

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok. It's fine to use die function.

@cmb69 cmb69 requested a review from KalleZ May 4, 2019 08:56
@KalleZ
Copy link
Member

KalleZ commented May 4, 2019

Ah I wasn't aware of this magic I removed with PDO, in that case lets go for it. I think its a more elegant solution with the DSN environment variable too

@cmb69
Copy link
Member Author

cmb69 commented May 4, 2019

Thanks for reviewing, @KalleZ! Applied as e4757ec.

@cmb69 cmb69 closed this May 4, 2019
@cmb69 cmb69 deleted the pdo-fb-common branch May 4, 2019 12:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants