Double quagmire: 1.) Possible bug in ShortcodeParser but 2.) Can't easily override it

[Patrick Nelson]

I'm noticing what I think might be a potential bug in SilverStripe's ShortcodeParser class. Both of these are sort of complex, but I'll cover the bug first and then cover why it's also a problem that I can't work around this cleanly or in a suitably extensible fashion.

Issue #1: Possible Bug in ShortcutParser Class

In an unrelated Github issue #4095, I outlined a desire to setup captions with YouTube videos (per our client request, this is required for inline editing within TinyMCE, thus the time dedicated to it). After resolving that, I noticed that the embeded shortcodes, e.g. [embed ... ], are parsed in such a way that their resulting HTML tags will be moved around, even for <iframes> which should be treated in the same fashion as <img> tags (but they are not, when passed through this parser). For example, this code:

<div class="captionImage center" style="width: 480px;">
   [embed width="480" height="270" class="center" thumbnail="http://i.ytimg.com/vi/1JjpHmt7J8g/hqdefault.jpg?r=90570"]http://www.youtube.com/watch?v=1JjpHmt7J8g[/embed]
   <p class="caption center">Test Caption</p>
</div>

Will become:

<div class="captionImage center" style="width: 480px;">
</div>
<div class="media center">
   <iframe width="480" height="270" src="http://www.youtube.com/embed/1JjpHmt7J8g?feature=oembed" frameborder="0" allowfullscreen=""></iframe>
</div>
<div class="captionImage center" style="width: 480px;">
   <p class="caption center">Test Caption</p>
</div>

As you can see in the highlighted area, the actual embed tag is broken outside of the containing tag, new tags cloned and prepended/appended and etc (retaining inline styles) which have become siblings. Since an <iframe> tag is inline, wouldn't it be preferable that it would be treated like an image class? Preferably, this would result instead since it's inline:

<div class="captionImage center" style="width: 480px;">
   <div class="media center">
      <iframe width="480" height="270" src="http://www.youtube.com/embed/1JjpHmt7J8g?feature=oembed" frameborder="0" allowfullscreen=""></iframe>
   </div>
   <p class="caption center">Test Caption</p>
</div>

Using CSS, I can target the .captionImage.center class and do what I need to accomplish the desired effect. The same is true for the other classes (left, right and leftAlone). Even if this is still needed as a general rule of thumb for reasons outside of my own perception, I should have a method by which I can override this...

Issue #2: Cannot easily override it.

For privacy's sake, my site's example module name will be "demo" which, like the real module, falls between "cms" and "framework" alphabetically. The ShortcodeParser class [un/fortunately] maintains global state and retains a singleton instance all on its own via a static ::get() method. and can be configured and etc. There are a few techniques that I could use to try to override the ->moveMarkerToCompliantHome() method (the culprit in my case) to do what I want to do. 

1. Manually edit the method (bad for very obvious reasons).
   
   
2. Since SS doesn't have a true IoC container, I could try using Object::useCustomClass() to "overload" with my own class, which could extend the ShortcodeParser, but that requires setting up another module like "aaa" just to ensure that my version of the class runs first alphabetically and hope to gosh that nothing changes that particular internal functionality since now I'm depending on modules always loading alphabetically (due to how this global state is being maintained).
   
   
3. Do the opposite and setup a module called "zzz" try to set the active shortcode parser to be my parser instead, but that poses other issues. I'll already be depending on the alphabetical loading issue, but now I also have no way to set my instance of the parser to be configured the same way as the original parser. For example: ShortcodeParser doesn't allow public access (at least no getter) to its registered instances or its registered short code handlers. I can override the "embed" one that's currently there, but I could be clobbering an existing handler that may change in the future.

Conclusion:

So I'm sort of stuck, without some sort of IoC configuration that I can define ahead of time (like I could with Laravel via service providers), I don't have a method of changing this without manually editing some framework file and adding another somewhat hacky patch to help resolve the particular issue that this is causing. This does touch, by the way, somewhat on the discussion about using Traits instead of Extensions for modular extensibility in the SS framework and resolves the "How do we extend core functionality without editing core files when we're using traits now instead?" -- this is how you would do that.

Does anyone have a recommendation for either #1 or #2 above? e.g. Maybe a general bug fix which can be contributed to core framework or possibly a better method (than I've outlined) for overriding built-in class functionality? Thanks!

- Patrick

[Patrick Nelson]

I think I'm going to go this route, since it looks like this class should probably already be a child of the Object class anyway, for consistency. Then I can overload it and do what I need externally without modifying core code (aside from this, of course). 

Issue: https://github.com/silverstripe/silverstripe-framework/issues/4100

PR: https://github.com/silverstripe/silverstripe-framework/pull/4101

[Martine bloem]

Instead of using Object::useCustomClass(), could you use an injector in yml? As I understand it the config system is loaded up front? So that would eliminate the aaa, ccc, zzz mess...

--
You received this message because you are subscribed to the Google Groups "SilverStripe Core Development" group.
To unsubscribe from this group and stop receiving emails from it, send an email to silverstripe-d...@googlegroups.com.
To post to this group, send email to silverst...@googlegroups.com.
Visit this group at http://groups.google.com/group/silverstripe-dev.
For more options, visit https://groups.google.com/d/optout.

[Patrick Nelson]

Are you a wizard?

Seriously though -- that's a better alternative, thanks for that suggestion :) It seems to work pretty well with the slight modifications I've made in PR #4101. 

[Martine bloem]

Haha hardly :) 

But just today, fixing someone's old 2.4 site, I found myself trying to implement an injector, now thát didn't work :)