These are chat archives for
Sign in to start talking
Issue tracking system
Nov 19 2015 02:58
Hmmmm. I'd say go ahead and make a PR for the first one and if all looks good and works well then I think it will be fine to convert a pages incrementally and push them to master
Nov 19 2015 07:27
created, i'll let you know when it's ready to merge
i'll take view.php as next, i hope you don't need to modify it soon :)
Nov 19 2015 19:47
Misc::displayErrorMessage($msg) renders $msg with error template and exits. i think some exception would be appropriate in controllers, what you think?
altho i could add $this->error() method for now
Nov 19 2015 20:52
view.php ported, but could you check maybe the project id check can be dropped as issue::access already does the same thing?
and do you look at individual commits if you review my pulls?
Nov 19 2015 21:43
Changing to an exception would probably be the "proper" way to do it. I wouldn't want to change it to $this->error incase a workflow or other non controller code needs to call it
I generally just look at files changed, not individual commits
Nov 19 2015 21:57
The explicit project check (on line 117) can be dropped since as you pointed out issue::access checks that
Everything looks good to me, I haven't run it yet just read the diffs. I barely recognize the code :)
You can merge it or if you like I'll check it into my machine tomorrow and run it
Nov 19 2015 22:07
I think i port more classes still
Ordering by file size :)
And maybe checking most accessed files from my system :)
Nov 19 2015 22:09
When you do the update page you can probably reuse the logic in getColumnsForDisplay. Not sure best way to share logic like that
Nov 19 2015 22:11
If multiple controllers share method, there should be new class for that. Afaik zend calls them view helpers
Maybe should use zend mvc here?
It's all new to me
I think the harder part is moving foo.php to these classes, later can add real framework