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%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%23issuecomment-48978400%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%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%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%20need%20to%20the%20function%20for%20proper%20results.%5Cr%5Cn%5Cr%5Cn%22%2C%20%22created_at%22%3A%20%222014-07-15T00%3A49%3A21Z%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%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%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%28WHERE%20clause%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.%5Cr%5Cn%5Cr%5CnThe%20original%20query%20avoids%20this%20by%20only%20including%20results%20where%20the%20products_attributes_id%20fields%20from%20both%20tables%20are%20equal%20and%20the%20products_id%20matches.%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%20need%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%20%28all%20versions%20of%20MySQL%29.%20It%20is%20the%20equivalent%20of%20the%20explicit%20query%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%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%5D%2C%20%22title%22%3A%20%22File%3A%20admin/includes/functions/general.php%3AL2558-2584%22%7D%7D%7D'></a>
<a href='https://www.codereviewhub.com/'><img src='http://www.codereviewhub.com/site/github-bar.png' height=40></a>
- [ ] <a href='#crh-comment-General Comment'></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#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,
- <a href='https://github.com/texdc'><img border=0 src='https://avatars.githubusercontent.com/u/1590605?size=16' height=16 width=16'></a> :+1:<a href='#crh-update-none'></a>
- <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 need to the function for proper results.
- [ ] <a href='#crh-comment-Pull 522084e112948161f57f197dc60bae1a70e73118 admin/includes/functions/general.php 40'></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_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 (WHERE clause). When no matching row is found (based upon the ON clause) in the second table, null is returned for fields from the second table.
The original query avoids this by only including results where the products_attributes_id fields from both tables are equal and the products_id matches.
- <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 need 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 (all versions of MySQL). It is the equivalent of the explicit query:
"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"
- <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.
- [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.


<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://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>
:+1: