search

jbowens / jBBCode

A lightweight but extensible BBCode parser
http://jbbcode.com
MIT License
164 stars 32 forks source link

Fix for BBCode rendering of ElementNode attributes #55

Closed DaSourcerer closed 9 years ago

DaSourcerer commented 9 years ago

As the attributes can be injected freely and they are being written out in order, the current code would allow a tag to be rendered as [tag attr=value=val] instead of the desired [tag=val attr=value].

DaSourcerer commented 9 years ago

Are there any objections to this? This is somewhat holding me back with #53 …

shmax commented 9 years ago

I don't fully understand it, but if the tests all still pass, then :+1:

jbowens commented 9 years ago

looks fine to me

Kubo2 commented 9 years ago

@DaSourcerer:

I don't understand jBBCode system completely yet, because I didn't have enough time to discover it nor completely read it, but IMHO you should have added some test to ilustrate the behavior you changed / fixed to ensure exactly this behavior works (I think it's specifically called bug test-case).

DaSourcerer commented 9 years ago

@shmax, @Kubo2: Sorry, I thought this was kind of self-explanatory. Seems I was wrong. The problem is this: The parser allows for attributes (options) to be associated directly with a tag. This is managed by storing said attribute together with all "regular" ones having the tag's name as the key. When rendering the input as BBCode again, the parser assumes this special attribute to come in first.

Now there are two ways in which this can go wrong:

  1. Somebody programmatically assigns an attribute to the tag but it isn't the first value in the attributes array.
  2. The input contains a tag with an attribute which has a name that is equal to the tag's name and doesn't come in as first attribute (e.g. [foo bar=baz foo=1]).

In both cases, there would've been some sort of hiccup when rendering BBCode again.

@Kubo2:

IMHO you should have added some test to ilustrate the behavior you changed / fixed to ensure exactly this behavior works

You are of course right. But I was running into some kind of chicken-egg problem here as tests have changed drastically in #53. Having added a test case here would have caused problems later. I hope I can commit a fitting test case later this evening ...