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

809: Added double quotation marks character to Android escape function #627

Closed
wants to merge 3 commits into
base: develop
from

Conversation

Projects
None yet
3 participants
@ignaciogs
Contributor

ignaciogs commented Dec 28, 2016

No description provided.

Show outdated Hide outdated gp-includes/formats/format-android.php
@@ -161,7 +161,7 @@ private function unescape( $string ) {
*/
protected function escape( $string ) {
$string = addcslashes( $string, "'\n" );
$string = str_replace( array( '&', '<' ), array( '&amp;', '&lt;' ), $string );

This comment has been minimized.

@toolstack

toolstack Dec 29, 2016

Contributor

The escaping should be done on the line above (addcslashes()).

In addition, do we need to escape \ as well?

@toolstack

toolstack Dec 29, 2016

Contributor

The escaping should be done on the line above (addcslashes()).

In addition, do we need to escape \ as well?

This comment has been minimized.

@toolstack

toolstack Jan 10, 2017

Contributor

@ignaciogs have you had a chance to look at the above?

@toolstack

toolstack Jan 10, 2017

Contributor

@ignaciogs have you had a chance to look at the above?

This comment has been minimized.

@ignaciogs

ignaciogs Jan 11, 2017

Contributor

Hi, I Added a new commit

@ignaciogs

ignaciogs Jan 11, 2017

Contributor

Hi, I Added a new commit

@ocean90 ocean90 changed the title from Added double quotation marks character to Android escape function to 628: Added double quotation marks character to Android escape function Jan 10, 2017

@ocean90 ocean90 changed the title from 628: Added double quotation marks character to Android escape function to Added double quotation marks character to Android escape function Jan 10, 2017

Show outdated Hide outdated gp-includes/formats/format-android.php
@@ -161,7 +161,8 @@ private function unescape( $string ) {
*/
protected function escape( $string ) {
$string = addcslashes( $string, "'\n" );
$string = str_replace( array( '&', '<', '"' ), array( '&amp;', '&lt;', '\"' ), $string );
$string = addcslashes( $string, '"' );

This comment has been minimized.

@toolstack

toolstack Jan 11, 2017

Contributor

You don't need a second call to addcslashes (it may in fact break things), simply add the double quote to the list of characters to escape (so "'\n" becomes "'\n"").

@toolstack

toolstack Jan 11, 2017

Contributor

You don't need a second call to addcslashes (it may in fact break things), simply add the double quote to the list of characters to escape (so "'\n" becomes "'\n"").

@toolstack

This comment has been minimized.

Show comment
Hide comment
@toolstack

toolstack Jan 17, 2017

Contributor

@ignaciogs can you update the test data to include the escaped double quotes here:

https://github.com/GlotPress/GlotPress-WP/blob/develop/tests/phpunit/data/translation.android.xml#L12

Contributor

toolstack commented Jan 17, 2017

@ignaciogs can you update the test data to include the escaped double quotes here:

https://github.com/GlotPress/GlotPress-WP/blob/develop/tests/phpunit/data/translation.android.xml#L12

@ocean90

This comment has been minimized.

Show comment
Hide comment
@ocean90

ocean90 Mar 14, 2017

Member
  • Needs an issue according to our contributing rules which explains and documents
  • Unit tests need to be updated
Member

ocean90 commented Mar 14, 2017

  • Needs an issue according to our contributing rules which explains and documents
  • Unit tests need to be updated

@toolstack toolstack added this to the 3.0 milestone Jul 11, 2017

@toolstack toolstack changed the title from Added double quotation marks character to Android escape function to 809: Added double quotation marks character to Android escape function Sep 1, 2017

@toolstack

This comment has been minimized.

Show comment
Hide comment
@toolstack

toolstack Sep 1, 2017

Contributor

Closing in favour of #811 which includes updated data.

Contributor

toolstack commented Sep 1, 2017

Closing in favour of #811 which includes updated data.

@toolstack toolstack closed this Sep 1, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment