Strip invalid characters from attributes when outputting.

[mattheu]

This is the flip side to #496 - if you've not opted in to encoding attributes, then we should prevent things just breaking.

Currently you can get into trouble if you add certain characters to your attributs. Basically anything that breaks the shortcode parsing.

Encoding is a hack to get around this - but if encoded is disabled - we should strip the following characters "'[]. There may be more too. Line breaks perhaps? This ensures people dont end up breaking things.

[danielbachhuber]

Hm. I'm not sure how I feel about this. I understand the technical problem of sending this data back to the editor, but I feel like it violates the principle that you shouldn't ever unexpectedly delete user data.

[mattheu]

I agree - but don't like the implication that it just breaks silently either. We could strip and diplay a notification? Maybe just an alert()?

[khromov]

Why can't we encode " ' [ ] the same way we encode other characters? i.e. they would be represented by " ' [ ] ?

If we go the "strip" route, we'd only need to strip "[] - single quotes are allowed in shortcode attributes that are double-quote enclosed. (And vice versa.)

We have implemented rudimentary stripping in our Shortcake fork:
https://github.com/Aftonbladet/Shortcake/commit/36bf33dea6cb2de121516c41c7515b6c76be1fe0

We also changed the double quote syntax to single quote, because single quotes are much less likely to appear in regular text:
https://github.com/Aftonbladet/Shortcake/commit/936945f1abe7f2083ab3c8856f01d86ba265bac4

(Neither of those changes feel solid enough to make a PR for, but I'd be happy to assist in any way.)

[danielbachhuber]

Why can't we encode " ' [ ] the same way we encode other characters?

We can and do. This issue is to accommodate the scenario where encode=>false.

[khromov]

Thanks for explaining @danielbachhuber .

Why can't all fields should have encode enabled by default? For example, text and textarea have similar use cases, so why would they have different encode defaults?

Perhaps by setting encode => false, the developer accepts that certain characters will not be possible to use. This could be outlined in the docs?

[danielbachhuber]

Why can't all fields should have encode enabled by default? For example, text and textarea have similar use cases, so why would they have different encode defaults?

encode is disabled by default because it requires special handling in the shortcode callback.

[mattheu]

Why can't we encode " ' [ ] the same way we encode other characters?

Turns out we can't do this because TinyMCE will automatically unescape these.

It looks like this issue could be fixed upstream with the upcoming improvements to Shortcode handling - so we're going to punt this for now.

[khromov]

It's a shame this won't get resolved for 0.6.0. I think it will be hard to gain major adoption when it takes a single character to break an entire shortcode into an unresolvable mess.

[danielbachhuber]

I think it will be hard to gain major adoption when it takes a single character to break an entire shortcode into an unresolvable mess.

This problem needs to be solved at the shortcode parsing layer, which is outside the scope of Shortcake.

[khromov]

That's the ideal solution, but there should be a tradeoff between ideal and pragmatic. Isn't the idea with feature plugins that they should be usable on their own merit?

[danielbachhuber]

Isn't the idea with feature plugins that they should be usable on their own merit?

Shortcake is usable on its own merit. If this issue is a big concern for you, it's easily possible to extend Shortcake with your own workaround for the time being.