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

Do not catch errors from Slimmer processors #203

Merged
merged 2 commits into from Aug 14, 2017
Merged

Conversation

@fofr
Copy link
Contributor

@fofr fofr commented Aug 14, 2017

When the body inserter fails, the page errors but the resulting error gets cached. This means the cached error pages get seen by users. Raise errors to avoid caching an incorrect page.

Now we are no longer catching errors we do not need to specifically call Airbrake. We are also moving away from Airbrake.

Follow on from #202 and alphagov/government-frontend#446

cc @tijmenb

When the body inserter fails, the page errors but the resulting error
gets cached. This means the cached error pages gets seen by users.

Raise error to avoid caching an incorrect page.
@fofr fofr force-pushed the raise-errors-without-airbrake branch from 0af3772 to ef5830c Aug 14, 2017
@thomasleese thomasleese force-pushed the raise-errors-without-airbrake branch 2 times, most recently from f0c3559 to ef5830c Aug 14, 2017
@tijmenb
Copy link
Contributor

@tijmenb tijmenb commented Aug 14, 2017

👍 Can we also remove the strict option? Looks like it's not being used after this.

@fofr
Copy link
Contributor Author

@fofr fofr commented Aug 14, 2017

The strict option also warns about invalid HTML during development/test. eg Multiple elements with the same ID.

@tijmenb
Copy link
Contributor

@tijmenb tijmenb commented Aug 14, 2017

Ah, I see, GH search didn't return it for me: https://github.com/alphagov/slimmer/search?utf8=✓&q=strict

Catching these errors may lead to an incorrectly processed page being
cached.

Now we are no longer catching errors we do not need to specifically
call Airbrake. We are also moving away from Airbrake.
@fofr fofr force-pushed the raise-errors-without-airbrake branch from ef5830c to 8884875 Aug 14, 2017
@fofr fofr merged commit 556198e into master Aug 14, 2017
1 check passed
1 check passed
continuous-integration/jenkins/branch This commit looks good
Details
@fofr fofr deleted the raise-errors-without-airbrake branch Aug 14, 2017
fofr added a commit that referenced this pull request Aug 14, 2017
* Stop catching errors from Slimmer processors (#203)
* Removes all references to Airbrake
@fofr fofr mentioned this pull request Aug 14, 2017
fofr added a commit that referenced this pull request Aug 14, 2017
* Stop catching errors from Slimmer processors (#203)
* Removes all references to Airbrake
36degrees added a commit that referenced this pull request Sep 19, 2017
Some applications (like Service Manual Frontend) opt into using Slimmer for their javascript tests (so that they get e.g. jQuery) but in a test context the header logo is stubbed out, so this breaks.

This was previously masked by Slimmer swallowing exceptions raised within the processors, but this was removed in #203.
36degrees added a commit that referenced this pull request Sep 19, 2017
Some applications (like Service Manual Frontend) opt into using Slimmer for their javascript tests (so that they get e.g. jQuery) but in a test context the header logo is stubbed out, so this breaks.

This was previously masked by Slimmer swallowing exceptions raised within the processors, but this was removed in #203.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants
You can’t perform that action at this time.