[RFC] Get rid of all bypass of generate_text_for_* family

Note: We are moving the topics of this forum and it will be deleted at some point

Publish your own request for comments/change or patches for the next version of phpBB. Discuss the contributions and proposals of others. Upcoming releases are 3.2/Rhea and 3.3.
Post Reply
User avatar
brunoais
Registered User
Posts: 958
Joined: Fri Dec 18, 2009 3:55 pm

[RFC] Get rid of all bypass of generate_text_for_* family

Post by brunoais » Tue Jul 09, 2013 5:44 pm

In some places in the code, there's code that does just about the same as the generate_text_for_* family of functions.
For example:
includes/message_parser.php:+-1217
includes/functions_posting.php:+-1110
includes/functions_privmsgs.php:+-2024
includes/mcp/mcp_queue.php:+-141
amoung some others

The only difference here is that calling in those ways, it does not trigger the related events (core.modify_text_for_display_before, core.modify_text_for_storage_after, core.modify_text_for_edit_after, etc...) but if they start using that group of functions, they would, instead.

Why this RFC:
1st It's much, much easier and logical to place textFormatter in the those function group generate_text_for_* than placing in the bbcode class because, as far as I know, it's textFormatter that will do the text censoring and BBcode parsing.
2nd It makes to me a whole lot more sense to use those three generate_text_for_* instead of using the mix of all those other functions. If it wasn't for the event I wouldn't ask for it here.

So, there are 3 things that can be done.
1. Leave as is
2. Apply this RFC and add a parameter into the function such that the caller can choose if the events should be triggered or not (2nd preferred option)
3. Apply this RFC -> Replace all those manual text parsing with the call to the respective function.

User avatar
EXreaction
Registered User
Posts: 1555
Joined: Sat Sep 10, 2005 2:15 am

Re: [RFC] Get rid of all bypass of generate_text_for_* family

Post by EXreaction » Tue Jul 09, 2013 8:00 pm

All the bbcode parsing should really be done through the generate_text_for functions.

I would make a new ticket for each location that doesn't use them and create one PR for each location. If you want to do this I'll gladly review and merge them. :)

User avatar
brunoais
Registered User
Posts: 958
Joined: Fri Dec 18, 2009 3:55 pm

Re: [RFC] Get rid of all bypass of generate_text_for_* family

Post by brunoais » Tue Jul 09, 2013 9:43 pm

Great.
Sure, I can do it.
It was part of the plans I made :).

I'll create the generic ticket and I'll create a sub-task for each place I find that kind of thing.
Then I can call you @IRC so that we can review the most difficult cases.

Should I create a different PR for each case or a single PR will do?

User avatar
EXreaction
Registered User
Posts: 1555
Joined: Sat Sep 10, 2005 2:15 am

Re: [RFC] Get rid of all bypass of generate_text_for_* family

Post by EXreaction » Wed Jul 10, 2013 1:38 am

Please do a different PR for each location, it'll make the process easier and faster for us.

Thanks!

User avatar
JoshyPHP
Registered User
Posts: 348
Joined: Fri Jul 08, 2011 9:43 pm

Re: [RFC] Get rid of all bypass of generate_text_for_* family

Post by JoshyPHP » Tue Feb 10, 2015 3:51 am

Checking up on that one. The tracker entry is at https://tracker.phpbb.com/browse/PHPBB3-11635

From what I can tell, all of the rendering is now done through generate_text_for_display(), correct? I sifted through the sources and couldn't find any code that used the BBCode class directly. While doing so I noticed that bbcode instances were still created and left unused. I sent a PR that removes them: https://github.com/phpbb/phpbb/pull/3387

A number of files still use decode_message() instead of generate_text_for_edit(), and parse_message instead of generate_text_for_storage(). As far as my own PR is concerned it doesn't matter since it modifies decode_message()/parse_message and not the generate_text_for_* functions.

User avatar
brunoais
Registered User
Posts: 958
Joined: Fri Dec 18, 2009 3:55 pm

Re: [RFC] Get rid of all bypass of generate_text_for_* family

Post by brunoais » Tue Feb 10, 2015 7:58 am

I actually paused these solutions because I was told to, while a new BBCode parser was being thought.

User avatar
JoshyPHP
Registered User
Posts: 348
Joined: Fri Jul 08, 2011 9:43 pm

Re: [RFC] Get rid of all bypass of generate_text_for_* family

Post by JoshyPHP » Tue Feb 10, 2015 8:54 am

Are you aware of any remaining locations that should use generate_text_for_display() but don't? I looked for it and I didn't find any so I assume there are none left.

User avatar
brunoais
Registered User
Posts: 958
Joined: Fri Dec 18, 2009 3:55 pm

Re: [RFC] Get rid of all bypass of generate_text_for_* family

Post by brunoais » Tue Feb 10, 2015 10:08 am

I checked... Some places I had identified were gone, other placed went to use the message parser directly or there's really no actual bypass towards that function due to how they work.
I wonder if I should just cancel all the PR that haven't been worked on yet...

Post Reply