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.

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

Fixing config syntax #207

Merged
merged 8 commits into from
Mar 15, 2019
Merged

Fixing config syntax #207

merged 8 commits into from
Mar 15, 2019

Conversation

ChrisThompsonTLDR
Copy link
Contributor

This wasn't following Laravel syntax making it impossible to overload.

This is the normal way to overload a config param in Laravel

config(['sentry.breadcrumbs.sql_bindings' => false]);

this uses dot syntax.

The problem is that the config/sentry.php file uses breadcrumbs.sql_bindings which won't be overwrite-able with Laravel.

dump(config('sentry'));
config(['sentry.breadcrumbs.sql_bindings' => false]);
dd(config('sentry'));

will generate

array:2 [
  "dsn" => ""
  "breadcrumbs.sql_bindings" => true
]
array:2 [
  "dsn" => null
  "breadcrumbs.sql_bindings" => true
  "breadcrumbs" => array:1 [
    "sql_bindings" => false
  ]
]

The config param wasn't overwritten because config() can't accept a config param with a . in the name, it will map that to a multi-dimensional array.

Current syntax isn't compatible with Laravel config functions.
@stayallive
Copy link
Collaborator

stayallive commented Mar 8, 2019

I would say yes and no to this change for the following reasons:

  • Yes, you are correct, with that . in the name it's impossible to set using the config classes provided by Laravel.

  • No, because it has no use to change from user code... after your application has loaded this configuration has been read and changing it will not effect the registered events.

You can ofcourse still override it from your own config/sentry.php config file (you can publish with php artisan vendor:publish --provider="Sentry\Laravel\ServiceProvider").


But if we go through with this change I have 2 requests (still like the config to be ease to use too):

  • Make it backwards compatible so we don't have to wait for a breaking change release and we break as little as possible considering 1.0 was just released
  • Add tests for the above so we know it works and keeps working

Happy to add the above if you don't have the time just let me know.

@ChrisThompsonTLDR
Copy link
Contributor Author

@stayallive I'd be interested in having you do the tests. I'm not super familiar with writing tests and would appreciate seeing them written by someone that knows what they are doing.

I wasn't sure how to make it backwards compatible other than checking two different keys (old key with dot syntax and new key with array).

@stayallive
Copy link
Collaborator

stayallive commented Mar 11, 2019

So I won't claim I'm an expert on Laravel package testing but I think I came up with a good way to test this. Let me know what you think.

The funny thing is because it's not a runtime configuration property it made it a bit harder to test directly and had to add some scaffolding.

@stayallive stayallive requested a review from HazAT March 11, 2019 09:53
Copy link
Member

@HazAT HazAT left a comment

Choose a reason for hiding this comment

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

LGTM 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants