search

wp-shortcake / shortcake

Shortcake makes using WordPress shortcodes a piece of cake.
GNU General Public License v2.0
665 stars 143 forks source link

Handling of Percent Character in Field #756

Closed cpallansch closed 6 years ago

cpallansch commented 6 years ago

Need to run replacement of "%" characters to HTML entities prior to calling decodeURIComponent() or else a JS error gets thrown.

davisshaver commented 6 years ago

@cpallansch I see the change to the regex but I don't see that it's being run before decodeURIComponent(). Is that still the issue? Also any chance you could add the test case to https://github.com/wp-shortcake/shortcake/blob/master/js-tests/src/shortcodeAttributeModelSpec.js?

goldenapples commented 6 years ago

I think this PR is correct, just that the description is a bit confusing.

Replacing "%" characters with "%" has to happen before decodeURIComponent/encodeURIComponent when encoding attribute values to save into post content. That's happening correctly in Shortcode.formatShortcode() here.

The current bug, which this PR solves, was that in retrieving an attribute from a shortcode string for display in the modal UI, only the first "%" character was being decoded:

screenshot from 2017-08-11 10-34-29

I can confirm this regex change fixes the problem.

It would, however be great to use this chance to add some test coverage for this.

cpallansch commented 6 years ago

You are correct. We realized that the real root cause was not what I had in my original description of the change. It turned out that it was just needing to change the replace() call with the regex /%/g instead of using the string “%” for the first parameter. With this change it works as expected.

I was working on coming up with a unit test for this, but got side-tracked. The test scenario is a little bit different than where you were pointing me to originally as the problem manifests itself when the value is saved to the view and then returned (I believe). I started by simply taking one of your test cases and tried to use a value like “100% of the problems in 99%” which would cause an issue in the original code. However, the way the test cases were written were comparing to string literals and didn’t really exercise the round trip logic.

Anyway, if I can get something to work I will definitely pass it along.

Thank you for looking into this and fixing the use of the regex.