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

Using PdoDatabase? #248

Closed
andrewteg opened this issue Mar 4, 2021 · 3 comments
Closed

Using PdoDatabase? #248

andrewteg opened this issue Mar 4, 2021 · 3 comments

Comments

@andrewteg
Copy link

Wow, this library looks great. I'm curious if for a new project there are any reasons not to wrap the $db in a PdoDatabase::fromDsn class.

I started with this from the docs:

$db = new \Delight\Db\PdoDsn('pgsql:dbname=my-database;host=localhost;port=5432', 'my-username', 'my-password');

I wanted to insert something custom into user_profiles so I setup code like this and hoped to just use the existing connection to insert:

try { $user_data = array('id' => $userId, 'first_name' => $_POST['first_name'], 'last_name' => $_POST['last_name']); $db->insert('users_profiles', $user_data); } catch (Exception $e) { die('Invalid profile insert'); }

That threw a "Fatal error: Uncaught Error: Call to undefined method Delight\Db\PdoDsn::insert()" and I realized wrapping it in a PdoDatabase::fromDsn allows me to use the $db classes like $db->insert():

$db = \Delight\Db\PdoDatabase::fromDsn(new \Delight\Db\PdoDsn('pgsql:dbname=my-database;host=localhost;port=5432', 'my-username', 'my-password'));

So I guess I'm asking (1) why isn't that the preferred way and (2) is there anything dangerous about that on a new project?

Thanks.

@ocram
Copy link
Contributor

ocram commented Mar 4, 2021

Thank you!

There’s absolutely no difference between the two ways of creating or accessing the database instance, and thus nothing dangerous, either. In fact, if you only pass the configuration without creating the database instance yourself, the library will just create the instance in the next step – so the outcome is exactly the same. The only difference is whether you care about the database instance (and want to use it yourself) or just create it for this library.

But you made an excellent point: Given that many developers would like to use the database instance for something else as well, and given that there’s no downside (in particular with regard to performance or resource usage), the default should be changed. This has been done now: f5060b5

Thanks again!

@ocram ocram closed this as completed Mar 4, 2021
@andrewteg
Copy link
Author

I'm developing this out and found an instance where FETCH_KEY_PAIR would be nice for a custom users table with a user options stored, and I didn't see KEY PAIR idea in the DB class. I was wondering if you had a reason for not including that or anyone knew a better way to get key/value pairs from the DB class than just a foreach after the select like this:

$data = $db->select('SELECT id, val FROM users_custom_table WHERE user_id = ?', [$user_id]);
foreach ($data AS $row) {
	$profile['data'][$row['id']] = $row['val'];
}

Thanks again, and thanks for any ideas or tips,
Andrew

@ocram
Copy link
Contributor

ocram commented Apr 14, 2021

It seems this existing issue for the database component seems to address what you describe. It needs to be implemented in that component. But this should really be done shortly, as it’s an important and overdue feature.

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

No branches or pull requests

2 participants