Replace ShortcodeParser with external library

[Firesphere]

Follow-up from #5487

Replace the current ShortcodeParser library with an external, maintained package. Up for the job so far, seems to be thunderer/Shortcode.

[Firesphere]

quote @sminnee

And in _config.php:

ShortcodeParser::get('default')->register('embed', array('EmbedShortcodeProvider', 'handleShortcode'))

We can decide a better approach once we see what other shortcode engines expect.

I would actually like to have the shortcodes registered via the yml file eventually. That would save a lot I think. Any pros/cons?

[sminnee]

I would actually like to have the shortcodes registered via the yml file eventually. That would save a lot I think. Any pros/cons?

It makes sense, but I'd wait until we have found the right shortcode library to see what it expects.

[sminnee]

thunderer/Shortcode looks like the only viable option, although it is a 1-person project. It's MIT licensed, which is compatible.

[sminnee]

From @hafriedlander: "However most of the code in SilverStripe's ShortcodeParser is actually to try and adjust the inserted content to still be HTML compliant, not actually parsing. (So if the shortcode tries to insert a p tag into an existing p tag it still works). It doesn't look like thunderer/shortcode attempts that - like most shortcode systems, it's content-type agnostic / ignorant.

(The initial use case, for those interested, was so a shortcode could insert a <figure><img /><figcaption /></figure> block which gets floated left or right - these need to be pulled out of any surrounding p tag or most browsers do the wrong thing)."

This situation is covered in ShortcodeParserTest::testExtract(). The behaviour is hard-coded to interrogate a class argument of any shortcode. It doesn't appear to be used by any core code, but I expect it's been used by modules. It would be an unnecessary regression to drop this functionality.

[Firesphere]

From the looks of it, most frameworks actually implement a parser themselves, indeed thunderer seems the only viable option. Considering how much it's used, there are surprisingly few libraries available.

[sminnee]

Having looked into this more, I have mixed feelings.

• The thunderer module is of modest popularity and only has a single maintainer.
• It's certainly got more features than our shortcode handler, but it lacks a key feature that ours has.
• Our shortcode handler isn't a massive source of open issues - it looks like there are 6 or 8 open features

I wonder if it's actually better just to leave the shortcode handler as-is for now? In providing an HTML-aware shortcode handler, the current system does provide a unique service.

[Firesphere]

Yep, I've not yet found a good alternative either. Although looking at the BBCode parser, I'm getting slightly twitchy.

[sminnee]

As an aside, these seem to be the open bugs in the shortcode parser:

• shortcode parser doesnt run on https connections #4647
• Shortcode parsed twice by the template #4244
• HTMLText un-escaped when shortcode parser is used #3925
• ShortCodeParser: A parameter value of 0 is ignored by the parser and fails. #2382
• ampersands in asset filenames break the shortcode parser.  silverstripe-cms#906

And these seem to be bugs to do with how the shortcode system is being used (which pulling in a new handler wouldn't fix)

• embed Shortcodes break Text::Summary() method. #2929
• Shortcodes not replaced in FulltextSearch results #1751
• [2010-05-04] Consistently use ShortcodeParser in Text/HTMLText value getters (breaking [sitetreelink_id] in SiteTree.Content) #1381

I believe that this is a bug in a particular shortcode handler:

• [2012-06-08] "Insert media" Show [embed] shortcode when no oEmbed thumbnail is available silverstripe-cms#461

[sminnee]

Maybe our efforts are actually better spend fixing those bugs? ;-)

[Firesphere]

Possibly/probably :)
But the BBCodeParser is still something. It seems very unmaintained and old. Also, I can't find any usages of it?

[sminnee]

Based on the previous discussion, do you think we should close this one, Simon?

[dhensby]

Can't we use https://github.com/thunderer/Shortcode and then create a custom "HTML aware" processor?

[sminnee]

The issue is that we'd wind up rewriting the internals of Thunderer so fundamentally that I'm not sure they'd accept a PR.

As an alternative, it might be worth spinning out our shortcode system as a separate micropackage, so as to decouple it.

[dhensby]

Looking at https://github.com/thunderer/Shortcode#processing - all the context is provided so why can't we write a Parser that will apply all the context related HTML compliance that we have in https://github.com/silverstripe/silverstripe-framework/blob/master/parsers/ShortcodeParser.php ?

I am taking a very quick look at the docs, of course, without digging so I may have missed someting - but it looks like we won't get any less context than we currently do and we can write/extend a parser to behave as we need.

Sorry if I've missed something obvious or I'm assuming a leap in functionality that isn't there.

[Firesphere]

I think, at the moment, it's too much effort to try and write a wrapper around an external shortcode parser, while what's already in core, is sufficient for now.
All frameworks I've checked, have their own implementation of parsers, hardly anyone uses a library (probably, because there isn't one).
The best option I can think of, is indeed either leaving it and clean it up a bit (like what I'm doing due to the removal of oEmbed), or make it a micropackage, which we might possibly upgrade to a full parser, where our parserpackage would become the standard.

[dhensby]

That's a shame - I think it'd be great to pull out what we can, even if it means supporting a 3rd party library. that parser looks pretty well architected and the kind of thing that would work well for us.

[Firesphere]

Agreed, but it is the only one that actually seems to be somewhat maintained. And, as @sminnee said, maintained by 1 person, not a community.
I've tested it a bit, it's really nice to work with, but it's unsuitable at the moment. See the issue I raised, where the answer was "you can disable the p-tag on TinyMCE". To me, that's not the answer that you want, because you can't expect everyone to do that.
TL;DR: For now, this issue is parked until someone comes up with a solution.

[chillu]

+1 on moving BBCodeParser out of core into a module before we release a 4.0 beta - anybody opposed to this? Unlike a shortcode parser, I don't see a good reason why BBCode parsing belongs into core.

[sminnee]

I'd suggest we quickly shift BBCodeParser to an abandoned module on SilverStripe-archive, make that a dependency of forum, and raise an issue on forum to switch to another parser lib?

[sminnee]

@chillu I've done this here #5998 and here https://github.com/silverstripe-archive/silverstripe-bbcodeparser

[sminnee]

@Firesphere I'm closing this - i think that #5998 covers the most important change

[sminnee]

FYI the question of whether we can use the thunderer library has been picked up again at #5987

[mrmorphic]

Hi. I've been doing a little work with shortcodes lately. Our content team needed nested shortcodes in particular. For anyone who is interested in an immediate (interim) solution it's here: https://github.com/GOVTNZ/aacustomshortcodes

It is a backwards compatible (runs all existing framework tests) drop-in replacement for the framework's shortcode parser. When registering a shortcode you can tell the parser more about the shortcode so it understands it's nestable, which guides the parser.

While it inherits from ShortcodeParser, it is almost completely self-contained, so could form a direct replacement for the framework's parser if that was considered desirable.

M

[tractorcow]

Thanks @mrmorphic ; I'll keep an eye on your work, and in particular the possibility to support nested shortcodes, once we move to a new shortcode parser. :)

[tractorcow]

I'm going to re-open this; Although the specific crash has been removed, the shortcode parser is currently not working optimally, and the need to replace this with an external lib (my pick is thunderer) still exists.

[kinglozzer]

If anyone who stumbles upon this wants to use the Thunder parser (e.g. to support nested shortcodes), it’s fairly straightforward to do that while leaving the core shortcode functionality untouched:

// _config.php
$parser = new ThunderShortcodeParser();
$parser->install();
$parser->registerThunderShortcode('my_shortcode', function(ShortcodeInterface $s) {
    return sprintf('Hello, %s!', $s->getParameter('name'));
});

<?php

namespace App\View\Parsers;

use SilverStripe\View\Parsers\ShortcodeParser;
use Thunder\Shortcode\HandlerContainer\HandlerContainer;
use Thunder\Shortcode\Parser\RegularParser;
use Thunder\Shortcode\Processor\Processor;

class ThunderShortcodeParser extends ShortcodeParser
{
    protected $thunderShortcodes = [];

    public function install()
    {
        $legacyParser = self::$instances['default'] ?? null;
        self::$instances['default'] = $this;

        // Copy registered shortcodes from core parser
        if ($legacyParser) {
            $this->shortcodes = $legacyParser->getRegisteredShortcodes();
        }
    }

    public function registerThunderShortcode($shortcode, $callback)
    {
        $this->thunderShortcodes[$shortcode] = $callback;
        return $this;
    }

    public function parse($content)
    {
        $handlers = new HandlerContainer();
        foreach ($this->thunderShortcodes as $shortcode => $callback) {
            $handlers->add($shortcode, $callback);
        }

        $processor = new Processor(new RegularParser(), $handlers);
        $content = $processor->process($content);

        return parent::parse($content);
    }
}

[GuySartorelli]

We haven't actioned this after all these years, and the shortcodeparser is not currently a high priority.
I'm going to close this issue given there aren't any obvious third-party libraries we can use which have high assurance of existing for the lifespan of Silverstripe CMS.