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

Do not remove DBTNG #48

Closed
chx opened this issue Sep 11, 2013 · 14 comments
Closed

Do not remove DBTNG #48

chx opened this issue Sep 11, 2013 · 14 comments

Comments

@chx
Copy link

chx commented Sep 11, 2013

Careful... even core had a security weakness from writing dynamic queries (it was in taxonomy module). Dynamic queries and security is challenging.

@mikl
Copy link

mikl commented Sep 11, 2013

I think we should be pretty safe from security issues if we just use real parameterized queries. I think that can be done with PDO.

@davereid
Copy link

Stated this in IRC, but worth reiterating here:

I find the goal of dropping DBTNG really hard to support. I've encountered so many random SQL oddities between MySQL and PostgreSQL (and SQLite) in modules that are just naturally taken care of by DBTNG and I would have had to do the same thing I did in Drupal 6: write more than one query in a switch statement based on db_driver(), which is crap. I actually enjoy using DBTNG but the fact that we have db_query() still available is good mix of both simple and the complex. I would highly encourage DBTNG to not be removed.

@mikl
Copy link

mikl commented Sep 11, 2013

Aye, parameterized queries are fine and dandy until you are working with a complex query being built and you find it'be nice if the parts of query being written and the parameters would pass together

You mean, like hook_query_alter()? I've always considered any use of that to be a terrible code smell and violation of the Open/closed principle. I shan't miss it :)

@quicksketch
Copy link
Member

@davereid I hear what you're saying about DB-specific queries, but if we can get a substantial performance boost (this needs to be proven of course) by removing DBTNG and it decreases learning curve at the same time, it's probably worth the odd 1-in-100 query that needs to be specific. I can't even get away from DB-specific queries today, even with DBTNG, though admittedly it's more like 1-in-1000 queries instead of 1-in-100. Example: https://drupal.org/node/2026891

@jenlampton
Copy link
Member

Also, if we can get views in core, maybe the views query builder can help with some of those queries that need to be alterable.

@chx
Copy link
Author

chx commented Sep 12, 2013

As some took this issue as my 'endorsment' or anything like that of backdrop, which is not the case, I have deleted all my comments from this repo and also closing this.

@chx chx closed this as completed Sep 12, 2013
@rudiedirkx
Copy link

Is DBTNG the db_select() -> SelectQuery and db_update() -> UpdateQuery etc? Don't remove that!

@quicksketch
Copy link
Member

For the flip side, see #61 for removing DBTNG.

If you don't want to remove DBTNG, please continue commenting here. :)

@alexweber
Copy link

Agree 100% with @davereid on this one, you don't necessarily have to use it in core but leave it in there for contrib :)

@rudiedirkx
Copy link

Will somebody explain what TNG entails exactly and why it's so very horrible named?

@mikl
Copy link

mikl commented Sep 18, 2013

@rudiedirkx: this is DBTNG, and the name is a Star Trek pun.

It's basically all the OOP query building stuff added in Drupal 7, like db_select(), db_insert() etc. Before DBTNG, almost all database interaction involved writing SQL and calling db_query().

@mkalkbrenner
Copy link

The discussion continues at #61

@rudiedirkx
Copy link

@mikl I understand the name. It's not very descriptive.

@mkalkbrenner The discussion continues here as well. That's why there are 2 issues.

I think DBTNG should stay. It's very useful for reusing sophisticated queries. You can have a base SelectQuery object and extend that with conditions and joins per use case. Sometimes writing SQL is much easier. Sometimes it's not.

@mikl
Copy link

mikl commented Sep 20, 2013

@rudiedirkx this issue is closed. Please move over to #61.

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

No branches or pull requests

8 participants