search

thunderer / Shortcode

Advanced shortcode (BBCode) parser and engine for PHP
http://kowalczyk.cc
MIT License
376 stars 30 forks source link

Ideas #16

Open thunderer opened 8 years ago

thunderer commented 8 years ago

Just a list of issues to remember:

Regular parser:

BBCode:

Built-in handlers:

rhukster commented 8 years ago

I have a few of ideas to add to the list:

  1. Option to only process registered handlers.
    Related to the performance issues I have seen in RegularParser it might be good to have more checks to ensure that only registered handlers are processed. During my testing of a page in my documentation that was using square brackets to highlight method signatures, these were being treated as shortcodes.

    I think that either by default, or at the very least an option should be to ignore all shortcodes that don't explicitly match the registered handlers. This would improve performance dramatically and limit the amount of work the library has to do.

  2. Direct control over nested tags. With regards to nested handlers, currently you can nest any shortcode. However there are situations where you a particular shortcode can only be nested inside another. In fact in many complex nested shortcodes, there are situations where the exact nesting of the shortcodes is important, and should only work if the correct nesting is found.

    A simple example of this is with tabs:

    [ui-tabs]
[ui-tab title="Tab 1"]
This is some content for Tab 2
[/ui-tab]
[ui-tab title="Tab 2"]
This is some content for Tab 2
[/ui-tab]
[/ui-tabs]

    As you can clearly see, a ui-tab shortcode only makes sense inside a ui-tabs shortcode. Other shortcode libraries I have come across allow you to define which child/nested shortcodes should be processed by the parent shortcode. This could increase performance and increase reliability of nested shortcode setups.

  3. Shortcode ID. This is something I ran into when developing my Tabs shortcode. Basically I needed to get a unique ID for each particular shortcode. To accomplish this I created an id based on the shortcode 'text':

    private function getShortcodeId($shortcode)
{
   return substr(md5($shortcode->getShortcodeText()), -10);
}

    This is not ideal, but serves my purposes. It would be really useful if an ID could be generated and assigned to each unique shortcode instance.

  4. Shortcode getChildren() The ProcessedShortcode class provides a really useful getParent() method which I use extensively when working with nested shortcodes. This allows me to get at a parents parameters when working with children. However, it would be really useful to be able to get an array of child shortcodes from the parent. Currently I get around this by creating an array at the object level and assigning all children to a unique parent id. This way I can retreive the shortcodes directly from the parent:

    $this->handlers->add('ui-tabs', function(ShortcodeInterface $shortcode) {

     // Add assets
     $this->assets->add('js', ['jquery', 101]);
     $this->assets->add('js', 'plugin://shortcode-ui/js/ui-tabs.js');
     $this->assets->add('css', 'plugin://shortcode-ui/css/ui-tabs.css');

     $hash = $this->getShortcodeId($shortcode);

     $output = $this->grav['twig']->processTemplate('partials/ui-tabs.html.twig', [
         'hash' => $hash,
         'active' => $shortcode->getParameter('active', 0),
         'position' => $shortcode->getParameter('position', 'top-left'),
         'theme' => $shortcode->getParameter('theme', $this->grav['config']->get('plugins.shortcode-ui.theme.tabs', 'default')),
         'child_tabs' => $this->child_states[$hash],
     ]);

     return $output;
 });

 $this->handlers->add('ui-tab', function(ShortcodeInterface $shortcode) {
     // Add tab to tab state using parent tabs id
     $hash = $this->getShortcodeId($shortcode->getParent());
     $this->child_states[$hash][] = $shortcode;
     return;
 });

    This works but could be more elegant if i didn't have to have manage my own child_states array.

FYI Some of these ideas stem from the discussions in PR #26

thunderer commented 8 years ago

@rhukster these are all great ideas, let me explain how they could be achieved:

  1. shortcodes without handlers are not processed (they are returned verbatim), but their content may be processed if Processor::withAutoProcessContent() is set to true. If you want to remove them then look at the branch runtime-events, there is an event FilterShortcodesEvent using which you could remove all matched shortcodes at given level without registered handler. I'm still working on the events subsystem, but you get the idea. There remains one question though: what to do with nested shortcodes with handlers?
  2. as for direct control over nested shortcodes I'd suggest to invert the control and validate parent shortcode name in child, as you mentioned there is ProcessedShortcode::getParent() method which could come in handy. If the parent is invalid, just return ProcessedShortcode::getContent(). Would it solve your problem?
  3. shortcode ID is a cool idea, I came to the conclusion that the only truly unique information about given shortcode is an offset from the beginning of the whole text. That's why I plan to introduce ProcessedShortcode::getBaseOffset() which will return the number representing an offset from the beginning of the whole text. What do you think?
  4. getting children of given shortcode is as easy as creating a parser object and calling parse() method on the shortcode's content. I see that this could be a lot easier if I exposed the Processor::getParser() method so that you could write in handler (assuming that $s is an instance of ProcessedShortcode) that $children = $s->getProcessor()->getParser()->parse($s->getContent()); or even $children = $s->getChildren() that would encapsulate the behavior for you.

Please let me know what do you think.

rhukster commented 8 years ago

1) I think an event model would be handy to customize the processing rules as long as it was not overly complex or expensive in processing time. iv'e not looked at that branch, but I like the concept.

2) Your solution should solve that problem yes.

3) The getBaseOffset() would work as a unique id. So yes, that would do nicely.

4) Yes, getting access to the parser would be most helpful.

Thanks!

thunderer commented 8 years ago

@rhukster getBaseOffset() was introduced in changesets https://github.com/thunderer/Shortcode/compare/c87b3e97714dcf98ae9ae7a5a26af883b534e2a4...9f74c82d723529b0f2e3de7f7d22512efe5f6fe1 . Also you may be interested in new ShortcodeFacade (with README update) PR that was merged today. That should sum up all the ideas you came up with so far. :smile: I got no more ideas so v0.6.0 should be tagged after weekend.

rhukster commented 8 years ago

Yup, i saw you merged your facade updates. I'm going to take a look and probably update my Grav shortcode-core plugin to use it. I'll test the getBaseOffset() and with my recent changes it should be a good solution for getting a unique id of each shortcode.

Prior to recent updates, i was caching independently of the page, so each shortcode needed a unique id site-wide, which a simple offset would not provide. However, now, things are cached per-page, so should be good to go.

Thanks!

dimayakovlev commented 7 years ago

Can you add option for method getParameter to return parameter value as boolean?

What I mean:

public function getParameter($name, $default = null, $boolean = false)
{
  $value = $this->hasParameter($name) ? $this->parameters[$name] : $default;
  return $boolean ? filter_var($value, FILTER_VALIDATE_BOOLEAN) : $value;
}
thunderer commented 7 years ago

@dimayakovlev can you show me some use cases for this? Isn't casting to boolean sufficient?

$value = (bool)$s->getParameter('name');
dimayakovlev commented 7 years ago

It can be useful if parameter value is 'true' or 'false' (strings).

This is example from Grav Shortcode plugin: Safe-Email Address: [safe-email autolink="true" icon="envelope-o"]user@domain.com[/safe-email].

It does not matter if autolink="true" or autolink="false", this part of code would be executed https://github.com/getgrav/grav-plugin-shortcode-core/blob/develop/shortcodes/SafeEmailShortcode.php#L29-L30.

Simple casting to boolean in this scenario does not work (no problems if parameter value can be only '1' or '0', but). So it will be easier if the method getParameter or other special method, can return Boolean, because strings false and true are often used as parameters values.

thunderer commented 7 years ago

@dimayakovlev Thanks, but in this case, I would rather prefer that you call filter_var() in your own shortcode handler. This leaves you in control and does not introduce unnecessary optional parameters in library code.

$value = filter_var($s->getParameter('name'), FILTER_VALIDATE_BOOLEAN);
dimayakovlev commented 7 years ago

@thunderer ok. :)

jenstornell commented 6 years ago

I have an idea. The first thing I noticed was that it quite much code, even for the most simple task, as we can see here:

use Thunder\Shortcode\ShortcodeFacade;
use Thunder\Shortcode\Shortcode\ShortcodeInterface;

$facade = new ShortcodeFacade();
$facade->addHandler('hello', function(ShortcodeInterface $s) {
    return sprintf('Hello, %s!', $s->getParameter('name'));
});

$text = '
    <div class="user">[hello name="Thomas"]</div>
    <p>Your shortcodes are very good, keep it up!</p>
    <div class="user">[hello name="Peter"]</div>
';
echo $facade->process($text);

Maybe for this case simplify it even more. Something like this?

shortcode::create('hello', function($s) {
  return sprintf('Hello, %s!', $s->get('name'));
});

$text = '
    <div class="user">[hello name="Thomas"]</div>
    <p>Your shortcodes are very good, keep it up!</p>
    <div class="user">[hello name="Peter"]</div>
';
echo shortcode::process($text);
thunderer commented 6 years ago

@jenstornell I already had that discussion back when I released first versions, you can read my answer here. The ShortcodeFacade class is already a compromise that provides sane defaults and allows the simplest possible usage while maintaining high software quality. Singletons and the global state itself can cause major issues with stability and testability of your implementation. Though, if you really want to build a singleton to wrap Shortcode then it is very easy to write something like this in your project:

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

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

final class ShortcodeSingleton
{
    /** @var HandlerContainer */
    private static $handlers;
    /** @var Processor */
    private static $processor;

    private static function instance(): Processor
    {
        if(null === static::$processor) {
            static::$handlers = new HandlerContainer();
            static::$processor = new Processor(new RegularParser(), static::$handlers);
        }
    }

    public static function add(string $name, callable $handler): void
    {
        static::instance();
        static::$handlers->add($name, $handler);
    }

    public static function process(string $text): string
    {
        static::instance();
        return static::$processor->process($text);
    }
}
jenstornell commented 6 years ago

I did something similar to my Kirby CMS plugin:

https://github.com/jenstornell/kirby-shortcode/blob/master/lib/shortcode.php

echo shortcode::parse($text);

But I understand your concerns with this approach. For my plugin it's probably the best but not for all other scenarios.