-
Notifications
You must be signed in to change notification settings - Fork 12
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
Deprecate not passing server base url and secret #195
Deprecate not passing server base url and secret #195
Conversation
…utton-server-configuration-trough-env
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #195 +/- ##
============================================
- Coverage 99.02% 98.93% -0.09%
- Complexity 475 477 +2
============================================
Files 53 53
Lines 1123 1127 +4
============================================
+ Hits 1112 1115 +3
- Misses 11 12 +1 ☔ View full report in Codecov by Sentry. |
…utton-server-configuration-trough-env
Thank you, nice to do it already in 6.0 :) . |
…utton-server-configuration-trough-env
if (empty($secret)) { | ||
@trigger_error(sprintf('Constructing "%s" without passing a secret is deprecated and will throw an exception 6.0.', self::class), \E_USER_DEPRECATED); | ||
} | ||
|
||
// Keeping backward compatibility with older deployed versions |
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 trigger a notice when this is present in the environment to ensure that developers remove their hacks to get around this :) .
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.
So just replacing \E_USER_DEPRECATED
with \E_USER_NOTICE
?
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.
No, deprecated is fine and will be displayed by common reporting tools like Symfony profiler. notice would cause throwing an exception e.g. with Symfony error handler.
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.
Ok. So what should I change? Do you want to make the change?
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 would just compare if getenv
returns something else than false
for both variables and would trigger a deprecation notice then that reminds the consumer to stop passing environment variables for security reasons.
Co-authored-by: Felix Jacobi <felix@jacobi-hamburg.net>
Co-authored-by: Felix Jacobi <felix@jacobi-hamburg.net>
No description provided.