refactor(app): modernize and tidy up public entrypoint#7463
refactor(app): modernize and tidy up public entrypoint#7463joshtrichards wants to merge 8 commits intomainfrom
Conversation
895a860 to
4a2d5bb
Compare
|
There are unfortunately some conflicts now, so this needs a rebase |
|
I like some of these changes, e.g. adding type hints. But having all changes in one commit makes it really hard to review, especially if actual changes are mixed with reordering lines. For a proper review I would need this cleaned up into smaller commits, or at least stripped by the reordering. |
4a2d5bb to
2d75a54
Compare
Signed-off-by: Josh Richards <josh.t.richards@gmail.com>
Signed-off-by: Josh Richards <josh.t.richards@gmail.com>
Signed-off-by: Josh Richards <josh.t.richards@gmail.com>
Signed-off-by: Josh Richards <josh.t.richards@gmail.com>
Signed-off-by: Josh Richards <josh.t.richards@gmail.com>
Signed-off-by: Josh Richards <josh.t.richards@gmail.com>
Signed-off-by: Josh <josh.t.richards@gmail.com>
Signed-off-by: Josh Richards <josh.t.richards@gmail.com>
a825df9 to
2cc9f67
Compare
Fixed. |
| //------------------------------------------------- | ||
|
|
||
| // TODO: Figure out why the default plain text renderer is being used by logging | ||
| // TODO: Change logging to not generate stack traces for 404s |
There was a problem hiding this comment.
@pabzm didnt you find a solution to this already? :)
There was a problem hiding this comment.
Indeed, and I had meant to post it, which I forgot. Thank you for the reminder!
See #7631
There was a problem hiding this comment.
Thank you! I guess this todo can after the merge of #7631 then be removed, right? :)
There was a problem hiding this comment.
@joshtrichards please remove this todo here after #7631 is merged. Thanks! :)
| //------------------------------------------------- | ||
|
|
||
| // TODO: Figure out why the default plain text renderer is being used by logging | ||
| // TODO: Change logging to not generate stack traces for 404s |
There was a problem hiding this comment.
@joshtrichards please remove this todo here after #7631 is merged. Thanks! :)
| }); | ||
|
|
||
| // Auth Redirector | ||
| $app->get('/', function (\Psr\Http\Message\RequestInterface $request, Response $response, array $args) use ($container) { |
There was a problem hiding this comment.
\Psr\Http\Message\RequestInterface is not the same as Request, which is taken from Psr\Http\Message\ServerRequestInterface, please don't change that.
pabzm
left a comment
There was a problem hiding this comment.
Thank you for this effort! I would be willing to merge it if you address the small issues.
| ini_set('max_execution_time', AIO_MAX_EXECUTION_TIME); // Prevent timeouts on slow networks | ||
| ini_set('session.cookie_lifetime', AIO_COOKIE_LIFETIME); // Auto-logout on browser close | ||
| ini_set('session.gc_maxlifetime', AIO_SESSION_MAX_LIFETIME); // 24h session duration | ||
| ini_set('log_errors_max_len', AIO_LOG_ERRORS_MAX_LEN); // Full error logs |
There was a problem hiding this comment.
With the abstractions of the values in constants we cannot refer to the default values in comments here. If they ever get changed, the comments would get confusing.
php/public/index.php
Outdated
| 'is_dri_device_enabled' => $configurationManager->nextcloudEnableDriDevice, | ||
| 'is_nvidia_gpu_enabled' => $configurationManager->enableNvidiaGpu, | ||
| 'is_talk_recording_enabled' => $configurationManager->isTalkRecordingEnabled, | ||
| 'is_docker_socket_proxy_enabled' => $configurationManager->isDockerSocketProxyEnabled, |
There was a problem hiding this comment.
Please fix the indentation here (should be spaces, not a tab)
| 'was_start_button_clicked' => $configurationManager->wasStartButtonClicked, | ||
| 'has_update_available' => $dockerActionManager->isAnyUpdateAvailable(), | ||
| 'is_mastercontainer_update_available' => ( $bypass_mastercontainer_update ? false : $dockerActionManager->IsMastercontainerUpdateAvailable() ), | ||
| 'is_mastercontainer_update_available' => ( $bypassMastercontainerUpdate ? false : $dockerActionManager->IsMastercontainerUpdateAvailable() ), |
There was a problem hiding this comment.
This whole commit is a fixup for f88957a#r2851763584, isn't it?
For next time I'd like to ask you to squash such commits before requesting a review. Or even better: split off the variable renaming into a separate commit, so it stays atomic. Thanks!
Code was already pretty tidy; this just elevates it a bit more while cleaning up some minor tidbits.
php/public/index.phpfor improved readability, maintainability, and adherence to best practices$groupin SlimServerRequestInterfacerather thanRequestInterfaceprofilealias for the/containers/routeNot offended if you don't merge this, but I was reading through it anyhow. ;-)
Shouldn't be any breaking changes; largely cosmetic. If something is broken it's probably a typo on my part. ;-)