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

fix: don't run middleware in Sinatra when not configured #629

Merged
merged 1 commit into from
Nov 5, 2024

Conversation

tadast
Copy link
Contributor

@tadast tadast commented Nov 4, 2024

While #549 has removed middleware loading for Rails unless configured, it has also removed the code that prevented the middleware from executing in other frameworks unless configured i.e. return @app.call(env) unless config[:FLAG].

A similar change to rails initializer in #549 is now also required for Sinatra (perhaps the same for Hanami?), because currently it loads and executes the feedback and user informer middleware even when they are not configured.

@tadast tadast force-pushed the tt/lazy-middleware branch from e79f7d1 to 0803230 Compare November 4, 2024 18:36
@tadast tadast changed the title Do not load/execute feedback and informer middleware in Sinatra when they are not configured fix: Do not load/execute feedback and informer middleware in Sinatra when they are not configured Nov 4, 2024
@tadast tadast changed the title fix: Do not load/execute feedback and informer middleware in Sinatra when they are not configured fix: don't run feedback and informer middleware in Sinatra when not configured Nov 4, 2024
@tadast tadast force-pushed the tt/lazy-middleware branch from 0803230 to b58de71 Compare November 4, 2024 18:37
@tadast tadast changed the title fix: don't run feedback and informer middleware in Sinatra when not configured fix: don't run middleware in Sinatra when not configured Nov 4, 2024
@tadast
Copy link
Contributor Author

tadast commented Nov 4, 2024

Not sure why the tests are failing, they're failing for me on master too...

@roelbondoc
Copy link
Member

Not sure why the tests are failing, they're failing for me on master too...

@tadast thanks for this. I've updated master to fix those broken tests. If you are able, could you merge in the latest changes from our master branch into yours? Once the tests pass we can merge this in.

While honeybadger-io#549
has removed middleware loading for Rails unless configured, it has
also removed the code that prevented the middleware from executing
unless configured.

A similar change is also required for Sinatra, because currently
it tries to execute when not configured.

When a Sinatra app does not use i18n gem and has user feedback
disabled, it errs with a "I18n::InvalidLocale - :en is not a valid
locale" exception from ERB, because Honeybadger tries to render
the feedback form.
@tadast tadast force-pushed the tt/lazy-middleware branch from b58de71 to e5e1ef3 Compare November 5, 2024 19:44
@tadast
Copy link
Contributor Author

tadast commented Nov 5, 2024

Nice one, looking green now @roelbondoc
Thanks

@roelbondoc roelbondoc merged commit d84c2b1 into honeybadger-io:master Nov 5, 2024
59 checks passed
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.

2 participants