Add hook functionality

[grimmdude]

Adds hook functionality that allows you to extend Parsedown without having to extend the core class. To use, write a class that declares methods with the same name as the methods which you would like to modify the output of. Then register the class with Parsedown.

class HookExample
{
    public function inlineUrl($url) {
        $url['element']['attributes']['rel'] = 'nofollow';
        return $url;
    }
}

$Parsedown = new Parsedown;
$Parsedown->registerHook('HookExample');
echo $Parsedown->text('http://google.com');

[PhrozenByte]

What's the reason for using static classes? One could use multiple Parsedown objects at once, using static classes for hooks allows the hook to be used by just a single Parsedown object.

[grimmdude]

I believe this should work fine with multiple Parsedown objects. By registering the class first with Parsedown::registerHook() it's essentially a global hook.

-Garrett

[PhrozenByte]

Other way round: You can't register a hook for just a single Parsedown object. Avoid static classes wherever possible, they are inflexible... Using static classes has just disadvantages here.

[grimmdude]

I see what you're saying. So in that case it's actually a matter of just using an instance variable instead of a static one to store the hooks. I've pushed this change.

-Garrett

[PhrozenByte]

... and still it's impossible to store information specific to a single Parsedown instance within a hook... 😑

[grimmdude]

I'm not sure what you mean, you can have different hooks for each Parsedown instance if desired. The hooks aren't meant to store information specific to an instance; they're just detached code that can change the output of specified methods.

-Garrett

[PhrozenByte]

The hooks aren't meant to store information specific to an instance

Simply: Why? 😩

As a final attempt: This has just disadvantages. Some features are impossible to implement without storing temporary data which are specific to a single Parsedown instance, e.g. everything that references data on the bottom of the file, like footnotes or links. Adding a hook feature is about adding flexibility, don't limit the flexibility by doubtful design decisions like static classes. You don't need static classes here, don't use them. Please look beyond the horizon... 😉

[grimmdude]

I'm trying to get on the same page as you, forgive me if it's taking a few tries to understand your points. I've pushed a change to make the hook methods non-static, and also pass the instance of Parsedown into the hook so it's available there. However most of the data in the Parsedown instance will be unavailable to the hook since it's visibility is mostly set to protected. I'm not sure if changing that is an option or not.

[erusev]

Thanks, this looks interesting, but before I merge sth like this I'd like to explore a few other options. I'd like to see if we can think of an approach that would not only allow for modifying currently supported types (ex: link, image, etc.) but also allow for adding new types.

[grimmdude]

I've gone ahead and made the change you suggested to return the hook instance from Parsedown::registerHook(). I also added a few public methods to add items from those arrays you mentioned as well as hooking into Parsedown::isBlockContinuable() and Parsedown::isBlockCompletable().

Should this be enough to make this library fully extendable?

[PhrozenByte]

It's not that easy...

Neither Parsedown::addBlockType() nor Parsedown::addInlineType() work - both Parsedown::$BlockTypes and Parsedown::$InlineTypes require an array key to specify the marker to search for. Furthermore: What do you think happens when a new block (e.g. Parsedown::$BlockTypes['$'] = 'Dollar') is added? Parsedown will try to call Parsedown::blockDollar() - a method that doesn't exist. Additionally, you forgot to update Parsedown::$inlineMarkerList.

I'd like to offer my help... Do you want to try it again yourself or should I open a new PR based on your work?

[grimmdude]

OK, figured as much. If you want to open a new PR based on this one I would appreciate it. You seem to be very familiar with the inner workings.

-Garrett

[PhrozenByte]

@erusev: Do you prefer rather a dead-simple but comparatively costly approach or a more complex, but more performant solution?

The dead-simple solution relies on always iterating through all registered hooks and searching for appropriate methods. This means, Parsedown doesn't know which hook registered a specific marker and needs to check all hooks for e.g. a blockDollar() method to call the hook. With a small number of plugins this shouldn't be a problem, but with an increasing number of plugins, the additional computation time explodes. However, this may be mitigated by caching. It's basically @grimmdude's solution extended to support new markers.

The more complex solution introduces a new ParsedownPluginInterface interface, what allows hooks to return a list of markers they want to hook into. This requires to change the structure of Parsedown::$BlockTypes and thus breaks BC (i.e. Parsedown 2.0).

[erusev]

@PhrozenByte i'll need some time to think it over

[CriseX]

Pinging this to subscribe for updates, really would prefer something like this over to trait'ing and extending the core classes. It gets quite cumbersome when you have to make sure everything remains in sync because Parsedown extra is a separate class rather than an injected dependency/an extension.

[aidantwoods]

Not via hooks, but you should be able to achieve the same flexibility with the changes made in 2.0.x branch by, for example, substituting the Url inline for your own and deferring to Url for parsing, but writing your own renderer.

[aidantwoods]

It's not released yet (so API might change a little), but I've rewritten ParsedownExtra over in erusev/parsedown-extra#172, which should give a fairly good demo for how I see this working in future. Some additional info over in: #708 (comment).