zc_install - for issue #99

<a href='#crh-start'></a><a href='#crh-data-%7B%22processed%22%3A%20%5B%22https%3A//github.com/zencart/zc-v1-series/pull/100%23discussion_r14977316%22%2C%20%22https%3A//github.com/zencart/zc-v1-series/pull/100%23discussion_r14977318%22%2C%20%22https%3A//github.com/zencart/zc-v1-series/pull/100%23discussion_r14977323%22%2C%20%22https%3A//github.com/zencart/zc-v1-series/pull/100%23discussion_r14977365%22%2C%20%22https%3A//github.com/zencart/zc-v1-series/pull/100%23discussion_r14978240%22%2C%20%22https%3A//github.com/zencart/zc-v1-series/pull/100%23discussion_r14978328%22%2C%20%22https%3A//github.com/zencart/zc-v1-series/pull/100%23discussion_r14981176%22%2C%20%22https%3A//github.com/zencart/zc-v1-series/pull/100%23issuecomment-49164964%22%2C%20%22https%3A//github.com/zencart/zc-v1-series/pull/100%23issuecomment-49188753%22%5D%2C%20%22comments%22%3A%20%7B%22Pull%20a522f0ea17a4a7632ddb5f93153206f3c5cab21a%20zc_install/includes/classes/class.systemChecker.php%2010%22%3A%20%7B%22html_url%22%3A%20%22https%3A//github.com/zencart/zc-v1-series/pull/100%23discussion_r14977365%22%2C%20%22comments%22%3A%20%5B%7B%22body%22%3A%20%22I%20don%27t%20really%20like%20this%20complex%20conditional%20-%20but%20at%20the%20very%20least%2C%20you%20can%20%60return%20TRUE%3B%60%20here.%22%2C%20%22created_at%22%3A%20%222014-07-16T01%3A12%3A14Z%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%20zc_install/includes/classes/class.systemChecker.php%3AL231-243%22%7D%2C%20%22Pull%20a522f0ea17a4a7632ddb5f93153206f3c5cab21a%20zc_install/includes/classes/class.systemChecker.php%2046%22%3A%20%7B%22html_url%22%3A%20%22https%3A//github.com/zencart/zc-v1-series/pull/100%23discussion_r14977323%22%2C%20%22comments%22%3A%20%5B%7B%22body%22%3A%20%22%5BUnnecessary%5D%28http%3A//c2.com/cgi/wiki%3FElseConsideredSmelly%29%20%60else%60.%22%2C%20%22created_at%22%3A%20%222014-07-16T01%3A09%3A57Z%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%20zc_install/includes/classes/class.systemChecker.php%3AL525-565%22%7D%2C%20%22General%20Comment%22%3A%20%7B%22html_url%22%3A%20%22https%3A//github.com/zencart/zc-v1-series/pull/100%23issuecomment-49164964%22%2C%20%22comments%22%3A%20%5B%7B%22body%22%3A%20%22%40lhungil%20I%20agree%2C%20multiple%20return%20points%20are%20an%20indication%20that%20refactoring%20is%20needed%2C%20it%27s%20not%20just%20style.%20%202%20returns%20is%20probably%20a%20good%20maximum.%22%2C%20%22created_at%22%3A%20%222014-07-16T13%3A29%3A48Z%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%22Ya%2C%20%40zcwilt%20and%20I%20were%20thinking%20the%20same%20thing%20when%20we%20were%20troubleshooting%20a%20quirk%20in%20there%20yesterday.%20Definitely%20something%20to%20tackle%20in%20the%20future.%20We%20opted%20to%20wait%20since%20zc_install%20is%20used%20once%20at%20install%20time%20and%20then%20deleted.%20Doesn%27t%20have%20to%20be%20stellar%20code%2C%20so%20doesn%27t%20really%20require%20our%20immediate%20focus.%20A%20good%20project%20for%20when%20other%20priorities%20aren%27t%20competing.%22%2C%20%22created_at%22%3A%20%222014-07-16T16%3A09%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%22General%20Comment%22%7D%2C%20%22Pull%20a522f0ea17a4a7632ddb5f93153206f3c5cab21a%20zc_install/includes/languages/lngEnglish.php%2047%22%3A%20%7B%22html_url%22%3A%20%22https%3A//github.com/zencart/zc-v1-series/pull/100%23discussion_r14977316%22%2C%20%22comments%22%3A%20%5B%7B%22body%22%3A%20%22%3A%2B1%3A%20%22%2C%20%22created_at%22%3A%20%222014-07-16T01%3A09%3A43Z%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%20zc_install/includes/languages/lngEnglish.php%3AL140-147%22%7D%2C%20%22Pull%20a522f0ea17a4a7632ddb5f93153206f3c5cab21a%20zc_install/includes/classes/class.zcDatabaseInstaller.php%2017%22%3A%20%7B%22html_url%22%3A%20%22https%3A//github.com/zencart/zc-v1-series/pull/100%23discussion_r14977318%22%2C%20%22comments%22%3A%20%5B%7B%22body%22%3A%20%22If%20this%20isn%27t%20being%20used%2C%20it%20can%20be%20removed.%20%20Source%20control%20will%20keep%20track%20of%20it.%20%20Otherwise%2C%20please%20condense%20it.%22%2C%20%22created_at%22%3A%20%222014-07-16T01%3A09%3A50Z%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%22I%20was%20just%20working%20on%20this.%20There%27s%20actually%20a%20bug%20here%20in%20that%20other%20parts%20of%20the%20code%20are%20expecting%20a%20return%20value.%20Some%20of%20that%27s%20legacy%20logic%2C%20and%20warrants%20turning%20into%20another%20issue%20to%20address%20that.%22%2C%20%22created_at%22%3A%20%222014-07-16T01%3A56%3A54Z%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%20Carry-on%2C%20then.%20Wasn%27t%20sure%20if%20you%20had%20finished%20yet.%5Cr%5Cn%5Cr%5Cn%3E%20On%20Jul%2015%2C%202014%2C%20at%208%3A56%20PM%2C%20Chris%20Brown%20%3Cnotifications%40github.com%3E%20wrote%3A%5Cr%5Cn%3E%20%5Cr%5Cn%3E%20In%20zc_install/includes/classes/class.zcDatabaseInstaller.php%3A%5Cr%5Cn%3E%20%5Cr%5Cn%3E%20%3E%20%40%40%20-91%2C13%20%2B91%2C13%20%40%40%20public%20function%20parseSqlFile%28%24fileName%2C%20%24options%20%3D%20NULL%29%5Cr%5Cn%3E%20%3E%20%20%20%20%20%20%7D%5Cr%5Cn%3E%20%3E%20%20//if%20%28count%28%24lines%29%20%3C%20200%29%20sleep%285%29%3B%5Cr%5Cn%3E%20%3E%20%20%20%20%20%20%24this-%3EdoJsonProgressLoggingEnd%28%29%3B%5Cr%5Cn%3E%20%3E%20-%20%20%20%20if%20%28count%28%24this-%3EupgradeExceptions%29%20%3E%200%29%5Cr%5Cn%3E%20%3E%20-%20%20%20%20%7B%5Cr%5Cn%3E%20%3E%20-%20%20%20%20%20%20return%20TRUE%3B%5Cr%5Cn%3E%20%3E%20-%20%20%20%20%7D%20else%5Cr%5Cn%3E%20%3E%20-%20%20%20%20%7B%5Cr%5Cn%3E%20%3E%20-%20%20%20%20%20%20return%20FALSE%3B%5Cr%5Cn%3E%20%3E%20-%20%20%20%20%7D%5Cr%5Cn%3E%20%3E%20%2B//%20%20%20%20%20if%20%28count%28%24this-%3EupgradeExceptions%29%20%3E%200%29%5Cr%5Cn%3E%20%3E%20%2B//%20%20%20%20%20%7B%5Cr%5Cn%3E%20%3E%20%2B//%20%20%20%20%20%20%20return%20TRUE%3B%5Cr%5Cn%3E%20%3E%20%2B//%20%20%20%20%20%7D%20else%5Cr%5Cn%3E%20%3E%20%2B//%20%20%20%20%20%7B%5Cr%5Cn%3E%20%3E%20%2B//%20%20%20%20%20%20%20return%20FALSE%3B%5Cr%5Cn%3E%20%3E%20%2B//%20%20%20%20%20%7D%5Cr%5Cn%3E%20I%20was%20just%20working%20on%20this.%20There%27s%20actually%20a%20bug%20here%20in%20that%20other%20parts%20of%20the%20code%20are%20expecting%20a%20return%20value.%20Some%20of%20that%27s%20legacy%20logic%2C%20and%20warrants%20turning%20into%20another%20issue%20to%20address%20that.%5Cr%5Cn%3E%20%5Cr%5Cn%3E%20%5Cu2014%5Cr%5Cn%3E%20Reply%20to%20this%20email%20directly%20or%20view%20it%20on%20GitHub.%22%2C%20%22created_at%22%3A%20%222014-07-16T02%3A00%3A36Z%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%22Updated.%20Works%20better%20now.%22%2C%20%22created_at%22%3A%20%222014-07-16T04%3A48%3A23Z%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%20zc_install/includes/classes/class.zcDatabaseInstaller.php%3AL91-104%22%7D%7D%7D'></a>
<a href='https://www.codereviewhub.com/'><img src='http://www.codereviewhub.com/site/github-bar.png' height=40></a>
- [x] <a href='#crh-comment-Pull a522f0ea17a4a7632ddb5f93153206f3c5cab21a zc_install/includes/classes/class.zcDatabaseInstaller.php 17'></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/100#discussion_r14977318'>File: zc_install/includes/classes/class.zcDatabaseInstaller.php:L91-104</a></b>
- <a href='https://github.com/texdc'><img border=0 src='https://avatars.githubusercontent.com/u/1590605?size=16' height=16 width=16'></a> If this isn't being used, it can be removed.  Source control will keep track of it.  Otherwise, please condense it.
- <a href='https://github.com/drbyte'><img border=0 src='https://avatars.githubusercontent.com/u/404472?size=16' height=16 width=16'></a> I was just working on this. There's actually a bug here in that other parts of the code are expecting a return value. Some of that's legacy logic, and warrants turning into another issue to address that.
- <a href='https://github.com/texdc'><img border=0 src='https://avatars.githubusercontent.com/u/1590605?size=16' height=16 width=16'></a> :+1: Carry-on, then. Wasn't sure if you had finished yet.
> On Jul 15, 2014, at 8:56 PM, Chris Brown <notifications@github.com> wrote:
>
> In zc_install/includes/classes/class.zcDatabaseInstaller.php:
>
> > @@ -91,13 +91,13 @@ public function parseSqlFile($fileName, $options = NULL)
> >      }
> >  //if (count($lines) < 200) sleep(5);
> >      $this->doJsonProgressLoggingEnd();
> > -    if (count($this->upgradeExceptions) > 0)
> > -    {
> > -      return TRUE;
> > -    } else
> > -    {
> > -      return FALSE;
> > -    }
> > +//     if (count($this->upgradeExceptions) > 0)
> > +//     {
> > +//       return TRUE;
> > +//     } else
> > +//     {
> > +//       return FALSE;
> > +//     }
> I was just working on this. There's actually a bug here in that other parts of the code are expecting a return value. Some of that's legacy logic, and warrants turning into another issue to address that.
>
> —
> Reply to this email directly or view it on GitHub.
- <a href='https://github.com/drbyte'><img border=0 src='https://avatars.githubusercontent.com/u/404472?size=16' height=16 width=16'></a> Updated. Works better now.
- [x] <a href='#crh-comment-Pull a522f0ea17a4a7632ddb5f93153206f3c5cab21a zc_install/includes/classes/class.systemChecker.php 46'></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/100#discussion_r14977323'>File: zc_install/includes/classes/class.systemChecker.php:L525-565</a></b>
- <a href='https://github.com/texdc'><img border=0 src='https://avatars.githubusercontent.com/u/1590605?size=16' height=16 width=16'></a> [Unnecessary](http://c2.com/cgi/wiki?ElseConsideredSmelly) `else`.
- [x] <a href='#crh-comment-Pull a522f0ea17a4a7632ddb5f93153206f3c5cab21a zc_install/includes/classes/class.systemChecker.php 10'></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/100#discussion_r14977365'>File: zc_install/includes/classes/class.systemChecker.php:L231-243</a></b>
- <a href='https://github.com/texdc'><img border=0 src='https://avatars.githubusercontent.com/u/1590605?size=16' height=16 width=16'></a> I don't really like this complex conditional - but at the very least, you can `return TRUE;` here.
- [ ] <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/100#issuecomment-49164964'>General Comment</a></b>
- <a href='https://github.com/texdc'><img border=0 src='https://avatars.githubusercontent.com/u/1590605?size=16' height=16 width=16'></a> @lhungil I agree, multiple return points are an indication that refactoring is needed, it's not just style.  2 returns is probably a good maximum.
- <a href='https://github.com/drbyte'><img border=0 src='https://avatars.githubusercontent.com/u/404472?size=16' height=16 width=16'></a> Ya, @zcwilt and I were thinking the same thing when we were troubleshooting a quirk in there yesterday. Definitely something to tackle in the future. We opted to wait since zc_install is used once at install time and then deleted. Doesn't have to be stellar code, so doesn't really require our immediate focus. A good project for when other priorities aren't competing.


<a href='https://www.codereviewhub.com/zencart/zc-v1-series/pull/100?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/100?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/100'><img src='http://www.codereviewhub.com/site/github-refresh.png' height=26></a>
<a href='#crh-end'></a>
:+1:<a href='#crh-update-complete'></a>