[RFC] Integrate s9e\TextFormatter

[Jacob]

We already give error messages for things like images being too large and stick in Preview mode if somebody else has posted and the Enable post review forum option is on. This would be no different.

It would be different. (By the way, I don't know that phpBB gives an error message for posting too large images, maybe you're refering to the images in the signature?) And it would be different because almost nobody is going to know the rules for posting proper BBCodes, the posibilities are limitless. "Was the center outside the quote that did this, or was the list between the whatevers?"

And remember, I'm not trying to jam it down your throat; I'm asking for the option to allow blocking malformed BBCode posts. Putting the admin in control of his own forum is a good thing.

Most admins don't know and don't care about "illegal" or "malformed" BBCode posts, and giving them the option of making the life of the forum users a hell is not a good thing. Nobody wants to see an error saying "hey, this is wrong, you should do it properly". As JoshyPHP said, if you know that this is wrong do it right for me and shut up.

Please, don't complicate things.

[JoshyPHP]

I've just committed a big overhaul to the way s9e\TextFormatter automatically generates rules, which makes it possible to individually toggle the "rules generators" and even extend them. Those rules generators cover functionalities that are usually hardcoded within BBCode parsers, such as optional end tags or not parsing BBCodes within code tags. Some generators are also meant to improve/ensure HTML compliance.

Most importantly, they exist to lift the burden of having to configure the many interactions between tags (or here, BBCodes) from the user. phpBB comes with 14 default BBCodes. Just defining whether a BBCode can be used inside of another (or itself) would be 14 * 14 = 196 settings.

Below is a list of the default generators and how they relate to a forum's BBCodes. For a more complete definition for these rules generators, see RulesGenerators.md. You can also take a look at the testdox output and search for "RulesGenerators". You'll find a few examples that are relatively human-readable, albeit technical and specific to the jargon used in the library.

• AutoCloseIfVoid - Makes it possible to use [img=foo.png]. Or rather, if a user uses this syntax, it will not break the rest of the post.
• AutoReopenFormattingElements - Fixes <b><i></b></i> in a way that is consistent with HTML5 rules.
• EnforceContentModels - Prevents using * outside of a list, quote inside of b or flash inside of url.
• EnforceOptionalEndTags - Automatically closes *.
• IgnoreTagsInCode - Disables all formatting within code tags.
• IgnoreTextIfDisallowed - Prevents text from being output between <ol> and the first <li>. This is mainly for compliance.
• IgnoreWhitespaceAroundBlockElements - Prevents excessive <br/>s around quotes and list items.
• NoBrIfWhitespaceIsPreserved - Does not convert new lines to <br/> inside of <pre>.

move all of our bbcodes to custom ones

I have removed all manual rules from the default BBCodes, which means that right now you could move them to the database with no other change. You could just copy/paste the default definitions to the custom BBCodes form and they would work the same.

[JoshyPHP]

Finally, I have a couple of outstanding matters that I need to address before declaring this PR ready. The first of which is on the first page, it's about whitespace within code tags. s9e\TextFormatter preserves it, and does not really have a mechanism to modify it. The legacy routines do that:

Code: Select all

$code = str_replace("\t", '   ', $code);
$code = str_replace('  ', '  ', $code);
$code = str_replace('  ', '  ', $code);
$code = str_replace("\n ", "\n ", $code);

It replaces tabs with 3 spaces, and replaces consecutives spaces with non-breakable spaces. This is not a behaviour that I can implement in s9e\TextFormatter, but I will modify phpBB's renderer to post-process any text within <code> elements with the same legacy routines unless instructed otherwise. It's a hack, but it's consistent with legacy. Long-term, I'd recommend preserving whitespace with <pre><code> and slapping a JavaScript highlighter on top of it. That what cool sites use.

The other item is lone tags. In s9e\TextFormatter, if you open a tag you get a tag. The BBCode parser does not look ahead to see if that tag might get closed someday. If it's never explicitely closed (or implicitely closed like *, or via an autoClose rule), it gets automatically closed at the end of the text. It's possible to force the BBCodes parser to look for a potential end tag and only accept a start tag if it has an end tag in sight, but that needs to be set on a per-BBCode basis. I'm fine with the current behaviour but I'm open to suggestions.

[Pony99CA]

We already give error messages for things like images being too large and stick in Preview mode if somebody else has posted and the Enable post review forum option is on. This would be no different.

It would be different. (By the way, I don't know that phpBB gives an error message for posting too large images, maybe you're refering to the images in the signature?) And it would be different because almost nobody is going to know the rules for posting proper BBCodes, the posibilities are limitless. "Was the center outside the quote that did this, or was the list between the whatevers?"

First, I believe if the admin sets pixel limits on images and somebody tries to post images that are larger, they will get an error.

Second, the fact that "nobody" knows the BBCode rules is probably a good reason to tell them when their post won't look like they intended.

And remember, I'm not trying to jam it down your throat; I'm asking for the option to allow blocking malformed BBCode posts. Putting the admin in control of his own forum is a good thing.

Most admins don't know and don't care about "illegal" or "malformed" BBCode posts, and giving them the option of making the life of the forum users a hell is not a good thing. Nobody wants to see an error saying "hey, this is wrong, you should do it properly".

Clearly I want to see it.

I also doubt that preventing malformed posts will make most users' lives "hell". Most users probably will never see the message; only those that try to edit BBCodes or nest them are likely to see that.

As JoshyPHP said, if you know that this is wrong do it right for me and shut up.

Please, don't complicate things.

Please don't be reductive. As I stated earlier, the software likely won't do it "right". It might be able to fix things so that there aren't any errors, but that may not be what the user intended (as I plainly spelled out previously, it will likely be wrong more often than right).

I'm not saying that the software shouldn't try to correct the error; I'm saying that instead of correcting it and posting, the user should see a warning saying that there was malformed BBCode (which the system tried to fix) and preview the post so that user can see if it was formatted as intended. If the user is happy with it, he can submit the post; otherwise he'll have to edit it.

The other item is lone tags. In s9e\TextFormatter, if you open a tag you get a tag. The BBCode parser does not look ahead to see if that tag might get closed someday. If it's never explicitely closed (or implicitely closed like *, or via an autoClose rule), it gets automatically closed at the end of the text. It's possible to force the BBCodes parser to look for a potential end tag and only accept a start tag if it has an end tag in sight, but that needs to be set on a per-BBCode basis. I'm fine with the current behaviour but I'm open to suggestions.

This is exactly what I'm talking about. I believe that phpBB currently does auto-close unclosed tags at the end of the post, but that's often the completely wrong behavior. I would continue to do that, but give a warning and go to Preview as I pointed out above. This gives the user the chance to fix his post before submitting. (Or at least give the option to turn on the warning.)

Maybe once this is done, if you implement the warning, we could try it here on Area 51 and see if it really was "hell" or not.

Steve

[JoshyPHP]

I believe if the admin sets pixel limits on images and somebody tries to post images that are larger, they will get an error.

I can confirm that's how it works. By default there are no limits, but if you set a limit on width or height it will download the image and check its size and potentially output an error. Personally, I'd rather see those routines (which probably predate the democratization of max-width in browsers) replaced by some CSS.

I believe that phpBB currently does auto-close unclosed tags at the end of the post

It uses regexps, so tags are usually matched in pairs.

Maybe once this is done, if you implement the warning, we could try it here on Area 51 and see if it really was "hell" or not.

About the warnings, I will not implement that as part of this RFC/PR because it's outside of its scope but I encourage you to create a new RFC about it so people can discuss it.

[EXreaction]

As far as code tags, it would be far preferable to use <pre> to preserve whitespace over the hacks we used before.

I don't think I'll mind the auto-closing tags.

For 3.2 I think there is a pretty good chance we'll have a WYSIWYG editor included, which should take care of most issues users have posting bbcodes.

[Pony99CA]

Maybe once this is done, if you implement the warning, we could try it here on Area 51 and see if it really was "hell" or not.

About the warnings, I will not implement that as part of this RFC/PR because it's outside of its scope but I encourage you to create a new RFC about it so people can discuss it.

The warnings would have to be caught by the text formatter and returned to the calling code for display, right? Will your text formatter do that? If so, great; if not, the requirement has to be part of this RFC (unless the requirement is rejected, of course).

Steve

[JoshyPHP]

s9e\TextFormatter logs a warning when a tag is not allowed in context (e.g. quote inside of b.) If you wanted phpBB to display those as errors, you'd need to modify phpbb_textformatter_s9e_parser::get_errors() and add something like

Code: Select all

elseif ($msg === 'Tag is not allowed in this context' && $context['tag']->getLen())
{
    $errors[] = $this->user->lang('CANNOT_YADDA_YADDA_BBCODE');
} 

[JoshyPHP]

After 4 weeks of development, producing 99 commits totalling 5548 line changes over 61 files and including 157 unit tests, I hereby declare my work on this PR done.

Future PRs should include migrating old posts to the new format, removing the legacy routines, and finally make use of s9e\TextFormatter's very awesome plugins. But for the moment, I'm taking a break and waiting patiently for 3.2 to get branched.

In the meantime I'll still be participating to topics such as this one, looking for feedback and ideas so please keep them coming.

[EXreaction]

Nice work. As soon as I get some time I'll review this PR.