[RFC] Integrate s9e\TextFormatter

[nickvergessen]

Well adding a url field to the author on quotes is only half the feature we had before.
Previously you could also color it (e.g. with the administrator color on admin posts)

sample

With all these BC breaks, I personally would not like to have it changed. However I can also see the PR growing infinite, with all the requests and features we have and want to have, so I'm really split here.

[brunoais]

Although that is possible... I really believe it is really too rarely used to make sense to give support to it.
Also, was that in the use-cases of it or was that just something that happened of how the BBCode parser works? I'd bet on the latter based on the comments and how the code is written.
(not exactly, but part of it: https://github.com/phpbb/phpbb/blob/dev ... er.php#L88 ).
Also, there was no information (wen the parser was made) that that use-case had been taken into account.

Assuming the above is correct, making that not happening anymore isn't changing the use-cases of the tag. The new use-cases that later had appeared due to the way it parses become moot given that the major use-cases for it (a link to the original) becomes available.

[JoshyPHP]

Well adding a url field to the author on quotes is only half the feature we had before.

If the argument is 1 BBCode = 1 feature then it's only 16.67% of what you had before (b, i, u, color, url and email are all allowed in the quote's author field.) And it's even less when you consider that in the current system you can use multiple URLs and multiple colors and multiple underlines (etc...) in the quote's author.

Realistically, apart from the url BBCode I don't think those features will be missed by many people. Or by anybody really. For those who do miss them, it will be possible to work something out as an extension as soon as we create events to give them access to the library. And even without an extension, if someone really wants rich quote authors they can create a custom BBCode that's used for the quote's author line.

I'm going to add the URL-enhanced quote BBCode tomorrow. It will automatically extract the first URL it finds in the quote's author. That means that the following will be functionally equivalent and they won't require a migration.

Code: Select all

[quote="[url=http://example.org]author[/url]"]...[/quote]
[quote="author" url="http://example.org"]...[/quote]

Censoring domains or URLs is a very common practice so we need a solution, either a new feature to blacklist URLs or a modification of how the censors work.

I have spent time on this and I've come to the conclusion that there is no simple, clean way to have a censor that is:

• fully dynamic
• HTML-safe
• works on URLs

You can get two of them without too much work, but to get all three requires either a lot of work or it will be a hack.

You can get a censor that is fully dynamic on the post's text, HTML-safe and blocks URLs at parsing time. The URL blocking would not be retroactive.

Or you can just run censor_text() on the HTML. It's not HTML-safe but that's closer to what 3.1 does.

There are many possibilities but all of them involve a trade-off. To keep things in perspective, regardless of how thorough the link censor is, all it takes to bypass it is a URL shortener.

Edit: after some more brainstorming, I think I can use a modified version of the HTML-safe censor I use in the library to censor text nodes and the content of HTML attributes. That might be the closest thing to how 3.1 behaves. I'll look into it ~tomorrow.

[JoshyPHP]

Status update
Links point to the relevant commit, commits usually contain regression tests.

Added limited support for url in quote author.
The following forms are supported:

Code: Select all

[quote="[url=http://example.org]author[/url]"]...[/quote]
[quote="[url]http://example.org[/url]"]...[/quote]
[quote="http://example.org"]...[/quote]
[quote="author" url="http://example.org"]...[/quote]

Allowed text in places where text is not valid HTML.
That's consistent with 3.1.

Replaced the Censor plugin with something that is run at rendering time.
The behaviour is close to 3.1's, with lower overhead. The censor applies to plain text and attribute values. Does not apply to tag names or attribute names.

Outstanding issues

1. Following up:

   • Default limits are set on the number of occurence of individual BBCodes in text (1000 of each), global number of BBCodes in text (10000) and nesting level (10.) Same thing for smilies, the default is 1000 smilies per post. Past this limit, the markup is ignored.

   Why?

   If you want to change those limits, I need a number between 1 and PHP_INT_MAX.
2. I need to create at least two events that trigger before and after the library is configured. That would allow extensions to modify almost any aspect of text formatting. I'm not sure how you'll want them named. I guess core.text_formatter_s9e_configure_before and core.text_formatter_s9e_configure_after?

Next

I would like to finish that as soon as possible, but I also see that there is still an open discussion with no agreement nor consensus about the storage, and I think that this point is crucial and need to be decided.

With the storage question seemingly settled, what's the next step?

[brunoais]

[*]I need to create at least two events that trigger before and after the library is configured. That would allow extensions to modify almost any aspect of text formatting. I'm not sure how you'll want them named. I guess core.text_formatter_s9e_configure_before and core.text_formatter_s9e_configure_after?

My opinion is (with what I know):
core.bbcode_parsing_before
core.bbcode_parsing_after

Depending on the code structure, I may find better names. Regardless, I would never use the names you suggested .

[JoshyPHP]

• The service is called text_formatter, it's not specific to BBCodes
• The specific implementation is s9e (other implementations are theorically possible)
• The event is triggered when the library is configured, not during parsing

Extensions that want to access the library before/after parsing/rendering can do it through the service, via the core.modify_text_for_* events. For example:

Code: Select all

$parser_service = $phpbb_container->get('text_formatter.parser');
if ($parser_service instanceof \phpbb\textformatter\s9e\parser)
{
    // Returns an instance of s9e\TextFormatter\Parser
    $parser_service->get_parser();
}

[brunoais]

Ah, so the event is meant to be specific for your BBCode parser, is that it?

[JoshyPHP]

The event is for accessing the s9e\TextFormatter library before/after it's configured with phpBB's default configuration. It will let extensions modify BBCodes, smilies, the word censor, etc...

[brunoais]

I think there should be something before that, then. Something that allows to alter by the phpBB level and not already inside textFormatter itself.
The event should allow altering all those parameters but it should use a general purpose interface that should be equal to every BBCode parser that phpBB may have (from the current plans)

[JoshyPHP]

I'm not sure what you're suggesting exactly. This event gives access to an instance of s9e\TextFormatter\Configurator in order to use features that are specific to this library. That's why it cannot be a generic event.

Also, it is important to not confuse BBCode parsers with this set of services. This PR is not limited to BBCodes. Smilies are not BBCodes, neither is the word censor. In phpBB, magic links are not BBCodes either. That's why the services are called text_formatter.* and not something specific such as bbcodes.*