Page MenuHomePhabricator

Remove direct calls to Database::query() and use inbuilt methods instead.
Open, MediumPublic

Description

In 99% of cases, there should be no benefit from using raw sql over the nicer inbuilt methods which build the queries more programmatically.

Essentially a prequisite to doing any sort of DB schema abstraction (as Chad wanted to do in the near future).

Goes for both phase3 and extensions...


Version: 1.17.x
Severity: enhancement

Details

Reference
bz26670

Event Timeline

bzimport raised the priority of this task from to Medium.Nov 21 2014, 11:18 PM
bzimport added a project: Wikimedia-Rdbms.
bzimport set Reference to bz26670.
bzimport added a subscriber: Unknown Object (MLST).
Reedy created this task.Jan 11 2011, 7:35 PM

happy.melon.wiki wrote:

Should this be "Deprecate Database::query()"??

If so, there are 99 uses in /phase3 (65 of them in /maintenance), and ~380 more in /extensions.

Is this a tracking bug? Are you guys offering to use this bug as a rallying point for your own efforts?

happy.melon.wiki wrote:

(In reply to comment #3)

Is this a tracking bug? Are you guys offering to use this bug as a rallying
point for your own efforts?

That's why I asked. Something specific like "deprecate Database::query()" would be a specific achievable goal; the current title sounds more like a tracking bug...

Deprecating it isn't quite going to work, I think. In the end, it'll end up being probably a protected method, as we the way we do it is build a query string from the array, and then dump that out, maybe some refactoring relatedly.

It's sort of a tracking bug, but also not. Unless we're gonna start doing numerous bugs "Fix raw sql building in X", "Fix raw sql building in Y".

I know Roan and myself (among others), are slowly working through them

Also not sure if Chad has logged a feature request to move to a more abstract schema.. Either way, this would be a blocking bug for it

Certainly, having a more targeted approach wouldn't be a bad thing

Can I just say,
I was in your trenches a minute ago and the OP was not judging too harshly. A few recommendations for minor recoding are listed below.

Great work fighting bloat in general--the code is not in any risk of becoming a BusinessObjectModel, leave that powerpoint in the suitcase thank you.

2c..

  • Get all that DB_SLAVE logic outta here. Instead, use flags which indicate when stale data is tolerable.
  • Separating "bean" accessors and factory stuffing from app logic.
  • Do object fields and database rows really have to be named differently?? If you accepted the dual object/row convention, here is one small, immediate win in not reenumerating SELECT fields: function Article::pageData() { $row = $dbr->selectRow(page_table_name, Article::newNullArticle(), $conditions); }

(And then the real fun can be had: giving internals and calculated fields crazy, hidden names... ___'s the limit!)

  • Type checking and coercion don't seem necessary. Cannot the database constraints and driver do their job?

and more abstractly,

  • Imagine the poor guy trying to write an extension into or onto the data layer. Reducing the points of entry is key. I think I saw some GOTO statements somewhere in there...

When I read the bug title I was so going to close this bug as wontfix, because suckiness is a favorable feature of raw SQL :)

Taking this bug for the core, at least in a first time.

(In reply to comment #8)

Taking this bug for the core, at least in a first time.

Dereckson: Still working on this, or should this be reset to NEW/nobody?

Dereckson: I am resetting the assignee of this issue to default because there has been no progress in the last months. Feel free to take it again when you are actually planning to fix this. Thanks.

Krinkle set Security to None.
Krinkle removed a subscriber: Unknown Object (MLST).
awight removed a subscriber: awight.Mar 21 2019, 4:06 PM