Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[3.2][ticket/11768] Integrate s9e\TextFormatter #3461

Merged
merged 82 commits into from Apr 5, 2015

Conversation

@Nicofuma

This comment has been minimized.

Copy link
Member

commented Mar 2, 2015

thanks

namespace phpbb\textformatter;
/**
* text_formatter.cache service

This comment has been minimized.

Copy link
@Nicofuma

Nicofuma Mar 2, 2015

Member

don't mention the service in the doc block

* Currently only used to signal that something that could effect the rendering has changed.
* BBCodes, smilies, censored words, templates, etc...
*
* @todo functionality should be moved to data_access

This comment has been minimized.

Copy link
@Nicofuma

Nicofuma Mar 2, 2015

Member

todo?

This comment has been minimized.

Copy link
@JoshyPHP

JoshyPHP Mar 2, 2015

Author Contributor

I think it's a note from a previous iteration that I forgot to remove.

*
* @todo functionality should be moved to data_access
*
* @package phpBB3

This comment has been minimized.

Copy link
@Nicofuma

Nicofuma Mar 2, 2015

Member

we don't use @Package

*
* Also used to get templates.
*
* @package phpBB3

This comment has been minimized.

Copy link
@Nicofuma

Nicofuma Mar 2, 2015

Member

we don't use @Package

namespace phpbb\textformatter;
/**
* text_formatter.data_access service

This comment has been minimized.

Copy link
@Nicofuma

Nicofuma Mar 2, 2015

Member

don't mention the service

*
* @return array
*/
public function get_words()

This comment has been minimized.

Copy link
@Nicofuma

Nicofuma Mar 2, 2015

Member

use a more precise function name

/**
* text_formatter.parser service
* @package phpBB3

This comment has been minimized.

Copy link
@Nicofuma

Nicofuma Mar 2, 2015

Member

remove service name and @Package

* text_formatter.parser service
* @package phpBB3
*/
abstract class parser

This comment has been minimized.

Copy link
@Nicofuma

Nicofuma Mar 2, 2015

Member

should be an interface

/**
* text_formatter.renderer service
* @package phpBB3

This comment has been minimized.

Copy link
@Nicofuma
/**
* Render given text
*
* @param string $text Text, as parsed by the text_formatter.parser service

This comment has been minimized.

Copy link
@Nicofuma

Nicofuma Mar 2, 2015

Member

don't mention the service but the corresponding interface

/**
* Automatically set the smilies path based on config
*
* Called by the service container

This comment has been minimized.

Copy link
@Nicofuma

Nicofuma Mar 2, 2015

Member

to be removed

This comment has been minimized.

Copy link
@JoshyPHP

JoshyPHP Mar 2, 2015

Author Contributor

I don't know which part you want to remove.

* Configure this renderer as per the user's settings
*
* Should set the locale as well as the viewcensor/viewflash/viewimg/viewsmilies options.
* Called by the service container

This comment has been minimized.

Copy link
@Nicofuma

Nicofuma Mar 2, 2015

Member

again

* text_formatter.renderer service
* @package phpBB3
*/
abstract class renderer

This comment has been minimized.

Copy link
@Nicofuma

Nicofuma Mar 2, 2015

Member

should be/have an interface

/**
* Creates s9e\TextFormatter objects
* @package phpBB3

This comment has been minimized.

Copy link
@Nicofuma
class factory implements \phpbb\textformatter\cache
{
/**
* @var phpbb_cache_driver_interface $cache

This comment has been minimized.

Copy link
@Nicofuma

Nicofuma Mar 2, 2015

Member

/phpbb/cache/driver/driver_interface

/**
* @var \phpbb\textformatter\data_access
*/
protected $dal;

This comment has been minimized.

Copy link
@Nicofuma

Nicofuma Mar 2, 2015

Member

use a name with more sense

catch (\Exception $e)
{
/**
* @todo log an error?

This comment has been minimized.

Copy link
@Nicofuma

Nicofuma Mar 2, 2015

Member

todo?

This comment has been minimized.

Copy link
@JoshyPHP

JoshyPHP Mar 3, 2015

Author Contributor

That's for you to decide. I'll happily leave this question unanswered for now.

This comment has been minimized.

Copy link
@JoshyPHP

JoshyPHP Mar 3, 2015

Author Contributor

On that note, I've just enabled the creation of unsafe BBCodes to match 3.1's behaviour but at some point in the future the ACP module should be modified to leverage the library's ability to detect unsafe BBCodes. a4bdaae

{
if (isset($this->custom_tokens[$bbcode_name]))
{
$template = strtr($template, $this->custom_tokens[$bbcode_name]);

This comment has been minimized.

Copy link
@Nicofuma

Nicofuma Mar 2, 2015

Member

I'm not sure about the references

* @param string $template Style template (bbcode.html)
* @return array Associative array matching BBCode names to their template
*/
protected function extract_templates($template)

This comment has been minimized.

Copy link
@Nicofuma

Nicofuma Mar 2, 2015

Member

you need to extract all the bbcode templates for all the styles?

This comment has been minimized.

Copy link
@JoshyPHP

JoshyPHP Mar 3, 2015

Author Contributor

Yes.

/**
* s9e\TextFormatter\Parser adapter
* @package phpBB3

This comment has been minimized.

Copy link
@Nicofuma
class parser extends \phpbb\textformatter\parser
{
/**
* @var s9e\TextFormatter\Parser

This comment has been minimized.

Copy link
@Nicofuma

Nicofuma Mar 2, 2015

Member

\s9e...

protected $parser;
/**
* @var phpbb\user User object, used for translating errors

This comment has been minimized.

Copy link
@Nicofuma

Nicofuma Mar 2, 2015

Member

\phpbb\user

/**
* Constructor
*
* @param phpbb\cache\driver_interface $cache

This comment has been minimized.

Copy link
@Nicofuma

Nicofuma Mar 2, 2015

Member

same here, when you use the full namespace you have to include the first \

$parser = $cache->get($key);
if (!$parser)
{
extract($factory->regenerate());

This comment has been minimized.

Copy link
@Nicofuma

Nicofuma Mar 2, 2015

Member

extract?

This comment has been minimized.

Copy link
@JoshyPHP

JoshyPHP Mar 3, 2015

Author Contributor

Yes. Do you want to replace it with explicit var declarations?

/**
* s9e\TextFormatter\Renderer adapter
* @package phpBB3

This comment has been minimized.

Copy link
@Nicofuma

Nicofuma Mar 2, 2015

Member

@Package, class namespace

*/
public function __construct(\phpbb\cache\driver\driver_interface $cache, $cache_dir, $key, factory $factory)
{
$renderer_data = $cache->get($key);

This comment has been minimized.

Copy link
@Nicofuma

Nicofuma Mar 2, 2015

Member

what do you store in $renderer_data? (what kind/format of data and where they are generated, where are they used. Maybe the caching can be optimized here but I need more information to know)

This comment has been minimized.

Copy link
@JoshyPHP

JoshyPHP Mar 3, 2015

Author Contributor

A serialized instance of a class that extends s9e\TextFormatter\Renderer and the name of the class. You need the name of the class to load its PHP file before unserializing it.

This comment has been minimized.

Copy link
@Nicofuma

Nicofuma Mar 3, 2015

Member

in this case I think it's better to directly dump a php class (have a look at https://github.com/symfony/DependencyInjection/blob/master/Dumper/PhpDumper.php https://github.com/symfony/symfony/blob/2.7/src/Symfony/Component/Routing/Matcher/Dumper/PhpMatcherDumper.php and https://github.com/symfony/symfony/blob/2.7/src/Symfony/Component/Routing/Generator/Dumper/PhpGeneratorDumper.php)

Also you could save you some work by using \Symfony\Component\Config\ConfigCache (you can see an example in \phpbb\di\container_builder and \phpbb\routing\router)

This comment has been minimized.

Copy link
@Nicofuma

Nicofuma Mar 3, 2015

Member

I'm right when I assume that the data change only when you invalidate the cache, isn't it?

This comment has been minimized.

Copy link
@JoshyPHP

JoshyPHP Mar 3, 2015

Author Contributor

I've thought about it and realized we didn't need to cache an instance of the renderer anymore, just the name of the class. 736c9ee

This comment has been minimized.

Copy link
@Nicofuma

Nicofuma Mar 3, 2015

Member

actually if the data doesn't change it's much more faster to dump it as a correct php code (especially if you have Opcache or something similar installed)

This comment has been minimized.

Copy link
@JoshyPHP

JoshyPHP Mar 3, 2015

Author Contributor

I don't know what you mean. The library already dumps the compiled templates in a class, with all of the data it needs for rendering. That's what you're describing, correct? The file is in cache/<environment>/s9e_renderer_<hash>.php and it looks like this.

This comment has been minimized.

Copy link
@Nicofuma

Nicofuma Mar 3, 2015

Member

you removed the serialize thing. tight

/**
* Text manipulation utilities
* @package phpBB3

This comment has been minimized.

Copy link
@Nicofuma
*
* Used to manipulate a parsed text
*
* @package phpBB3

This comment has been minimized.

Copy link
@Nicofuma
JoshyPHP added 22 commits Mar 21, 2015
[ticket/11768] Updated the text_formatter.s9e.utils service
Made it use s9e\TextFormatter\Utils. Refactored some tests to make
them more readable.

PHPBB3-11768
[ticket/11768] Removed get_parser() / get_renderer() accessors
There's no need to access the s9e\TextFormatter objects outside of events.

PHPBB3-11768
[ticket/11768] Added renderer events
Added core.text_formatter_s9e_render_before and
core.text_formatter_s9e_render_after

PHPBB3-11768
[ticket/11768] Added parser events
Added core.text_formatter_s9e_parse_before and
core.text_formatter_s9e_parse_after

PHPBB3-11768
[ticket/11768] Updated utils service
Updated docblocks.
Removed remove_formatting() because it overlaps with clean_formatting()

PHPBB3-11768
[ticket/11768] Moved parser/renderer setup events
Moved down the setup events to make them happen after the service is
configured and ready to be used

PHPBB3-11768
[ticket/11768] Renamed service vars
The name of the variable that holds the service instance is now consistent
across events.

PHPBB3-11768
[ticket/11768] Moved the routine that replaces tabs with spaces
...to its own method. Also added a quick stripos() check for performance.

PHPBB3-11768

@JoshyPHP JoshyPHP force-pushed the JoshyPHP:ticket/11768 branch from 4f5c3a0 to 4e80565 Apr 2, 2015

@Elsensee

This comment has been minimized.

Copy link
Member

commented Apr 5, 2015

Is it quickly possible to convert posts using the old bbcode parser to use your new stuff 😉 (for example just before displaying them) or would you prefer to do that in another PR?

@Nicofuma

This comment has been minimized.

Copy link
Member

commented Apr 5, 2015

I'm going to merge this PR but I want to write down what we still have to do:

  • Add a cron job to silently reparse all the posts
  • Design an API/Interface for the whole text formatting thingy (I mean: replace generate_text_for_*, includes/bbcode.php and includes/message_parser.php)
  • At least make includes/message_parser.php autoloadable
  • We don't want to fix message_parser to be PHP7 compatible, so we need to reparse the posts on the fly (if needed) when we try to display them.

@Nicofuma Nicofuma merged commit 4e80565 into phpbb:master Apr 5, 2015

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
Nicofuma added a commit that referenced this pull request Apr 5, 2015
Merge pull request #3461 from s9e/ticket/11768
[3.2][ticket/11768] Integrate s9e\TextFormatter
@brunoais

This comment has been minimized.

Copy link
Contributor

commented Apr 6, 2015

@Nicofuma
I think that TextFormater on phpBB should exist somewhat the same way that the search backend exists. There should be a base interface of communication for basics setup and then each one would do its job based on that.
What do you think of that? Does it make sense?

@Nicofuma

This comment has been minimized.

Copy link
Member

commented Apr 6, 2015

It make sense yes but it's a little more complicated because we need to always be able to reparse a post with another formatter (and so to clean the parsed post) and we also need generic events for the most common goal of the extensions.

@brunoais

This comment has been minimized.

Copy link
Contributor

commented Apr 6, 2015

The same way there's an "installer" for each FULLTEXT search, one can be made for TextFormatter.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants
You can’t perform that action at this time.