Skip to content
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.

Sign up for GitHub

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

Avoid fatal error when creating local directories #116

Merged
merged 1 commit into from
May 15, 2023
Merged

Avoid fatal error when creating local directories #116

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion src/StorageDirectories.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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')) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we consider using php://stderr?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since this is for local development only, I find the message just noisy and unnecessary, but no strong feelings either way.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is interesting, I think this surfaces 2 different problems:

  1. STDERR is supposed to be a PHP constant, but it's not always defined
  2. These messages are not useful in local development

Regarding 1, the docs mention these constants should be available:

image

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?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 🤷‍♂️

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • 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?

Copy link
Member Author

@deleugpn deleugpn May 3, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💯 makes sense, yeah 1 is the most important part indeed!

fwrite(STDERR, 'Creating storage directories: ' . implode(', ', $directories) . PHP_EOL);
}

Expand Down