-
Notifications
You must be signed in to change notification settings - Fork 15
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Pulling back new changes from deployment #396
Conversation
app/Console/Commands.php
Outdated
@@ -83,9 +83,10 @@ public static function latexToPdf($path, $outputDir) | |||
if (self::isDebugMode()) { | |||
$result = "ok"; | |||
} else { | |||
$command = "pdflatex " . "-interaction=nonstopmode -output-dir " . $outputDir . " " . $path . " 2>&1"; | |||
$command = "/usr/bin/pdflatex " . "-interaction=nonstopmode -output-dir " . $outputDir . " " . $path . " 2>&1"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
add the full path to the config then
app/Http/Middleware/TrustProxies.php
Outdated
@@ -12,7 +12,7 @@ class TrustProxies extends Middleware | |||
* | |||
* @var array|string | |||
*/ | |||
protected $proxies; | |||
protected $proxies = ['172.5.0.10']; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's this and why?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We are running mars behind a reverse proxy and setting the reverse proxy as a trusted proxy for Laravel is mandatory if we want to use HTTPS. (https://laravel.com/docs/9.x/requests#configuring-trusted-proxies) There are some other hacky ways of setting it up but I didn't like them.
I would like it to be configurable from the .env, and I will add this to the PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay, and add a comment here. Will this affect local development?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not 100% sure, and have not tested it, but I think it only comes in to effect if at least one of the headers in the request:
Request::HEADER_X_FORWARDED_FOR |
Request::HEADER_X_FORWARDED_HOST |
Request::HEADER_X_FORWARDED_PORT |
Request::HEADER_X_FORWARDED_PROTO |
Request::HEADER_X_FORWARDED_AWS_ELB
Which are mostly stuff that can come from a reverse proxy / load-balancer. But I think it should not have any effect unless you are connecting from IP 172.5.0.10
and the APP_URL
's protocol is https.
I will look into it today or tomorrow, but I think it should not have any effect for local development.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have tested it in my local environment and have not found any issues. So I think it should be fine
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could we make this configurable instead of hard-coding the IP in the source code?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep, I would like to make it configurable, I just currently have a long lead-time
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Or if someone would fix it, I would happily give advice and / or check the code
Unfortunately I'm currently quite busy with my other tasks and other issues with the server. I'm planning on fixing the issues with this PR in about 3-4 weeks. |
PRINTER_ADDITIONAL_ARGS= | ||
PRINTER_STAT_ADDITIONAL_ARGS= |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure how much of the changes to printing will be necessary once the printer server is redesigned (eotvoscollegium/smaugreinstall#8). I'm planning on tackling that task on Thursday; could we delay this part until then?
* Added config for commands * style: format code with PHP CS Fixer This commit fixes the style issues introduced in f1d870d according to the output from PHP CS Fixer. Details: #400 * Fixed a bug and added some comments * style: format code with PHP CS Fixer This commit fixes the style issues introduced in 6e41962 according to the output from PHP CS Fixer. Details: #400 * Fixed command name * style: format code with PHP CS Fixer This commit fixes the style issues introduced in 6dfdd35 according to the output from PHP CS Fixer. Details: #400 --------- Co-authored-by: deepsource-autofix[bot] <62050782+deepsource-autofix[bot]@users.noreply.github.com>
* Added config for commands * style: format code with PHP CS Fixer This commit fixes the style issues introduced in f1d870d according to the output from PHP CS Fixer. Details: #400 * Fixed a bug and added some comments * style: format code with PHP CS Fixer This commit fixes the style issues introduced in 6e41962 according to the output from PHP CS Fixer. Details: #400 * Fixed command name * fix typo --------- Co-authored-by: deepsource-autofix[bot] <62050782+deepsource-autofix[bot]@users.noreply.github.com>
This is to merge the improvements @horcsinbalint has made to the development branch.
@kdmnk what do you think?
I can review it, but maybe only on Sunday or after that.Oh, the system doesn't let me do it; since it's technically my PR... but I can add comments, so it could be solved.