-
-
Notifications
You must be signed in to change notification settings - Fork 171
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
Update index.php #842
Update index.php #842
Conversation
Explanation of changes: 1. Extracted the runtime options array into a separate variable for better readability. 2. Used the runtime options array when accessing the context values, making it easier to change the options in the future. 3. Introduced a null check when accessing the environment variable to avoid warnings if it's not present in the context. 4. Used the null coalescing operator (??) to provide a default value of null for the environment variable if it's not present in the context. 5. Simplified the debug variable assignment using the ternary operator and explicit type casting to bool. 6. Added a use statement to import the $runtimeOptions variable into the anonymous function, allowing access to the options within the function.
Update README.md
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## 2.3.x #842 +/- ##
============================================
+ Coverage 50.51% 50.71% +0.19%
+ Complexity 2208 2179 -29
============================================
Files 437 438 +1
Lines 8241 8026 -215
============================================
- Hits 4163 4070 -93
+ Misses 4078 3956 -122
Flags with carried forward coverage won't be shown. Click here to find out more.
☔ View full report in Codecov by Sentry. |
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.
Thanks for the change! Just a few minor comments
@@ -11,13 +11,18 @@ | |||
|
|||
use SolidInvoice\Kernel; | |||
|
|||
$_SERVER['APP_RUNTIME_OPTIONS'] = [ | |||
$runtimeOptions = [ | |||
'env_var_name' => 'SOLIDINVOICE_ENV', |
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.
These values might be better of to be defined as a constant if we want to make them re-usable
return static function (array $context) { | ||
return new Kernel($context['SOLIDINVOICE_ENV'], (bool) $context['SOLIDINVOICE_DEBUG']); | ||
return static function (array $context) use ($runtimeOptions) { | ||
$environment = $context[$runtimeOptions['env_var_name']] ?? null; |
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 think we should set a default environment here (maybe prod
?) Having the environment as null
will cause some issues, since a lot of the standard config won't be loaded. The kernel also expects a string as the environment value, so this might cause an error if we use null
.
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.
Actually, I'm not sure if there will be a case where these values are not set. Since we use the Symfony runtime, it loads the values by default from the .env
file, so we should always have a default value
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.
Understood I do agree with you.
Explanation of changes: