Joomla! Issue Tracker - CMS

Download
Launch
Feeling Lucky

[#856] - Cleaned up use/syntax of database & query objects

Pending

User tests: Successful: Unsuccessful:

avatar ghost
ghost
21 Mar 2013

Tracker item: http://joomlacode.org/gf/project/joomla/tracker/?action=TrackerItemEdit&tracker_item_id=30349

This pull request is a clean-up of core code.
Across the core different variable names and method names are used for the same thing. Like quote() and q() are ambiguous. But some code is using quote() and some is using q().

So this fix cleans up a lot of that, making sure the core uses the same ethod names and syntax for the database & query objects.

Changes

  • Changed use of variable name $dbo to $db
  • Changed use of JFactory::getDBO() method to JFactory::getDbo()
  • Changed use of variable name $db->setQuery((string) $query) to $db->setQuery($query)
  • Changed use of variable name $sql to $query
  • Changed use of quote() method to q()
  • Changed use of quoteName() method to qn()
  • Changed use of q() method to quote()
  • Changed use of qn() method to quoteName()
  • Removed use of quoteName() method where not necessary (aliased tables)
  • Changed $query->delete();$query->from(...); to $query->delete(...);
  • Changed leftJoin(...) to join('LEFT', ...)
  • Converted some string formatted sql statements to query objects
  • Changed adjacent single line $query statements to single multiline statements:
$query = $db->getQuery(true);
$query->select(...);
$query->from(...);
$query->where(...);

becomes:

$query = $db->getQuery(true)
   ->select(...)
   ->from(...)
   ->where(...);

And some general syntax cleaning (like brackets on new lines and spaces around concatenators).

Testing instructions

Seeing this touches many files, you will just have to test everything :)

avatar nonumber nonumber - open - 21 Mar 2013
avatar nicksavov
nicksavov - comment - 21 Mar 2013

Thanks for coding this, Peter!

Tracker item:
http://joomlacode.org/gf/project/joomla/tracker/?action=TrackerItemEdit&tracker_item_id=30349

avatar betweenbrain
betweenbrain - comment - 21 Mar 2013

This is great to standardize how things are coded. Is there any advantage in using $db->q() over $db->quote()? I ask as it seems more intuitive to use the longer named functions, like $db->quote(), as they are more descriptive.

avatar mbabker
mbabker - comment - 21 Mar 2013

There's no real benefit, just a preference honestly. q and qn are just
proxies to quote and quoteName. Some prefer the proxies and some prefer
the full name.

On Thursday, March 21, 2013, Matt Thomas wrote:

This is great to standardize how things are coded. Is there any advantage
in using $db->q() over $db->quote()? I ask as it seems more intuitive to
use the longer named functions, like $db->quote(), as they are more
descriptive.

—
Reply to this email directly or view it on GitHub#856 (comment)
.

avatar betweenbrain
betweenbrain - comment - 21 Mar 2013

If there's no benefit, I'd like to suggest that full names are used. A lot of people look at Joomla's code (and I wish more would) to learn how it works. While proxies are certainly more convenient to use, and possibly less prone to bugs due to typos (?), I think the benefit of clearer, more understandable code outweighs that.

Just my opinion of course ;-)

avatar mbabker
mbabker - comment - 21 Mar 2013

I know a couple people who would love to see the proxies killed off, but
that's a discussion for another day ;-)

Now that the hi-jacking is done, tests are welcome

On Thursday, March 21, 2013, Matt Thomas wrote:

If there's no benefit, I'd like to suggest that full names are used. A lot
of people look at Joomla's code (and I wish more would) to learn how it
works. While proxies are certainly more convenient to use, and possibly
less prone to bugs due to typos (?), I think the benefit of clearer, more
understandable code outweighs that.

Just my opinion of course ;-)

—
Reply to this email directly or view it on GitHub#856 (comment)
.

avatar realityking
realityking - comment - 21 Mar 2013

The proxies are slower too but it's probably beyond measureable ;)

avatar nicksavov
nicksavov - comment - 21 Mar 2013

FYI, looks like this pull request already can't be automatically merged, so another pull request likely got in that modifies some of this code.

avatar nonumber
nonumber - comment - 21 Mar 2013

So should the updated pull request use q/qn or qoute/quoteName?
I personally prefer the shorthand as it makes the important parts of the
queries clearer.

Also, how to avoid a large pull request like this getting unmergeable
within hours?

avatar mbabker
mbabker - comment - 21 Mar 2013

I'd put it on list to get a consensus, obviously there are going to be a
lot of opinions and I truly doubt there will be a solution all will agree
on.

As for the PR getting unmergable quickly, it's a side effect of constantly
committing and you having a large pull. The only two ways to guarantee it
would stay clean would be to not commit or commit it immediately.

On Thursday, March 21, 2013, Peter van Westen wrote:

So should the updated pull request use q/qn or qoute/quoteName?
I personally prefer the shorthand as it makes the important parts of the
queries clearer.

Also, how to avoid a large pull request like this getting unmergeable
within hours?

—
Reply to this email directly or view it on GitHub#856 (comment)
.

avatar nonumber
nonumber - comment - 21 Mar 2013

Should be merge-able again...

avatar elinw
elinw - comment - 21 Mar 2013

I have to say that overall the important thing is consistency. We can always do a systematic change from one to the other if we decide we are worrying more about microseconds of loading or size of package. But a standard style is very useful.

avatar nonumber
nonumber - comment - 21 Mar 2013

So my vote goes to going with q() and qn().
And like Elin says, if at a later stage we want to go to quote() and quoteName() globally, then we can always change then.

avatar nicksavov
nicksavov - comment - 21 Mar 2013

+1 to what Matt said. I like the full name too, since it's more descriptive.

avatar mbabker
mbabker - comment - 21 Mar 2013

I'll go with the status quo, I do personally prefer the full method name and would like to see less method proxies in the code, but I do understand what the proxies do and wouldn't have issues with their use.

avatar nonumber
nonumber - comment - 21 Mar 2013

I am ok to change it all to quote() and quoteName() too. Whatever people feel is best.
As long as we all use the same thing in core.

Personally I like the q() and qn(), as to me the functionality of these methods in not of great importance to the actual query. I mean to the readability of it.
When reading the code, you want to quickly see what the field names and values are that are being used in the query. And I don't care about understanding that every field name is being quoteNamed.

But I also get that we might want to get rid of the proxies.

avatar nicksavov
nicksavov - comment - 22 Mar 2013

It's out of sync again :) Probably better to wait to do this immediately after 3.1.0's release (one week from today), if it's likely to un-sync a lot of other pull requests. Plus it will give us a longer time to test (until 3.1.1) since this touches so many files.

Thoughts?

avatar nonumber
nonumber - comment - 22 Mar 2013

Ok, you guys convinced me :)

Changed all the q() and qn() uses to quote() and quoteName().

Also there were some uses of $query->quote() and $query->quoteName().
These are proxies for the $db->quote() and $db->quoteName().
So I also changed their usage to the $db methods.

So maybe it is a good rule to not use proxy methods in core. Only have those available for 3rd party extensions and backwards compatibility.

avatar elinw
elinw - comment - 23 Mar 2013

I'd be for doing this as part of the general clean up that we've been doing. Mainly though we'd need to scan for any errors and run the system tests.

As for staying in synch it is always an issue with code cleaning and we just have to work on getting things reviewed and merged promptly or we will never make any progress.

avatar infograf768
infograf768 - comment - 30 Mar 2013

"Changed all the q() and qn() uses to quote() and quoteName().

Also there were some uses of $query->quote() and $query->quoteName().
These are proxies for the $db->quote() and $db->quoteName().
So I also changed their usage to the $db methods."

As you changed this, would be a good idea to correct the description of the patch in the tracker.

avatar nonumber
nonumber - comment - 30 Mar 2013

Thanks. Done.

avatar mbabker
mbabker - comment - 30 Mar 2013

I've got a handful of commits in my fork for this pull (for some reason, I can't send a PR to you). Could you pull those in as well? Based on my testing, with those changes, everything should be good to go after that. Just need a couple more folks to test to make sure.

Thanks for taking this on, a lot of quality improvements in here.

avatar nonumber
nonumber - comment - 30 Mar 2013

Thanks Michael. Changes made.

avatar nonumber
nonumber - comment - 1 Apr 2013

@Elin: Fixed the if/else syntax in that file. But that file was full of brackets on same line. As with a lot of other files.
Maybe good to have a global syntax cleanup too (in some other PR, some other time).

I cleaned up this entire file: nonumber@701dd5a

avatar mbabker
mbabker - comment - 1 Apr 2013

Global code style cleanup is on the radar for post 3.1.0 already. But in instances where it's already correct, we'd prefer to not step backwards.

avatar nonumber
nonumber - comment - 1 Apr 2013

Yeah, my own automatic code cleanup got in the way (project setting was still set to my own preferred style).

avatar nonumber
nonumber - comment - 2 Apr 2013

If there is anything else than needs to be changed for this to get pulled, let me know...

Add a Comment

Login with GitHub to post a comment