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

🐛 fixed error message code for HB translate helper #15529

Merged

Conversation

cweave
Copy link
Contributor

@cweave cweave commented Oct 5, 2022

Got some code for us? Awesome 🎊!

Please include a description of your change & check your PR against this list, thanks!

#15500 - Ghost has a policy to never throw 500 Internal Server errors for theme issues. Updated error handing to return a 400 error and a better error message "Oops, seems there is an error in the template."

This change adds a check inside of ghost\core\core\frontend\helpers\t.js if text or options is undefined, to throw an IncorrectUsageError error within the function.

Messaging was borrowed from ghost\core\core\frontend\web\middleware\error-handler.js.

image

Steps to Reproduce

  1. Open the default.hbs file of the current active theme
  2. Paste in the example translate helper with smart quotes {{t “This causes an error”}}
  3. Try to visit any page
  4. See the 400 error in the logs
[ghost] [2022-10-05 03:02:34] ERROR "GET /" 400 401ms
[ghost]
[ghost] [default.hbs] Oops, seems there is an error in the template.
[ghost]
[ghost] Error ID:
[ghost]     28e7d1e0-445a-11ed-adf2-afe89389d316
[ghost]
[ghost] ----------------------------------------
[ghost]
[ghost] IncorrectUsageError: [default.hbs] Oops, seems there is an error in the template.
[ghost]     at Object.t (C:\projects\Ghost\ghost\core\core\frontend\helpers\t.js:23:15)
[ghost]     at Object.wrapper (C:\projects\Ghost\node_modules\handlebars\dist\cjs\handlebars\internal\wrapHelper.js:15:19)
[ghost]     at Object.eval [as main] (eval at createFunctionContext (C:\projects\Ghost\node_modules\handlebars\dist\cjs\handlebars\compiler\javascript-compiler.js:262:23), <anonymous>:54:92)
[ghost]     at main (C:\projects\Ghost\node_modules\handlebars\dist\cjs\handlebars\runtime.js:208:32)
[ghost]     at ret (C:\projects\Ghost\node_modules\handlebars\dist\cjs\handlebars\runtime.js:212:12)
[ghost]     at ret (C:\projects\Ghost\node_modules\handlebars\dist\cjs\handlebars\compiler\compiler.js:519:21)
[ghost]     at renderTemplate (C:\projects\Ghost\node_modules\express-hbs\lib\hbs.js:490:13)
[ghost]     at _stackRenderer (C:\projects\Ghost\node_modules\express-hbs\lib\hbs.js:519:9)
[ghost]     at renderTemplate (C:\projects\Ghost\node_modules\express-hbs\lib\hbs.js:499:5)
[ghost]     at render (C:\projects\Ghost\node_modules\express-hbs\lib\hbs.js:526:5)
[ghost]     at renderIt (C:\projects\Ghost\node_modules\express-hbs\lib\hbs.js:588:18)
[ghost]     at C:\projects\Ghost\node_modules\express-hbs\lib\hbs.js:598:11
[ghost]     at _returnLayouts (C:\projects\Ghost\node_modules\express-hbs\lib\hbs.js:124:7)
[ghost]     at C:\projects\Ghost\node_modules\express-hbs\lib\hbs.js:135:7
[ghost]     at FSReqCallback.readFileAfterClose [as oncomplete] (node:internal/fs/read_file_context:68:3)
[ghost]

Ghost Version

5.17.x

I know the original issue stated v4.42.1, but I was consistently able to reproduce the issue in this version as well. If there is a need for that version as well, I am happy to investigate that as well.

When {{t “This causes an error”}) is used, text is coming into the t() function as undefined. I'm using Windows - unsure if that's causing something different as well, since someone else reported it was working fine in the issue.

  • There's a clear use-case for this code change, explained below
  • Commit message has a short title & references relevant issues
  • The build will pass (run yarn test:all and yarn lint)

We appreciate your contribution!

Also, if you'd be interested in writing code like this for us more regularly, we're hiring:
https://careers.ghost.org

closes TryGhost#15500
Per the issue, Ghost has a policy to never throw 500 Internal Server errors for theme issues. This change adds a check inside of `ghost\core\core\frontend\helpers\t.js` if `text` or `options` is undefined, to throw an `IncorrectUsageError` error within the function.

Messaging was borrowed from `ghost\core\core\frontend\web\middleware\error-handler.js`.
@codecov
Copy link

codecov bot commented Oct 5, 2022

Codecov Report

Base: 52.91% // Head: 52.89% // Decreases project coverage by -0.01% ⚠️

Coverage data is based on head (3f2d281) compared to base (289e8a8).
Patch coverage: 58.33% of modified lines in pull request are covered.

Additional details and impacted files
@@            Coverage Diff             @@
##             main   #15529      +/-   ##
==========================================
- Coverage   52.91%   52.89%   -0.02%     
==========================================
  Files        1378     1378              
  Lines       89183    89195      +12     
  Branches     9560     9563       +3     
==========================================
- Hits        47187    47184       -3     
- Misses      41048    41059      +11     
- Partials      948      952       +4     
Impacted Files Coverage Δ
ghost/core/core/frontend/helpers/t.js 62.16% <58.33%> (-9.84%) ⬇️
ghost/admin/app/helpers/gh-price-amount.js 44.44% <0.00%> (-22.23%) ⬇️
ghost/admin/app/components/gh-file-uploader.js 82.94% <0.00%> (-3.11%) ⬇️
...ost/core/core/server/models/base/plugins/events.js 69.48% <0.00%> (-1.48%) ⬇️
ghost/admin/app/controllers/offer.js 42.10% <0.00%> (+0.52%) ⬆️
ghost/admin/app/components/gh-site-iframe.js 45.45% <0.00%> (+2.27%) ⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@ErisDS ErisDS changed the title 🐛 fixed error message code for HB translate helper 🐛 fixed error message code for HB translate helper Oct 12, 2022
@ErisDS ErisDS merged commit 87d2166 into TryGhost:main Oct 12, 2022
@ErisDS
Copy link
Member

ErisDS commented Oct 12, 2022

Hi @cweave thanks so much for this PR and I'm sorry for taking so long to review it - we got a bit inundated.

The PR has now been merged 🎉 and will appear in the next release of Ghost - usually Fridays.

I'm not sure if you found this through hacktoberfest, but I've added the accepted label to this PR to make sure it counts.

If you'd like to take this further, I'd love to see a unit test added here for the underlying bug to ensure this can't happen again.

@cweave
Copy link
Contributor Author

cweave commented Oct 12, 2022

Thanks, @ErisDS ! I did indeed find it via Hacktoberfest. I'll take a look at unit testing as well ☺️

cweave added a commit to cweave/Ghost that referenced this pull request Oct 13, 2022
Added unit tests to the MR TryGhost#15529 / issue TryGhost#15500. Added a regex in the `t()` to explicitly check for smart apostrophes in the event that they are not stripped out/come through as undefined.
sam-lord pushed a commit that referenced this pull request Oct 17, 2022
closes: #15500

- Per the issue, Ghost has a policy to never throw 500 Internal Server errors for theme issues. This change adds a check inside of `ghost\core\core\frontend\helpers\t.js` if `text` or `options` is undefined, to throw an `IncorrectUsageError` error within the function.
- Messaging was borrowed from `ghost\core\core\frontend\web\middleware\error-handler.js`.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants