? Pending

User tests: Successful: Unsuccessful:

avatar Wang-Yu-Chao
Wang-Yu-Chao
30 Apr 2018

Pull Request for Issue # .

Summary of Changes

Replace DatabaseDriver::q and DatabaseDriver::qn with complete name DatabaseDriver::quote and DatabaseDriver::quoteName.

Testing Instructions

Code review.

Expected result

None

Actual result

None

Documentation Changes Required

None

avatar Wang-Yu-Chao Wang-Yu-Chao - open - 30 Apr 2018
avatar Wang-Yu-Chao Wang-Yu-Chao - change - 30 Apr 2018
Status New Pending
avatar joomla-cms-bot joomla-cms-bot - change - 30 Apr 2018
Category Administration com_admin com_associations com_banners com_categories com_contact com_content com_fields com_finder com_installer com_joomlaupdate com_languages com_menus com_modules com_newsfeeds com_postinstall com_redirect com_users CLI Front End
avatar brianteeman
brianteeman - comment - 30 Apr 2018

What is the reason for this change?
How does it improve the code?
How do you address the issue of thousands of add ons that use q and qn

avatar dgrammatiko
dgrammatiko - comment - 30 Apr 2018

How does it improve the code?

It improves readability of the code

avatar Wang-Yu-Chao Wang-Yu-Chao - change - 30 Apr 2018
Labels Added: ?
avatar Wang-Yu-Chao
Wang-Yu-Chao - comment - 30 Apr 2018

@brianteeman A unified naming style may avoid misunderstandings IMHO

avatar zero-24
zero-24 - comment - 30 Apr 2018

How do you address the issue of thousands of add ons that use q and qn

No need to do that as q and qn are still working ;-) and as @dgrammatiko said it improves the readability and where if not in 4.0 should we start with that :-)

avatar brianteeman
brianteeman - comment - 30 Apr 2018

My mistake in reading the original comment I assumed that you meant you were removing the ability to use q and qn

So my question now is why is this for J4 and not for staging - there are no backwards compatible changes/breaks

avatar ReLater
ReLater - comment - 30 Apr 2018

It improves readability of the code

A very subjective opinion in my opinion.

avatar zero-24
zero-24 - comment - 30 Apr 2018

A very subjective opinion in my opinion.

What is your opinion than? ;) The last I was told about function names was that they should speak for itself ;) And In my opinion there quote and quoteName is better than q and qn even when they are shorter and you know what that mean. But think of an developer not original comming from joomla? $db->quoute(Name) would be easier to understand than $db->q(n) where you need to check what q / qn does ;)

avatar mbabker
mbabker - comment - 30 Apr 2018

It improves readability of the code

A very subjective opinion in my opinion.

There is nothing wrong with using shortcuts, but when working with a large distributed codebase like Joomla's it is better to use the descriptive method names versus shortcuts so people can pick up on things a bit more easily ($db->e() isn't really clear without reading the documentation, $db->escape() gives some context without having to read the documentation).

If you write your API well you don't even need doc blocks; well written code should be self documenting (to the extent practical with the language) and be able to stand alone from its documentation. My personal coding style outside Joomla these days is to omit doc blocks unless they add information that isn't already clear from the method signature (i.e. an explanation of the logic or array type information that most PHP documentation parsers and IDEs will help you with that PHP natively doesn't because you don't have typed arrays or generics in the language).

public function escape(string $contents): string;

/**
 * Method to escape a string
 *
 * @param string $contents The string to escape
 *
 * @return string The escaped string
 */
public function escape($contents);
avatar HLeithner
HLeithner - comment - 1 May 2018

Changing q() and qn() to quote() and quoteName() would be a performance gain too.

Also it would be nice to change $query->q/qn to $db->q/qn because JDatabaseQuery does an expensive __call magic + switch + function call + $db->quoteName function call

btw. we have a $query->e() function for escape, this should also be rewritten if we use it in the core.
(JDatabaseDriver doesn't have the 'e' alias)

avatar mbabker
mbabker - comment - 1 May 2018

Also it would be nice to change $query->q/qn to $db->q/qn because JDatabaseQuery does an expensive __call magic + switch + function call + $db->quoteName function call

Actually that's gone in 4.0 because we weren't dealing with undefined method call errors at all and I thought it was overkill to have a magic __call method for 2 aliases, so just made the aliases "real" methods. So there's still a small performance loss in using q() or qn() because it still results in 2 method calls, but not as bad as the existing code.

avatar izharaazmi
izharaazmi - comment - 2 May 2018

I'd much prefer using $db->q(), $db->qn() and $db->esc() .

It doesn't need to explicitly "speak for itself" as its fairly common enough. About the expensive magic call and switch, I already had some discussion in the past with @mbabker that we should instead have a proper declaration of these shortcuts than via magic calls, which he mentioned in a comment above again.

I won't comment about the $query->q and $query->qn though as I have no opinion as such right now.

avatar brianteeman
brianteeman - comment - 9 May 2018

@Wang-Yu-Chao can you update with the requests from @mbabker and fix the merge conflict please

avatar HLeithner
HLeithner - comment - 9 May 2018

If this PR is accepted we should adjust https://developer.joomla.org/coding-standards/php-code.html too.

avatar HLeithner
HLeithner - comment - 4 Jun 2018

@Wang-Yu-Chao any chance to get an update on this?

avatar brianteeman
brianteeman - comment - 24 Jul 2018

@Wang-Yu-Chao Please can you update this to address the issues raised and fix the conflicts

avatar Wang-Yu-Chao
Wang-Yu-Chao - comment - 24 Jul 2018

Sorry folks I missed this issue. I'm getting on with it now.

avatar joomla-cms-bot joomla-cms-bot - change - 24 Jul 2018
Category Administration com_admin com_associations com_banners com_categories com_contact com_content com_fields com_finder com_installer com_joomlaupdate com_languages com_menus com_modules com_newsfeeds com_postinstall com_redirect com_users CLI Front End Administration com_admin com_associations com_categories com_contact com_content com_fields com_finder com_installer com_joomlaupdate com_languages com_menus com_modules com_newsfeeds com_postinstall com_redirect com_users CLI Front End
avatar brianteeman
brianteeman - comment - 8 Aug 2018

Sorry it looks like there are conflicts again that need fixing
and you still need to address #20259 (comment)

Once thats done we can merge this quiickly before it gets out of sync again

avatar brianteeman
brianteeman - comment - 15 Oct 2018

Hi - thank you so much for doing this but we didnt want you to have to redo the pr again so it was done in a new pr and merged. Sorry that this didnt get updated to state that

avatar brianteeman brianteeman - change - 15 Oct 2018
Status Pending Closed
Closed_Date 0000-00-00 00:00:00 2018-10-15 11:57:55
Closed_By brianteeman
avatar brianteeman brianteeman - close - 15 Oct 2018

Add a Comment

Login with GitHub to post a comment