Returns wrong shortcode content

[dimayakovlev]

If shortcode content is 0 (zero), then $sc->getContent(); return NULL.

[thunderer]

Hi @dimayakovlev, I created the Shortcode library used here in Grav. From my initial investigation, it turns out that it's indeed an issue, but only when you're using wordpress parser (default in this plugin). Can you post your plugin configuration and/or try with regular?

I'd appreciate if you could create an issue in my repository so that I can check if it's a limitation or an actual bug that can be solved. WordpressParser is meant for the complete compatibility with WordPress, so I'm only fixing inconsistencies, not its limitations.

Meanwhile, @rhukster, could you please change default parser configuration value to regular and update the README?

[rhukster]

hey @thunderer, thanks for the comment. I'm sure you remember I posted some performance issues on your repo a while back. I found the standard parser is just way to slow for Grav's use. The impact on most pages was in the 100s of milliseconds. That might not sound like much but for Grav which is a very performance oriented CMS, that's huge.

I know the standard parser is preferred as its more OO than the heavy regex-based wordpress parser, but the performance benefits are just too great for us :). We'll be sticking with wordpress unless the standard parser performance has increased significantly.

[thunderer]

@dimayakovlev I identified the issue as an incompatibility with WordPress inside WordpressParser. It was a really hard to find edge case in which type-coerced content was considered "falsy" and as such it was discarded. I just fixed that problem and released v0.6.4. Thank you for reporting this! I'm pretty sure I wouldn't find this just by looking at the code.

@rhukster I remember the excellent discussion we had in thunderer/Shortcode#27 and other issues. Did you evaluate RegularParser's performance with PHP7? If you don't need nesting shortcodes then RegexParser is still a fine choice, but WordpressParser is meant only for people who want 100% compatibility with WordPress. Maybe change the default parser to regex then? Could you please also update Shortcode's composer.json entry to ^0.6.4 and composer.lock accordingly? Thanks!

[dimayakovlev]

@thunderer thank you for your work!

[rhukster]

I updated to ^0.6.4 in composer.

I tried the regular parser with PHP 7.0.13, and my test page took 990ms to render. When switch back to regex or wordpress it took only about 150ms. So still a sizable difference.

Interestingly I have a couple of broken shortcodes when I use regex compared to wordpress. I'll re-release with the fixes you made for the wordpress parser, and then try to work out why those other shortcodes are breaking for regex.

Thanks!

[rhukster]

Ok this is weird, i'm not sure what's happening.. I have this code for fontawesome shortcode:

$this->shortcode->getHandlers()->add('fa', function(ShortcodeInterface $sc) {
            // Load assets if required
            if ($this->config->get('plugins.shortcode-core.fontawesome.load', false)) {
                $this->shortcode->addAssets('css', $this->config->get('plugins.shortcode-core.fontawesome.url'));
            }

            // Get shortcode content and parameters
            $str = $sc->getContent();
            $icon = $sc->getParameter('icon', false);

            if (!$icon) {
                $icon = $sc->getParameter('fa', trim($sc->getParameterAt(0), '='));
            }

            if (!Utils::startsWith($icon, 'fa-')) {
                $icon = 'fa-'.$icon;
            }

            $extras = explode(',',$sc->getParameter('extras', ''));
            foreach ($extras as $extra) {
                if (!empty($extra)) {
                    if (!Utils::startsWith($extra, 'fa-')) {
                        $extra = 'fa-'.$extra;
                    }
                    $icon .= ' '.$extra;
                }
            }

            $output = '<i class="fa '.$icon.'">'.$str.'</i>';

            return $output;
        });

And these examples:

[fa=cog /]

[fa icon=fa-camera-retro /]

[fa icon=fa-camera-retro extras=fa-4x /]

[fa icon=fa-circle-o-notch extras=fa-spin,fa-3x,fa-fw,margin-bottom /]

With wordpress these all work fine. With regex on the first [fa=cog /] example is picked up. I put a break point in the code and it's not even hit with the other ones. Any ideas @thunderer ??

[thunderer]

Hi @rhukster, thanks for additional test data, can you share your test document so that I can run my own benchmark against it? It would greatly help me investigate these performance drops. I already have some assumptions about what is going on under the PHP's hood, but I need to verify them.

As for the invalid RegexParser results, I prepared a PR thunderer/Shortcode#44 with explanation and test cases from your comment. Could you please test if it solves the issue? BTW I see that you managed to get around the lack of "BBCode" support in WordpressParser by removing the beginning =. This is one of the reasons I created my own RegexParser which does handle "BBCode" part correctly.

Again, your examples greatly help me find and solve all the problems with Shortcode. I am very thankful for the amount of the information I get from you.

[rhukster]

Thanks @thunderer, I tested with your PR and it works fine with the syntax I have above.

Now regarding the test cases.. It's a little tricky because everything i'm doing is based on a couple of plugins that we have for Grav which extend your core Shortcode library.

The simplest way is to setup a Grav instance (just Apache + PHP) - MAMP will do.

https://github.com/getgrav/grav/releases/download/1.1.9/grav-v1.1.9.zip

Then turn off caching (to get a better idea of how the processing is working) in user/config/system.yaml

Then install shortcode-ui plugin, as it depends on the shortcode-core plugin (where your vendor folder is located) that will be installed also:

$ bin/gpm install shortcode-ui

Once you have that, you can replace the core user/pages/01.home folder with the following.

01.home.zip

This contains my sample page with some images. This page uses most of the shortcodes defined in core and ui plugins.

it's really easier than it sounds! :)

[rhukster]

BTW, if you have any questions or issues, ping me and i can walk you through getting it up and running in a few mins.

[thunderer]

Hi @rhukster, I merged thunderer/Shortcode#44 and released v0.6.5 so your FontAwesome shortcode (and similar ones) should work as expected.

Thanks for the ZIP archive, I extracted the default.md file and ran a simple benchmark on PHP 7.1.0 with the script attached below. The times are not as bad as you reported above, but still - RegexParser takes 28-30ms on average and RegularParser needs 125-150ms to complete the process. I will think how to improve its performance but meanwhile, please settle with RegexParser.

<?php
declare(strict_types=1);
namespace X;

use Thunder\Shortcode\EventContainer\EventContainer;
use Thunder\Shortcode\HandlerContainer\HandlerContainer;
use Thunder\Shortcode\Parser\RegexParser;
use Thunder\Shortcode\Parser\RegularParser;
use Thunder\Shortcode\Processor\Processor;

require __DIR__.'/vendor/autoload.php';

$text = file_get_contents(__DIR__.'/default.md');
$handlers = new HandlerContainer();
$events = new EventContainer();

$time = microtime(true);
$processor = new Processor(new RegexParser(), $handlers);
$processor = $processor->withEventContainer($events);
$processor->process($text);
echo ((microtime(true) - $time) * 1000).'ms'."\n";

$time = microtime(true);
$processor = new Processor(new RegularParser(), $handlers);
$processor = $processor->withEventContainer($events);
$processor->process($text);
echo ((microtime(true) - $time) * 1000).'ms'."\n";

[rhukster]

Nice! I'll check this out shortly.

[rhukster]

@thunderer I just tested with shortcode 0.6.5, and that [fa=cog /] syntax still does not work with regex, only wordpress processor.

[thunderer]

@rhukster that's strange, I have a test for that and the only parser that does not support such syntax is WordpressParser (it does not support separate BBCodes at all, reports =bbcode as its first parameter). Can you share a short code example with the desired effect? Do you use getParameterAt() or getBbCode()?

[rhukster]

Yes, its this code i pasted above: #25 (comment)

[thunderer]

@rhukster please use getBbCode() instead of getParameterAt(0). WordpressParser does not understand BBCode part (ie. =cog) and treats it as a regular parameter. All other parsers do this correctly and separate that part from other parameters.

[rhukster]

Done, and released. Thanks.