User tests: Successful: Unsuccessful:
Pull Request for Issue # .
Replace DatabaseDriver::q and DatabaseDriver::qn with complete name DatabaseDriver::quote and DatabaseDriver::quoteName.
Code review.
None
None
None
Status | New | ⇒ | Pending |
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 |
How does it improve the code?
It improves readability of the code
Labels |
Added:
?
|
@brianteeman A unified naming style may avoid misunderstandings IMHO
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 :-)
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
It improves readability of the code
A very subjective opinion in my opinion.
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 ;)
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);
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)
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.
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.
@Wang-Yu-Chao can you update with the requests from @mbabker and fix the merge conflict please
If this PR is accepted we should adjust https://developer.joomla.org/coding-standards/php-code.html too.
@Wang-Yu-Chao any chance to get an update on this?
@Wang-Yu-Chao Please can you update this to address the issues raised and fix the conflicts
Sorry folks I missed this issue. I'm getting on with it now.
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 |
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
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
Status | Pending | ⇒ | Closed |
Closed_Date | 0000-00-00 00:00:00 | ⇒ | 2018-10-15 11:57:55 |
Closed_By | ⇒ | brianteeman |
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