Skip to content

Added missing schema option #417

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

Merged
merged 1 commit into from
Jan 20, 2021
Merged

Added missing schema option #417

merged 1 commit into from
Jan 20, 2021

Conversation

dragosprotung
Copy link
Contributor

No description provided.

@ste93cry ste93cry added this to the 4.0 milestone Jan 20, 2021
@ste93cry
Copy link
Contributor

Thank you for the contribution, can you please add an entry to the CHANGELOG?

@dragosprotung
Copy link
Contributor Author

Done

@ste93cry ste93cry merged commit d09677e into getsentry:master Jan 20, 2021
@dragosprotung dragosprotung deleted the patch-1 branch January 20, 2021 17:23
@Jean85
Copy link
Contributor

Jean85 commented Jan 21, 2021

@ste93cry how did this pass with the tests? Do we miss something there?

@dragosprotung
Copy link
Contributor Author

The integration tests do not validate the XML configuration against the schema.
The field does appear in the tests (see https://github.com/getsentry/sentry-symfony/blob/master/tests/DependencyInjection/Fixtures/xml/full.xml#L29)

@ste93cry
Copy link
Contributor

ste93cry commented Jan 21, 2021

Do we miss something there?

Yes we do, the messenger config node is missing in tests/DependencyInjection/Fixtures/xml/full.xml and this caused the tests to not break by fallbacking to the default value for the option and covering the issue

The integration tests do not validate the XML configuration against the schema.

False, I explained above why the tests didn't break. Try adding the option without your change and Symfony will complain with [ERROR 1866] Element '{https://sentry.io/schema/dic/sentry-symfony}messenger', attribute 'capture-soft-fails': The attribute 'capture-soft-fails' is not allowed.. Would you mind opening a new PR to add the missing attribute and fix the tests?

@dragosprotung
Copy link
Contributor Author

Ah, i confused capture-silenced-errors with capture-soft-fails in the xml fixtures. Opened #420

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

Successfully merging this pull request may close these issues.

3 participants