Avoid fatal error when creating local directories #116
Conversation
| @@ -31,7 +31,7 @@ public static function create() | |||
|
|
|||
| $directories = array_filter($directories, static fn ($directory) => ! is_dir($directory)); | |||
|
|
|||
| if (count($directories)) { | |||
| if (count($directories) && defined('STDERR')) { | |||
There was a problem hiding this comment.
Should we consider using php://stderr?
There was a problem hiding this comment.
Since this is for local development only, I find the message just noisy and unnecessary, but no strong feelings either way.
There was a problem hiding this comment.
This is interesting, I think this surfaces 2 different problems:
STDERRis supposed to be a PHP constant, but it's not always defined- These messages are not useful in local development
Regarding 1, the docs mention these constants should be available:
But this bug suggests they should only exist in CLI environments: https://bugs.php.net/bug.php?id=69452 So that makes a bit more sense in retrospect.
That being said, since the constant doesn't always exist, I think we shouldn't rely on it, and we should mess with it (re-define it) since we cannot cover scenarios when running outside of Lambda/Bref-controlled environment.
So I'd suggest we move away from the constant and go with @tillkruss's suggestion.
Regarding 2, we could disable the log message in local development?
There was a problem hiding this comment.
I think my PR achieves exactly that. When running inside Lambda, bref-init.php runs so the constant is always available https://github.com/brefphp/laravel-bridge/blob/master/src/bref-init.php#L10-L12
When running on local, the constant may not be available (because Bref::beforeStartup() didn't run) and since the constant is not defined, it will not attept to write to stderr.
Btw, sorry for the delay on answering, for some reason I'm not getting email notification about this PR even though I'm subscribed to it 🤷♂️
There was a problem hiding this comment.
- If we merge this and later solve 1 (move away from the constant), then this bit here will be "broken" (messages will never show) and will need to be updated.
- If we don't merge this PR and later solve 1, nothing will break (but 2 will not be solved ofc).
My point is that, given a choice, I'd rather not merge this PR as it will create extra work down the road when/if we solve problem 1.
Is there any other way we can solve 2 ("disable the log message in local development") with a different solution that doesn't rely on checking whether STDERR is defined?
There was a problem hiding this comment.
@mnapoli thanks now I got it! Smart thinking. Given the clarity I would go back to my first comment and say 2) is not that important. If you ever solve for 1) and suddenly my local starts popping up a message right when containers start up, it's not the end of the world. I guess what I'm saying is that this PR avoids a crash and hides an unimportant message. If the crash is avoided by a guaranteed of the constant being defined then the unimportant message starts showing up but the crash is still avoided, so no harm done.
At the end of the day I want 3) don't crash if you can't write an unimportant message. So I will get 2) and 3) with this PR and later this might get swapped into 1) and 3) instead, which seems to me better than involving an environmental check here.
Let me know your thoughts after this and I will work out something accordingly. I think that there won't be any new insights afterwards and we can come to a conclusion on the preference and move forward.
There was a problem hiding this comment.
💯 makes sense, yeah 1 is the most important part indeed!
|
@mnapoli @tillkruss thoughts on this? |
As part of my migration to Bref v2, I stumbled on #115. To solve the issue of the build server crashing with autoload, I changed my
bootstrap/app.phpfile to invoke theStorageDirectories::create()function. This works on the build server and on Lambda, but crashes locally because theBref::beforeStartupcallback is never executed, soSTDERRconstant definition is never defined. Since this is just a helper output and not something critical, I thought we could get away by attempting to write to STDERR only if the constant has been defined so that it still works on local development without going through Bref startup while still making sure that the folders will be created