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

Fix comment history with % sign. #22

Closed
wants to merge 1 commit into from
Closed

Fix comment history with % sign. #22

wants to merge 1 commit into from

Conversation

JustinElst
Copy link
Contributor

The issue will be solved in the upcomming 1.9.2 release of magento.
See the issues below:
http://www.magentocommerce.com/bug-tracking/issue/index/id/697
http://www.magentocommerce.com/bug-tracking/issue/index/id/519

Steps to reproduce:
Add comment to order in 'Comments History' containing '%' symbol

Expected Result:
Comment should be displayed after being saved

Actual Result:
Comment without message content is being displayed

The issue will be solved in the upcomming 1.9.2 release of magento.
See the issues below:
http://www.magentocommerce.com/bug-tracking/issue/index/id/697
http://www.magentocommerce.com/bug-tracking/issue/index/id/519

Steps to reproduce:
Add comment to order in 'Comments History' containing '%' symbol

Expected Result:
Comment should be displayed after being saved

Actual Result:
Comment without message content is being displayed
@Flyingmana
Copy link
Contributor

against merging it without good review
@LeeSaferite see Skype for reason

@JustinElst
Copy link
Contributor Author

Could you tell me how I could assist in a 'good review' because this works for me, but I know that it could break other parts of the code..
So how would we go about this.
And I am interested in the reason that you conveyed to @LeeSaferite

@JustinElst JustinElst closed this Jun 18, 2015
@LeeSaferite
Copy link
Contributor

@MrGekko The concern expressed by @Flyingmana over Skype was just that he didn't want to make the issue any worse and felt the issue warranted more review.

Having looked over the bug reports and the change I'm personally ok with the fix, but I think it may be prudent to wait a short amount of time to see what to official fix in 1.9.2 happens to be. I'll also check the existing official patches to see if the issue is addressed as well.

@LeeSaferite LeeSaferite reopened this Jun 18, 2015
@LeeSaferite LeeSaferite self-assigned this Jun 18, 2015
@LeeSaferite
Copy link
Contributor

Also, one other suggested solution was:

Do the str_replace with the HTML equivalent %

$data = str_replace('%', '%', $data);

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
hold review needed Problem should be verified
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants