[ticket/17469] Automatic updater front end#6864
Conversation
2b9d103 to
de6bd65
Compare
PHPBB3-17469
| }); | ||
| if (!response.ok) | ||
| { | ||
| phpbb.alert(response.status, response.statusText); |
There was a problem hiding this comment.
Was the idea to just output the status id and the text without any language parsing?
| { | ||
| case 'error': | ||
| phpbb.alert(resp.status, resp.error); | ||
| return; |
There was a problem hiding this comment.
i have no problem with it, but you may or may not want further code to execute based on the status, hence return seemed more fitting.
| arguments: | ||
| - '@filesystem' | ||
| - '@updater.get_updates' | ||
| - '@language' |
There was a problem hiding this comment.
we typically try to order these alphabetically
| { | ||
| $recheck = $request->variable('versioncheck_force', false); | ||
| $do_update = $request->variable('do_update', false); | ||
| $token = $request->variable('form_token', false); |
There was a problem hiding this comment.
Since token is a string shouldn't we kind of default to ''?
There was a problem hiding this comment.
it is easier to do the check if the default is false, does this break any parsing/validation?
| { | ||
| if ($this->user->data['user_type'] != USER_FOUNDER) | ||
| { | ||
| throw new http_exception(403); |
There was a problem hiding this comment.
You might want to add a language string for this:
throw new http_exception(403, 'INSTALL_PHPBB_NOT_INSTALLED');
There was a problem hiding this comment.
Maybe, but it should be inaccessible for other users anyway - as in link hidden, so probably you called this thing by hand, in which case no information for you.
There was a problem hiding this comment.
I wonder if we should maybe have these strings in a container parameter file
There was a problem hiding this comment.
i think that would make it harder to mock? the purpose here is to have them modifiable for test cases.
There was a problem hiding this comment.
also not sure if this is actually interface worthy :D
There was a problem hiding this comment.
it is not, but if you want to change them in the tests, this seems reasonable. although we could do mock objects as well i guess.
PHPBB3-17469
Checklist:
Tracker ticket:
https://tracker.phpbb.com/browse/PHPBB-17469