Skip to content

Conversation

@vovayatsyuk
Copy link
Member

Description

Magento 2.2.4 only.
When a product has " symbol in its name - js error appears on the page.

Manual testing scenarios

  1. Add " to the product name in the backend.
  2. Open this product on the frontend.
  3. Breadcrumbs are missing and product image block is not initialized. Console has the following error:
Uncaught SyntaxError: Unexpected string in JSON at position ..

screen shot 2018-05-11 at 10 03 46 pm

Contribution checklist

  • Pull request has a meaningful description of its purpose
  • All commits are accompanied by meaningful commit messages
  • All new or changed code is covered with unit/integration tests (if applicable)
  • All automated tests passed successfully (all builds on Travis CI are green)

@magento-engcom-team
Copy link
Contributor

Hi @vovayatsyuk. Thank you for your contribution.
Changes from your Pull Request will be available with the upcoming 2.2.5 release.

@tdgroot
Copy link
Member

tdgroot commented May 16, 2018

@magento-engcom-team isn't escapeJsQuote() deprecated? What's the policy on this?

@vovayatsyuk
Copy link
Member Author

Oops. Didn't noticed that. What should be used instead?

@tdgroot
Copy link
Member

tdgroot commented May 16, 2018

@vovayatsyuk Not sure, we tried using escapeJs(which isn't deprecated), but that resulted in a regression. I expect input from @magento-engcom-team and Magento core team on what the right method would be.

@t-richards
Copy link
Contributor

we tried using escapeJs(which isn't deprecated), but that resulted in a regression

What regression did you encounter when using this method? It seems to work for me, but the \u0020 conversion looks silly and is unreadable.

Also, escapeHtml seems unnecessary. Since we're in a JSON context, json_encode should suffice for encoding this value. This appears to be how other init objects work:

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.

5 participants