clean-up work

[Clean Code](https://www.42lines.net/2012/07/06/clean-code-reducing-wtfs-per-minute/) === Quality Code
fixes #91, #87
<a href='#crh-start'></a><a href='#crh-data-%7B%22processed%22%3A%20%5B%22https%3A//github.com/zencart/zc-v1-series/pull/96%23issuecomment-48860671%22%2C%20%22https%3A//github.com/zencart/zc-v1-series/pull/96%23issuecomment-48861613%22%2C%20%22https%3A//github.com/zencart/zc-v1-series/pull/96%23issuecomment-48862101%22%2C%20%22https%3A//github.com/zencart/zc-v1-series/pull/96%23issuecomment-48863308%22%2C%20%22https%3A//github.com/zencart/zc-v1-series/pull/96%23issuecomment-48864059%22%2C%20%22https%3A//github.com/zencart/zc-v1-series/pull/96%23issuecomment-48970907%22%2C%20%22https%3A//github.com/zencart/zc-v1-series/pull/96%23issuecomment-48981223%22%2C%20%22https%3A//github.com/zencart/zc-v1-series/pull/96%23discussion_r14864288%22%2C%20%22https%3A//github.com/zencart/zc-v1-series/pull/96%23discussion_r14864343%22%2C%20%22https%3A//github.com/zencart/zc-v1-series/pull/96%23discussion_r14864668%22%2C%20%22https%3A//github.com/zencart/zc-v1-series/pull/96%23discussion_r14865168%22%2C%20%22https%3A//github.com/zencart/zc-v1-series/pull/96%23discussion_r14909339%22%2C%20%22https%3A//github.com/zencart/zc-v1-series/pull/96%23discussion_r14909340%22%2C%20%22https%3A//github.com/zencart/zc-v1-series/pull/96%23discussion_r14910603%22%2C%20%22https%3A//github.com/zencart/zc-v1-series/pull/96%23discussion_r14911165%22%2C%20%22https%3A//github.com/zencart/zc-v1-series/pull/96%23discussion_r14911659%22%2C%20%22https%3A//github.com/zencart/zc-v1-series/pull/96%23discussion_r14912109%22%2C%20%22https%3A//github.com/zencart/zc-v1-series/pull/96%23discussion_r14912587%22%2C%20%22https%3A//github.com/zencart/zc-v1-series/pull/96%23discussion_r14912733%22%2C%20%22https%3A//github.com/zencart/zc-v1-series/pull/96%23discussion_r14913284%22%2C%20%22https%3A//github.com/zencart/zc-v1-series/pull/96%23discussion_r14913484%22%2C%20%22https%3A//github.com/zencart/zc-v1-series/pull/96%23discussion_r14913800%22%2C%20%22https%3A//github.com/zencart/zc-v1-series/pull/96%23discussion_r14914765%22%2C%20%22https%3A//github.com/zencart/zc-v1-series/pull/96%23discussion_r14915463%22%5D%2C%20%22comments%22%3A%20%7B%22General%20Comment%22%3A%20%7B%22html_url%22%3A%20%22https%3A//github.com/zencart/zc-v1-series/pull/96%23issuecomment-48860671%22%2C%20%22comments%22%3A%20%5B%7B%22body%22%3A%20%22%3A-1%3A%5Cr%5Cnplease%20remove%20this%20PR.%5Cr%5CnThe%20tone%20of%20the%20commit%20messages%20is%20arrogant%20and%20disrespectful%20to%20others%22%2C%20%22created_at%22%3A%20%222014-07-14T02%3A35%3A22Z%22%2C%20%22user%22%3A%20%7B%22avatar_url%22%3A%20%22https%3A//avatars.githubusercontent.com/u/227354%3F%22%2C%20%22html_url%22%3A%20%22https%3A//github.com/zcwilt%22%7D%7D%2C%20%7B%22body%22%3A%20%22%40zcwilt%20cleaned%20up%20commit%20messages%20via%20%60rebase%60%22%2C%20%22created_at%22%3A%20%222014-07-14T03%3A03%3A41Z%22%2C%20%22user%22%3A%20%7B%22avatar_url%22%3A%20%22https%3A//avatars.githubusercontent.com/u/1590605%3F%22%2C%20%22html_url%22%3A%20%22https%3A//github.com/texdc%22%7D%7D%2C%20%7B%22body%22%3A%20%22thanks%20for%20updating%20that.%20however%20think%20https%3A//github.com/texdc/ZenCart/commit/6b6638dfac8654e3bd7e4abbf938516e316cd93c%20slipped%20into%20your%20rebase%22%2C%20%22created_at%22%3A%20%222014-07-14T03%3A17%3A50Z%22%2C%20%22user%22%3A%20%7B%22avatar_url%22%3A%20%22https%3A//avatars.githubusercontent.com/u/227354%3F%22%2C%20%22html_url%22%3A%20%22https%3A//github.com/zcwilt%22%7D%7D%2C%20%7B%22body%22%3A%20%22%40zcwilt%20%20Yep%2C%20tricky%20things%20happen%20during%20%60rebase%60%3A%20very%20useful%2C%20but%20also%20very%20dangerous%20-%20always%20use%20with%20great%20care.%22%2C%20%22created_at%22%3A%20%222014-07-14T03%3A51%3A54Z%22%2C%20%22user%22%3A%20%7B%22avatar_url%22%3A%20%22https%3A//avatars.githubusercontent.com/u/1590605%3F%22%2C%20%22html_url%22%3A%20%22https%3A//github.com/texdc%22%7D%7D%2C%20%7B%22body%22%3A%20%22yeps%2C%20had%20that%20happen%20to%20me%20yesterday%2C%20%22%2C%20%22created_at%22%3A%20%222014-07-14T04%3A13%3A02Z%22%2C%20%22user%22%3A%20%7B%22avatar_url%22%3A%20%22https%3A//avatars.githubusercontent.com/u/227354%3F%22%2C%20%22html_url%22%3A%20%22https%3A//github.com/zcwilt%22%7D%7D%2C%20%7B%22body%22%3A%20%22%3A%2B1%3A%3Ca%20href%3D%27%23crh-update-none%27%3E%3C/a%3E%22%2C%20%22created_at%22%3A%20%222014-07-14T22%3A57%3A17Z%22%2C%20%22user%22%3A%20%7B%22avatar_url%22%3A%20%22https%3A//avatars.githubusercontent.com/u/1590605%3F%22%2C%20%22html_url%22%3A%20%22https%3A//github.com/texdc%22%7D%7D%2C%20%7B%22body%22%3A%20%22%3Ashipit%3A%22%2C%20%22created_at%22%3A%20%222014-07-15T01%3A37%3A16Z%22%2C%20%22user%22%3A%20%7B%22avatar_url%22%3A%20%22https%3A//avatars.githubusercontent.com/u/2963958%3F%22%2C%20%22html_url%22%3A%20%22https%3A//github.com/lhungil%22%7D%7D%5D%2C%20%22title%22%3A%20%22General%20Comment%22%7D%2C%20%22Pull%206a6b8ae5d0cdcc92786161b8efd334f229391da6%20admin/includes/functions/general.php%2050%22%3A%20%7B%22html_url%22%3A%20%22https%3A//github.com/zencart/zc-v1-series/pull/96%23discussion_r14864343%22%2C%20%22comments%22%3A%20%5B%7B%22body%22%3A%20%22Would%20it%20be%20better%20to%20use%20the%20existing%20Zen%20Cart%20methods%20in%20query_factory%20for%20prepared%20statements%20instead%20of%20using%20sprintf%3F%22%2C%20%22created_at%22%3A%20%222014-07-14T04%3A55%3A32Z%22%2C%20%22user%22%3A%20%7B%22avatar_url%22%3A%20%22https%3A//avatars.githubusercontent.com/u/2963958%3F%22%2C%20%22html_url%22%3A%20%22https%3A//github.com/lhungil%22%7D%7D%2C%20%7B%22body%22%3A%20%22Ya%2C%20not%20a%20fan%20of%20heredoc%20or%20sprintf%20for%20SQL%20queries%20either%2C%20at%20least%20not%20in%20the%20v1.x%20code%20base.%5Cr%5Cn%5Cr%5CnAlso%2C%20aren%27t%20RIGHT%20JOINs%20considered%20to%20be%20slower%20than%20LEFT%3F%22%2C%20%22created_at%22%3A%20%222014-07-14T05%3A17%3A53Z%22%2C%20%22user%22%3A%20%7B%22avatar_url%22%3A%20%22https%3A//avatars.githubusercontent.com/u/404472%3F%22%2C%20%22html_url%22%3A%20%22https%3A//github.com/drbyte%22%7D%7D%2C%20%7B%22body%22%3A%20%22Indeed%2C%20since%20the%20query%20was%20rewritten%20with%20the%20tables%20reversed%2C%20a%20LEFT%20JOIN%20would%20be%20required.%20The%20PR%20brings%20back%20nulls%20for%20attributes%20without%20downloads%2C%20which%20is%20the%20opposite%20of%20the%20desired%20result.%5Cr%5Cn%5Cr%5CnBut%20there%27s%20no%20need%20to%20rewrite%20the%20query%20at%20all.%20The%20original%20query%20is%20fine.%22%2C%20%22created_at%22%3A%20%222014-07-14T05%3A46%3A15Z%22%2C%20%22user%22%3A%20%7B%22avatar_url%22%3A%20%22https%3A//avatars.githubusercontent.com/u/404472%3F%22%2C%20%22html_url%22%3A%20%22https%3A//github.com/drbyte%22%7D%7D%2C%20%7B%22body%22%3A%20%22%3A%2B1%3A%3Ca%20href%3D%27%23crh-update-none%27%3E%3C/a%3E%22%2C%20%22created_at%22%3A%20%222014-07-14T22%3A57%3A17Z%22%2C%20%22user%22%3A%20%7B%22avatar_url%22%3A%20%22https%3A//avatars.githubusercontent.com/u/1590605%3F%22%2C%20%22html_url%22%3A%20%22https%3A//github.com/texdc%22%7D%7D%5D%2C%20%22title%22%3A%20%22File%3A%20admin/includes/functions/general.php%3AL2558-2594%22%7D%2C%20%22Pull%206a6b8ae5d0cdcc92786161b8efd334f229391da6%20testFramework/unittests/support/zcAdminTestCase.php%2012%22%3A%20%7B%22html_url%22%3A%20%22https%3A//github.com/zencart/zc-v1-series/pull/96%23discussion_r14864288%22%2C%20%22comments%22%3A%20%5B%7B%22body%22%3A%20%22I%20believe%20using%20an%20%5C%22if%5C%22%20block%20creates%20more%20readable%20code%20than%20an%20implied%20%60if%60%20via%20%60%7C%7C%60.%20I%20would%20recommend%20against%20this%20line%20change%20%28although%20would%20not%20be%20opposed%20to%20the%20addition%20of%20curly%20brackets%20to%20the%20original%20code%29.%22%2C%20%22created_at%22%3A%20%222014-07-14T04%3A50%3A54Z%22%2C%20%22user%22%3A%20%7B%22avatar_url%22%3A%20%22https%3A//avatars.githubusercontent.com/u/2963958%3F%22%2C%20%22html_url%22%3A%20%22https%3A//github.com/lhungil%22%7D%7D%2C%20%7B%22body%22%3A%20%22%3A%2B1%3A%3Ca%20href%3D%27%23crh-update-complete%27%3E%3C/a%3E%22%2C%20%22created_at%22%3A%20%222014-07-14T22%3A57%3A17Z%22%2C%20%22user%22%3A%20%7B%22avatar_url%22%3A%20%22https%3A//avatars.githubusercontent.com/u/1590605%3F%22%2C%20%22html_url%22%3A%20%22https%3A//github.com/texdc%22%7D%7D%5D%2C%20%22title%22%3A%20%22File%3A%20testFramework/unittests/support/zcAdminTestCase.php%3AL11-22%22%7D%2C%20%22Pull%20cf79ac113166d46f75a399220bde8e5d7c74d79d%20admin/includes/functions/general.php%2037%22%3A%20%7B%22html_url%22%3A%20%22https%3A//github.com/zencart/zc-v1-series/pull/96%23discussion_r14915463%22%2C%20%22comments%22%3A%20%5B%7B%22body%22%3A%20%22The%20usual%20ZC%20pattern%20for%20writing%20SQL%20queries%20is%20to%20put%20keywords%20like%20SELECT%2C%20FROM%2C%20JOIN%2C%20ON%2C%20WHERE%2C%20USING%2C%20HAVING%2C%20AND/OR%2C%20LIMIT%20on%20a%20new%20line%2C%20primarily%20for%20readability.%5Cr%5CnLet%27s%20do%20that%20here.%20%20eg%3A%20move%20the%20FROM%20to%20the%20next%20line.%5Cr%5Cn%5Cr%5CnThis%20brings%20me%20to%20another%20style%20issue%20...%20While%20I%20don%27t%20want%20to%20be%20terribly%20nitpicky%20here%20%28I%20know%20there%27s%20already%20been%20a%20lot%20of%20scrutiny%20in%20this%20PR%29%2C%20the%20majority%20of%20ZC%20code%20does%20SQL%20queries%20as%20one%20long%20multi-line%20string%2C%20wrapped%20by%20double-quotes.%20%20And%20I%27m%20torn%20about%20how%20fussy%20to%20be%20about%20that%20here%2C%20vs%20precedent-setting.%20%5Cr%5CnI%20know%20single-quotes%20are%20slightly%20more%20performant%20because%20they%20don%27t%20trigger%20the%20substitution%20parser%2C%20but%20then%20they%20require%20the%20user%20to%20be%20sure%20to%20escape%20any%20actual%20required%20apostrophes%20in%20the%20query.%20%28I%20know%2C%20heredoc/nowdoc%20stuff%20gets%20around%20all%20that%2C%20but%20we%20never%20use%20those%20in%20ZC%20...%20except%20maybe%20one%20addon%20I%20submitted%20a%20long%20time%20back%2C%20but%20that%27s%20not%20core%20code.%29%20%20%5Cr%5CnI%20also%20get%20that%20treating%20each%20line%20as%20a%20separate%20string%20with%20joining%20dots%20at%20the%20beginning%20of%20the%20next%20line%20has%20readability%20benefits%20and%20also%20quickly%20shows%20which%20line%20any%20quote-mismatching%20occurs%20on%2C%20assuming%20one%20is%20using%20syntax-highlighting.%5Cr%5CnBut%20it%20also%20makes%20it%20harder%20to%20grab%20that%20query%20from%20the%20code%20and%20drop%20it%20into%20phpMyAdmin%20%28et%20al%29%20and%20run%20it%20without%20getting%20rid%20of%20all%20those%20quotes%20and%20dots%20%28notwithstanding%20that%20tablename%20constants%20still%20require%20editing%20anyway%2C%20but%20that%27s%20less%20tedious%20than%20also%20cleaning%204%20lines%20of%20dots%20and%20quotes%29.%20%5Cr%5Cn%5Cr%5CnSo%2C%20as%20with%20anything%2C%20there%20are%20pros%20and%20cons%20of%20both%20conventions.%20I%20see%20neither%20being%20grossly%20better%20than%20the%20other%2C%20other%20than%20consistency%20with%20existing%20code.%5Cr%5Cn%5Cr%5CnI%27m%20going%20to%20suggest%20that%20code%20in%20existing%20core%20files%20follow%20the%20old%20convention%20as%20much%20as%20possible%2C%20and%20that%20we%20reserve%20the%20use%20of%20more%20modern%20styles%20for%20when%20new%20files%20are%20added.%20That%27s%20pretty%20much%20the%20standard%20approach%20used%20by%20any%20enterprise%20doing%20a%20migration%20from%20old%20to%20new.%5Cr%5Cn%5Cr%5CnSo%2C%20while%20I%20respect%20your%20choice%20of%20style%20used%20in%20your%20own%20work%2C%20I%27m%20going%20to%20ask%20you%20%28and%20everyone%20else%20who%20contributes%20to%20ZC%29%20to%20use%20the%20ZC%20style.%20%22%2C%20%22created_at%22%3A%20%222014-07-15T02%3A53%3A37Z%22%2C%20%22user%22%3A%20%7B%22avatar_url%22%3A%20%22https%3A//avatars.githubusercontent.com/u/404472%3F%22%2C%20%22html_url%22%3A%20%22https%3A//github.com/drbyte%22%7D%7D%5D%2C%20%22title%22%3A%20%22File%3A%20admin/includes/functions/general.php%3AL2558-2584%22%7D%2C%20%22Pull%20522084e112948161f57f197dc60bae1a70e73118%20admin/includes/functions/general.php%2040%22%3A%20%7B%22html_url%22%3A%20%22https%3A//github.com/zencart/zc-v1-series/pull/96%23discussion_r14910603%22%2C%20%22comments%22%3A%20%5B%7B%22body%22%3A%20%22Using%20a%20left%20join%20may%20cause%20rows%20to%20be%20returned%20with%20a%20%5C%22null%5C%22%20value%20when%20no%20matching%20products_attributes_id%20exists%20in%20TABLE_PRODUCTS_ATTRIBUTES_DOWNLOAD.%5Cr%5Cn%5Cr%5CnMay%20need%20to%20modify%20the%20ON%20or%20WHERE%20clause%20to%20exclude%20NULLs%20%28if%20I%20remember%20correctly%20doing%20the%20check%20in%20the%20ON%20clause%20is%20faster%29...%20Or%20switch%20back%20to%20the%20original%20query...%5Cr%5Cn%22%2C%20%22created_at%22%3A%20%222014-07-14T23%3A34%3A43Z%22%2C%20%22user%22%3A%20%7B%22avatar_url%22%3A%20%22https%3A//avatars.githubusercontent.com/u/2963958%3F%22%2C%20%22html_url%22%3A%20%22https%3A//github.com/lhungil%22%7D%7D%2C%20%7B%22body%22%3A%20%22That%27s%20the%20same%20argument%20that%20was%20used%20for%20the%20%60right%20join%60.%20%20Are%20you%20sure%20about%20that%3F%20%20Which%20is%20correct%3F%20%20%28I%27ve%20been%20using%20%60Doctrine%60%2C%20so%20my%20%60SQL%60%20is%20rusty.%29%22%2C%20%22created_at%22%3A%20%222014-07-14T23%3A51%3A52Z%22%2C%20%22user%22%3A%20%7B%22avatar_url%22%3A%20%22https%3A//avatars.githubusercontent.com/u/1590605%3F%22%2C%20%22html_url%22%3A%20%22https%3A//github.com/texdc%22%7D%7D%2C%20%7B%22body%22%3A%20%22Outer%20joins%20include%20all%20matching%20entries%20from%20the%20first%20table%20%28left%20table%20for%20left%20join%20-%20right%20table%20for%20right%20join%29.%20When%20no%20matching%20row%20is%20found%20%28based%20upon%20the%20ON%20clause%29%20in%20the%20second%20table%2C%20null%20is%20returned%20for%20fields%20from%20the%20second%20table.%22%2C%20%22created_at%22%3A%20%222014-07-15T00%3A09%3A45Z%22%2C%20%22user%22%3A%20%7B%22avatar_url%22%3A%20%22https%3A//avatars.githubusercontent.com/u/2963958%3F%22%2C%20%22html_url%22%3A%20%22https%3A//github.com/lhungil%22%7D%7D%2C%20%7B%22body%22%3A%20%22Why%20change%20the%20query%20at%20all%20though%3F%22%2C%20%22created_at%22%3A%20%222014-07-15T00%3A27%3A45Z%22%2C%20%22user%22%3A%20%7B%22avatar_url%22%3A%20%22https%3A//avatars.githubusercontent.com/u/404472%3F%22%2C%20%22html_url%22%3A%20%22https%3A//github.com/drbyte%22%7D%7D%2C%20%7B%22body%22%3A%20%22Because%20it%27s%20like%20the%20table-layout%20vs%20css%20debate%20-%20loose%20joins%20vs%20strict%20joins.%20%20I%20always%20prefer%20explicit%20vs%20implicit%20as%20it%20eliminates%20ambiguity%20and%20therefore%20bugs.%22%2C%20%22created_at%22%3A%20%222014-07-15T00%3A45%3A27Z%22%2C%20%22user%22%3A%20%7B%22avatar_url%22%3A%20%22https%3A//avatars.githubusercontent.com/u/1590605%3F%22%2C%20%22html_url%22%3A%20%22https%3A//github.com/texdc%22%7D%7D%2C%20%7B%22body%22%3A%20%22RIGHT%20JOIN%20test%20products_id%2022%5Cr%5Cn2%20Records%201%20with%20and%201%20without%20download%5Cr%5CnRIGHT%20JOIN%20test%20products_id%201%5Cr%5Cn5%20records%205%20without%20download%5Cr%5Cn%5Cr%5CnLEFT%20JOIN%20test%20products_id%201%5Cr%5Cn1%20Records%201%20with%20download%5Cr%5CnLEFT%20JOIN%20test%20products_id%201%5Cr%5Cn0%20records%5Cr%5Cn%5Cr%5CnOriginal%20test%3A%5Cr%5Cntest%20products_id%201%5Cr%5Cn1%20Records%201%20with%20download%5Cr%5Cntest%20products_id%201%5Cr%5Cn0%20records%5Cr%5Cn%5Cr%5CnRIGHT%20JOIN%20does%20not%20give%20proper%20results.%20LEFT%20JOIN%20gives%20proper%20results.%20ORIGINAL%20SELECT%20gives%20proper%20results.%5Cr%5Cn%5Cr%5CnNo%20change%20needed%20to%20the%20function%20for%20proper%20results.%22%2C%20%22created_at%22%3A%20%222014-07-15T00%3A50%3A49Z%22%2C%20%22user%22%3A%20%7B%22avatar_url%22%3A%20%22https%3A//avatars.githubusercontent.com/u/1992427%3F%22%2C%20%22html_url%22%3A%20%22https%3A//github.com/ajeh%22%7D%7D%2C%20%7B%22body%22%3A%20%22The%20following%20SQL%20Query%20is%20being%20generated%20by%20the%20current%20code%20in%20this%20PR%3A%5Cr%5Cn%5Cr%5Cn%5C%22select%20pad.products_attributes_id%20from%20products_attributes%20pa%5Cr%5Cnleft%20join%20products_attributes_download%20pad%5Cr%5Cnon%20pad.products_attributes_id%20%3D%20pa.products_attributes_id%20%5Cr%5Cnwhere%20pa.products_id%20%3D%201%5C%22%5Cr%5Cn%5Cr%5CnWhen%20run%20against%20the%20%5C%22demo%5C%22%20Zen%20Cart%20database%20the%20result%20is%205%20rows%20containing%20NULL%20values%20for%20products_attributes_id.%5Cr%5Cn%5Cr%5CnThe%20original%20query%20correctly%20returns%20ZERO%20results.%20The%20equivalent%20%5C%22explicit%5C%22%20SQL%20query%20would%20be%3A%20%5Cr%5Cn%5C%22select%20pad.products_attributes_id%20from%20products_attributes_download%20pad%5Cr%5Cninner%20join%20products_attributes%20pa%5Cr%5Cnon%20pad.products_attributes_id%20%3D%20pa.products_attributes_id%5Cr%5Cnwhere%20pa.products_id%20%3D%201%5C%22%5Cr%5Cn%5Cr%5CnI%20would%20not%20mind%20the%20code%20being%20changed%20to%20use%20an%20explicit%20inner%20join.%5Cr%5Cn%22%2C%20%22created_at%22%3A%20%222014-07-15T01%3A11%3A52Z%22%2C%20%22user%22%3A%20%7B%22avatar_url%22%3A%20%22https%3A//avatars.githubusercontent.com/u/2963958%3F%22%2C%20%22html_url%22%3A%20%22https%3A//github.com/lhungil%22%7D%7D%2C%20%7B%22body%22%3A%20%22Ah%2C%20the%20inner%20join%21%20%20I%20forgot%20about%20that%20one...%20thanks%20%40lhungil.%22%2C%20%22created_at%22%3A%20%222014-07-15T01%3A20%3A04Z%22%2C%20%22user%22%3A%20%7B%22avatar_url%22%3A%20%22https%3A//avatars.githubusercontent.com/u/1590605%3F%22%2C%20%22html_url%22%3A%20%22https%3A//github.com/texdc%22%7D%7D%2C%20%7B%22body%22%3A%20%22%3A%2B1%3A%22%2C%20%22created_at%22%3A%20%222014-07-15T01%3A33%3A05Z%22%2C%20%22user%22%3A%20%7B%22avatar_url%22%3A%20%22https%3A//avatars.githubusercontent.com/u/2963958%3F%22%2C%20%22html_url%22%3A%20%22https%3A//github.com/lhungil%22%7D%7D%5D%2C%20%22title%22%3A%20%22File%3A%20admin/includes/functions/general.php%3AL2558-2584%22%7D%2C%20%22Pull%20cf79ac113166d46f75a399220bde8e5d7c74d79d%20admin/includes/functions/general.php%2043%22%3A%20%7B%22html_url%22%3A%20%22https%3A//github.com/zencart/zc-v1-series/pull/96%23discussion_r14914765%22%2C%20%22comments%22%3A%20%5B%7B%22body%22%3A%20%22I%20like%20this%20boolean%20direct%20return%20with%20method%20chaining.%20It%20works%20well%20in%20this%20scenario%2C%20and%20has%20both%20efficiency/performance%20and%20readability%20benefits.%5Cr%5CnWill%20consider%20adopting%20this%20style%20as%20part%20of%20our%20coding%20standards%20docs.%22%2C%20%22created_at%22%3A%20%222014-07-15T02%3A16%3A03Z%22%2C%20%22user%22%3A%20%7B%22avatar_url%22%3A%20%22https%3A//avatars.githubusercontent.com/u/404472%3F%22%2C%20%22html_url%22%3A%20%22https%3A//github.com/drbyte%22%7D%7D%5D%2C%20%22title%22%3A%20%22File%3A%20admin/includes/functions/general.php%3AL2558-2584%22%7D%7D%2C%20%22approved%22%3A%20%7B%22https%3A//github.com/lhungil%22%3A%20%7B%22avatar_url%22%3A%20%22https%3A//avatars.githubusercontent.com/u/2963958%3F%22%7D%7D%7D'></a>
<a href='https://www.codereviewhub.com/'><img src='http://www.codereviewhub.com/site/github-bar.png' height=40></a>

<img src='http://www.codereviewhub.com/site/github-approved-avatar.png'><a href='https://github.com/lhungil'><img src='https://avatars.githubusercontent.com/u/2963958?size=34' width=34 height=34></a>

- [ ] <a href='#crh-comment-Pull cf79ac113166d46f75a399220bde8e5d7c74d79d admin/includes/functions/general.php 43'></a> <img src='http://www.codereviewhub.com/site/github-remaining.png' height=16 width=60>&nbsp;<b><a href='https://github.com/zencart/zc-v1-series/pull/96#discussion_r14914765'>File: admin/includes/functions/general.php:L2558-2584</a></b>
- <a href='https://github.com/drbyte'><img border=0 src='https://avatars.githubusercontent.com/u/404472?size=16' height=16 width=16'></a> I like this boolean direct return with method chaining. It works well in this scenario, and has both efficiency/performance and readability benefits.
Will consider adopting this style as part of our coding standards docs.
- [ ] <a href='#crh-comment-Pull cf79ac113166d46f75a399220bde8e5d7c74d79d admin/includes/functions/general.php 37'></a> <img src='http://www.codereviewhub.com/site/github-remaining.png' height=16 width=60>&nbsp;<b><a href='https://github.com/zencart/zc-v1-series/pull/96#discussion_r14915463'>File: admin/includes/functions/general.php:L2558-2584</a></b>
- <a href='https://github.com/drbyte'><img border=0 src='https://avatars.githubusercontent.com/u/404472?size=16' height=16 width=16'></a> The usual ZC pattern for writing SQL queries is to put keywords like SELECT, FROM, JOIN, ON, WHERE, USING, HAVING, AND/OR, LIMIT on a new line, primarily for readability.
Let's do that here.  eg: move the FROM to the next line.
This brings me to another style issue ... While I don't want to be terribly nitpicky here (I know there's already been a lot of scrutiny in this PR), the majority of ZC code does SQL queries as one long multi-line string, wrapped by double-quotes.  And I'm torn about how fussy to be about that here, vs precedent-setting.
I know single-quotes are slightly more performant because they don't trigger the substitution parser, but then they require the user to be sure to escape any actual required apostrophes in the query. (I know, heredoc/nowdoc stuff gets around all that, but we never use those in ZC ... except maybe one addon I submitted a long time back, but that's not core code.)
I also get that treating each line as a separate string with joining dots at the beginning of the next line has readability benefits and also quickly shows which line any quote-mismatching occurs on, assuming one is using syntax-highlighting.
But it also makes it harder to grab that query from the code and drop it into phpMyAdmin (et al) and run it without getting rid of all those quotes and dots (notwithstanding that tablename constants still require editing anyway, but that's less tedious than also cleaning 4 lines of dots and quotes).
So, as with anything, there are pros and cons of both conventions. I see neither being grossly better than the other, other than consistency with existing code.
I'm going to suggest that code in existing core files follow the old convention as much as possible, and that we reserve the use of more modern styles for when new files are added. That's pretty much the standard approach used by any enterprise doing a migration from old to new.
So, while I respect your choice of style used in your own work, I'm going to ask you (and everyone else who contributes to ZC) to use the ZC style.
- [x] <a href='#crh-comment-General Comment'></a> <img src='http://www.codereviewhub.com/site/github-completed.png' height=16 width=60>&nbsp;<b><a href='https://github.com/zencart/zc-v1-series/pull/96#issuecomment-48860671'>General Comment</a></b>
- <a href='https://github.com/zcwilt'><img border=0 src='https://avatars.githubusercontent.com/u/227354?size=16' height=16 width=16'></a> :-1:
please remove this PR.
The tone of the commit messages is arrogant and disrespectful to others
- <a href='https://github.com/texdc'><img border=0 src='https://avatars.githubusercontent.com/u/1590605?size=16' height=16 width=16'></a> @zcwilt cleaned up commit messages via `rebase`
- <a href='https://github.com/zcwilt'><img border=0 src='https://avatars.githubusercontent.com/u/227354?size=16' height=16 width=16'></a> thanks for updating that. however think https://github.com/texdc/ZenCart/commit/6b6638dfac8654e3bd7e4abbf938516e316cd93c slipped into your rebase
- <a href='https://github.com/texdc'><img border=0 src='https://avatars.githubusercontent.com/u/1590605?size=16' height=16 width=16'></a> @zcwilt  Yep, tricky things happen during `rebase`: very useful, but also very dangerous - always use with great care.
- <a href='https://github.com/zcwilt'><img border=0 src='https://avatars.githubusercontent.com/u/227354?size=16' height=16 width=16'></a> yeps, had that happen to me yesterday,
- [x] <a href='#crh-comment-Pull 6a6b8ae5d0cdcc92786161b8efd334f229391da6 testFramework/unittests/support/zcAdminTestCase.php 12'></a> <img src='http://www.codereviewhub.com/site/github-completed.png' height=16 width=60>&nbsp;<b><a href='https://github.com/zencart/zc-v1-series/pull/96#discussion_r14864288'>File: testFramework/unittests/support/zcAdminTestCase.php:L11-22</a></b>
- <a href='https://github.com/lhungil'><img border=0 src='https://avatars.githubusercontent.com/u/2963958?size=16' height=16 width=16'></a> I believe using an "if" block creates more readable code than an implied `if` via `||`. I would recommend against this line change (although would not be opposed to the addition of curly brackets to the original code).
- [x] <a href='#crh-comment-Pull 6a6b8ae5d0cdcc92786161b8efd334f229391da6 admin/includes/functions/general.php 50'></a> <img src='http://www.codereviewhub.com/site/github-completed.png' height=16 width=60>&nbsp;<b><a href='https://github.com/zencart/zc-v1-series/pull/96#discussion_r14864343'>File: admin/includes/functions/general.php:L2558-2594</a></b>
- <a href='https://github.com/lhungil'><img border=0 src='https://avatars.githubusercontent.com/u/2963958?size=16' height=16 width=16'></a> Would it be better to use the existing Zen Cart methods in query_factory for prepared statements instead of using sprintf?
- <a href='https://github.com/drbyte'><img border=0 src='https://avatars.githubusercontent.com/u/404472?size=16' height=16 width=16'></a> Ya, not a fan of heredoc or sprintf for SQL queries either, at least not in the v1.x code base.
Also, aren't RIGHT JOINs considered to be slower than LEFT?
- <a href='https://github.com/drbyte'><img border=0 src='https://avatars.githubusercontent.com/u/404472?size=16' height=16 width=16'></a> Indeed, since the query was rewritten with the tables reversed, a LEFT JOIN would be required. The PR brings back nulls for attributes without downloads, which is the opposite of the desired result.
But there's no need to rewrite the query at all. The original query is fine.
- [x] <a href='#crh-comment-Pull 522084e112948161f57f197dc60bae1a70e73118 admin/includes/functions/general.php 40'></a> <img src='http://www.codereviewhub.com/site/github-completed.png' height=16 width=60>&nbsp;<b><a href='https://github.com/zencart/zc-v1-series/pull/96#discussion_r14910603'>File: admin/includes/functions/general.php:L2558-2584</a></b>
- <a href='https://github.com/lhungil'><img border=0 src='https://avatars.githubusercontent.com/u/2963958?size=16' height=16 width=16'></a> Using a left join may cause rows to be returned with a "null" value when no matching products_attributes_id exists in TABLE_PRODUCTS_ATTRIBUTES_DOWNLOAD.
May need to modify the ON or WHERE clause to exclude NULLs (if I remember correctly doing the check in the ON clause is faster)... Or switch back to the original query...
- <a href='https://github.com/texdc'><img border=0 src='https://avatars.githubusercontent.com/u/1590605?size=16' height=16 width=16'></a> That's the same argument that was used for the `right join`.  Are you sure about that?  Which is correct?  (I've been using `Doctrine`, so my `SQL` is rusty.)
- <a href='https://github.com/lhungil'><img border=0 src='https://avatars.githubusercontent.com/u/2963958?size=16' height=16 width=16'></a> Outer joins include all matching entries from the first table (left table for left join - right table for right join). When no matching row is found (based upon the ON clause) in the second table, null is returned for fields from the second table.
- <a href='https://github.com/drbyte'><img border=0 src='https://avatars.githubusercontent.com/u/404472?size=16' height=16 width=16'></a> Why change the query at all though?
- <a href='https://github.com/texdc'><img border=0 src='https://avatars.githubusercontent.com/u/1590605?size=16' height=16 width=16'></a> Because it's like the table-layout vs css debate - loose joins vs strict joins.  I always prefer explicit vs implicit as it eliminates ambiguity and therefore bugs.
- <a href='https://github.com/ajeh'><img border=0 src='https://avatars.githubusercontent.com/u/1992427?size=16' height=16 width=16'></a> RIGHT JOIN test products_id 22
2 Records 1 with and 1 without download
RIGHT JOIN test products_id 1
5 records 5 without download
LEFT JOIN test products_id 1
1 Records 1 with download
LEFT JOIN test products_id 1
0 records
Original test:
test products_id 1
1 Records 1 with download
test products_id 1
0 records
RIGHT JOIN does not give proper results. LEFT JOIN gives proper results. ORIGINAL SELECT gives proper results.
No change needed to the function for proper results.
- <a href='https://github.com/lhungil'><img border=0 src='https://avatars.githubusercontent.com/u/2963958?size=16' height=16 width=16'></a> The following SQL Query is being generated by the current code in this PR:
"select pad.products_attributes_id from products_attributes pa
left join products_attributes_download pad
on pad.products_attributes_id = pa.products_attributes_id
where pa.products_id = 1"
When run against the "demo" Zen Cart database the result is 5 rows containing NULL values for products_attributes_id.
The original query correctly returns ZERO results. The equivalent "explicit" SQL query would be:
"select pad.products_attributes_id from products_attributes_download pad
inner join products_attributes pa
on pad.products_attributes_id = pa.products_attributes_id
where pa.products_id = 1"
I would not mind the code being changed to use an explicit inner join.
- <a href='https://github.com/texdc'><img border=0 src='https://avatars.githubusercontent.com/u/1590605?size=16' height=16 width=16'></a> Ah, the inner join!  I forgot about that one... thanks @lhungil.


<a href='https://www.codereviewhub.com/zencart/zc-v1-series/pull/96?mark_as_completed=1'><img src='http://www.codereviewhub.com/site/github-mark-as-completed.png' height=26></a>&nbsp;<a href='https://www.codereviewhub.com/zencart/zc-v1-series/pull/96?approve=1'><img src='http://www.codereviewhub.com/site/github-approve.png' height=26></a>&nbsp;<a href='https://www.codereviewhub.com/zencart/zc-v1-series/pull/96?approve=0'><img src='http://www.codereviewhub.com/site/github-undo-approve.png' height=26></a>&nbsp;<a href='https://github.com/zencart/zc-v1-series/pull/96'><img src='http://www.codereviewhub.com/site/github-refresh.png' height=26></a>
<a href='#crh-end'></a>
I respect your position with one caveat: if nothing changes, there is no migration.  It is this reluctance to improve standards that is the most concerning.