silverstripe / silverstripe-framework

Silverstripe Framework, the MVC framework that powers Silverstripe CMS
https://www.silverstripe.org
BSD 3-Clause "New" or "Revised" License
720 stars 820 forks source link

Refactor shortcode parsing #11260

Open maxime-rainville opened 1 month ago

maxime-rainville commented 1 month ago

Our approach to managing short code is dated and uses many anti-patterns.

Each individual shortcode class must implement the ShortcodeHandler interface. This interface requires the usage of static method. Nowadays, we prefer to use the singleton pattern and use instance methods.

Short code must be registered in _config.php by making this kind of calls: https://github.com/silverstripe/silverstripe-framework/blob/8886a3a93ddf5d9ad0b677f7403ca600894bbe00/_config.php#L14-L15

Registering shortcodes should be handle via the Injector.

Most of the logic in ShortcodeParser is dedicated to parsing BBCode shortcodes. There's libraries around for this exact purpose:

We should consider adopting them instead of maintaining our own parser.

maxime-rainville commented 1 month ago

I had a very quick look at the two libraries:

GuySartorelli commented 1 month ago

Each individual shortcode class must implement the ShortcodeHandler interface.

That's not true - shortcode parsers don't even need to be classes. You only register a callback. It could even be an anonymous function with the current setup.

But yes, I agree we should offload this to a third-party library if one is available and appropriate.

Alternatively we should allow registering them with yaml config, even if we change nothing else about our current approach.