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

Add escapeQuote and fix error on js addGroup function #3271

Closed
wants to merge 4 commits into from

Conversation

Imaginaerum
Copy link
Contributor

Add escapeQuote and fix error on js addGroup function.

@vkublytskyi
Copy link

@Imaginaerum, thank you for contribution.

Please note that your fix removes translation. Even if these strings are translated later by JavaScript we can not remove PHP translation now as this will be breaking incompatible change.

Also for escaping inside JavaScript strings escapeJsQuote instead of escapeQuote should be used.

So the correct code will be:
'<?php /* @escapeNotVerified */ echo $block->escapeJsQuote(__('...')) ?>'

Please correct your code and we will merge PR

@Imaginaerum
Copy link
Contributor Author

@vkublytskyi thanks for your reply.

I change my code.

@vrann
Copy link
Contributor

vrann commented Mar 20, 2017

@Imaginaerum please sync the PR with the latest develop branch.

@vrann vrann self-assigned this Mar 20, 2017
@vrann
Copy link
Contributor

vrann commented Mar 22, 2017

@Imaginaerum also, escapeJsQuote method was deprecated. Please use escapeJs method instead.
Doing so, please remove /* @escapeNotVerified */ comment next to the method call. It is needed to highlight unescaped string to static tests, which will not be the case anymore.

@vrann
Copy link
Contributor

vrann commented Mar 25, 2017

@Imaginaerum please feel free to re-open PR once the changes are made.

@vrann vrann closed this Mar 25, 2017
@vrann vrann added this to the March 2017 milestone Mar 25, 2017
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.

6 participants