CryoBB - Create your own Bulletin Board

Coding Critique is the place to post source code for peer review by other members of DevNetwork. Any kind of code can be posted. Code posted does not have to be limited to PHP. All members are invited to contribute constructive criticism with the goal of improving the code. Posted code should include some background information about it and what areas you specifically would like help with.

Popular code excerpts may be moved to "Code Snippets" by the moderators.

Moderator: General Moderators

User avatar
Jonah Bron
DevNet Master
Posts: 2764
Joined: Thu Mar 15, 2007 6:28 pm
Location: Redding, California

Re: CryoBB - Create your own Bulletin Board

Post by Jonah Bron »

s.dot wrote:But, PHPBB will parse this:

Code: Select all

[b]im bold[i] and italic[/b] yeah?[/i]
It does parse it, but it doesn't format as expected. As you can see in your last post, it comes out different because they both use the <span> tag.

PHPBB allows the output of invalid XHTML. Josh thinks we shouldn't do that, and I'm inclined to agree...
User avatar
s.dot
Tranquility In Moderation
Posts: 5001
Joined: Sun Feb 06, 2005 7:18 pm
Location: Indiana

Re: CryoBB - Create your own Bulletin Board

Post by s.dot »

Jonah Bron wrote:
s.dot wrote:But, PHPBB will parse this:

Code: Select all

[b]im bold[i] and italic[/b] yeah?[/i]
It does parse it, but it doesn't format as expected. As you can see in your last post, it comes out different because they both use the <span> tag.

PHPBB allows the output of invalid XHTML. Josh thinks we shouldn't do that, and I'm inclined to agree...
Indeed, particularly the "it does parse it" part. I don't care if the output is valid or not, or if it's what is expected, I just don't think it should leave a full tag unparsed.

Maybe it's just a "me" thing. :)
When I use the library I will edit the source to fix this, so it's not a big problem on -my- end.
Set Search Time - A google chrome extension. When you search only results from the past year (or set time period) are displayed. Helps tremendously when using new technologies to avoid outdated results.
josh
DevNet Master
Posts: 4872
Joined: Wed Feb 11, 2004 3:23 pm
Location: Palm beach, Florida

Re: CryoBB - Create your own Bulletin Board

Post by josh »

I was discussing with Jonah there are three flavors of BB Code I have identified from our discussion. This is the strategy pattern.

On improper nesting we can: 1 Output improper HTML, 2 Parse as much as we can safely & ignore the rest, 3 or Attempt to actually guess what the user meant. Different people will want different behavior, so when we add new behavior it should be a setting they can change, we shouldn't stop supporting the way it works now, when we add new behavior later.

I propose we support at minimum the first two, output improper HTML, or parse as much as we can safely & ignore the rest. I put a unit test for the latter, perhaps someone could write a unit test showing the ideal inputs & outputs and check it in commented out, or post the example usage scenario here, or whatever...

The 3rd strategy, trying to guess what the user meant is futile, but if you section them off to different components (classes), people can try out different strategies.

Code: Select all

function testShouldIgnoreInvalidNesting()
{
	$base = new CryoBB_Base();
	$base->addTag($this->tagBold());
	$base->addTag($this->tagItalics());
	
	$actual = $base->make('[b]foo[i]bar[/b][/i]');
	$this->assertEqual($actual, '<strong>foo[i]bar</strong>[/i]', 'Should ignore invalid nesting');
}
PS > These forums actually try to parse bb code inside of syntax tags, thats a FAIL
User avatar
s.dot
Tranquility In Moderation
Posts: 5001
Joined: Sun Feb 06, 2005 7:18 pm
Location: Indiana

Re: CryoBB - Create your own Bulletin Board

Post by s.dot »

josh wrote:PS > These forums actually try to parse bb code inside of syntax tags, thats a FAIL
I noticed that when creating this topic. :D The BB Code generator or mods on these forums have a lot of issues. Even typing in plain text < br > without spaces makes a line break.

BTW, I don't care about the output, as long as a full tag [ i ] ... [ /i ] is not left in the output! Maybe I'm being misunderstood or I'm misunderstanding you guys. It's not complicated though. As a dev, if i used the library, I would say "why is the i tag not being parsed"

In your test above, I would expect to see

Code: Select all

<strong>foo<span style="font-style: italic;">bar</strong></span>
It ignores invalid nesting but still parses the tag.
Set Search Time - A google chrome extension. When you search only results from the past year (or set time period) are displayed. Helps tremendously when using new technologies to avoid outdated results.
User avatar
Jonah Bron
DevNet Master
Posts: 2764
Joined: Thu Mar 15, 2007 6:28 pm
Location: Redding, California

Re: CryoBB - Create your own Bulletin Board

Post by Jonah Bron »

s.dot wrote:Indeed, particularly the "it does parse it" part. I don't care if the output is valid or not, or if it's what is expected, I just don't think it should leave a full tag unparsed.

Maybe it's just a "me" thing. :)
When I use the library I will edit the source to fix this, so it's not a big problem on -my- end.
We can totally do this if you want to, especially if you're planning on doing it anyway :roll: . I just wanted to avoid (what seemed to me) unnecessary work... but Josh seems to have some good concepts for doing it. What would the class structure for that look like, Josh?
josh wrote:PS > These forums actually try to parse bb code inside of syntax tags, thats a FAIL
At first look, it seems to work fine. But after a while, you begin to see that PHPBB doesn't do that good of a job.

I was just browsing the source of their parser (on Github here), and I'm not too fond of their implementation. Kind of messy, and there are a lot of switch statements. I have no idea how it works. The function of ours seems very obvious :) . When we're stable, we might want to tell somebody there about it :D
User avatar
s.dot
Tranquility In Moderation
Posts: 5001
Joined: Sun Feb 06, 2005 7:18 pm
Location: Indiana

Re: CryoBB - Create your own Bulletin Board

Post by s.dot »

Alright, just leave the tag unparsed. I will bow out of my argument now. :)
Set Search Time - A google chrome extension. When you search only results from the past year (or set time period) are displayed. Helps tremendously when using new technologies to avoid outdated results.
User avatar
s.dot
Tranquility In Moderation
Posts: 5001
Joined: Sun Feb 06, 2005 7:18 pm
Location: Indiana

Re: CryoBB - Create your own Bulletin Board

Post by s.dot »

I'm setting up a custom forum I built to use CryoBB for the bb code parser so we (and others) can see and use this live.
Will give jonah and josh FTP logins. Will take a day or two.
Set Search Time - A google chrome extension. When you search only results from the past year (or set time period) are displayed. Helps tremendously when using new technologies to avoid outdated results.
User avatar
Jonah Bron
DevNet Master
Posts: 2764
Joined: Thu Mar 15, 2007 6:28 pm
Location: Redding, California

Re: CryoBB - Create your own Bulletin Board

Post by Jonah Bron »

s.dot wrote:Alright, just leave the tag unparsed. I will bow out of my argument now. :)
Wait, didn't we just agree with you? :lol:
User avatar
s.dot
Tranquility In Moderation
Posts: 5001
Joined: Sun Feb 06, 2005 7:18 pm
Location: Indiana

Re: CryoBB - Create your own Bulletin Board

Post by s.dot »

Jonah Bron wrote:
s.dot wrote:Alright, just leave the tag unparsed. I will bow out of my argument now. :)
Wait, didn't we just agree with you? :lol:
I don't think so. :P
I meant that I would do it on my local copy. Not that I would do it in the public version. I would never override the respected opinion of multiple developers just to be an a$$.
Set Search Time - A google chrome extension. When you search only results from the past year (or set time period) are displayed. Helps tremendously when using new technologies to avoid outdated results.
User avatar
Jonah Bron
DevNet Master
Posts: 2764
Joined: Thu Mar 15, 2007 6:28 pm
Location: Redding, California

Re: CryoBB - Create your own Bulletin Board

Post by Jonah Bron »

Well I mean, I don't think providing that option would be bad in itself, just providing that as the only option. As Josh said, there are ways to do both. Lets do it!
josh
DevNet Master
Posts: 4872
Joined: Wed Feb 11, 2004 3:23 pm
Location: Palm beach, Florida

Re: CryoBB - Create your own Bulletin Board

Post by josh »

I think we should support both methods. My opinion is that over time we'll find not many people actually need the invalid nesting support, but if you want to build it, like I was telling Jonah, there's no reason not to.. I just think that no matter what it could make mistakes, so it should be a separate "mode"

If I understand Jonah's private messages correctly, the way this thing work is by first building up an in memory graph of objects that describe what "offsets" to replace text. That happens in Base. this line get the offsets

Code: Select all

$replaceMatches	= $this->matchTags($str);
What I would imagine is a switch statement in matchTags(). Depending on the "mode", it would instantiate a different object that would provide offsets using different strategies. Some would ignore invalid nesting, others could attempt to fix it in different ways. The person using our framework (some developer of a social network or forums) may have strong opinions on how to handle the invalid nesting, so its worth trying to support a couple different ways.

I was saying to Jonah in a PM though I think either way this framework should guarantee valid HTML, with predictable results regardless of valid/invalid nesting, in at least one of it's modes. (aka how it works now). My opinion is make sure in at least one mode it remains to work how it does now.
Last edited by josh on Wed Dec 22, 2010 6:40 pm, edited 1 time in total.
User avatar
Jonah Bron
DevNet Master
Posts: 2764
Joined: Thu Mar 15, 2007 6:28 pm
Location: Redding, California

Re: CryoBB - Create your own Bulletin Board

Post by Jonah Bron »

That's a good idea, just switch the tag matching object. What do we name the two objects?
josh
DevNet Master
Posts: 4872
Joined: Wed Feb 11, 2004 3:23 pm
Location: Palm beach, Florida

Re: CryoBB - Create your own Bulletin Board

Post by josh »

How about Strict and Overlapping? The first is really strict about matches, and the other would allow "overlapping" tags (tags closed in reverse order they were opened)
User avatar
Jonah Bron
DevNet Master
Posts: 2764
Joined: Thu Mar 15, 2007 6:28 pm
Location: Redding, California

Re: CryoBB - Create your own Bulletin Board

Post by Jonah Bron »

Okay, need we define an interface?
josh
DevNet Master
Posts: 4872
Joined: Wed Feb 11, 2004 3:23 pm
Location: Palm beach, Florida

Re: CryoBB - Create your own Bulletin Board

Post by josh »

First I'd say you'd need to define the expected inputs & outputs you want to see in this mode, with enough scenarios to show how it should work in all situations.

For example

Code: Select all

[b]foo[i]bar[/b][/i]
==

Code: Select all

<b>foo<i>bar</i></b>
"Should reverse close tag" that would be a spec.

Another would be what to do if I close a tag I never opened?
Post Reply