Silverstripe-framework: Centered image alignment crashes on save

[jaydenseric]

In a HTMLEditorField, selecting Centered, on its own. for an image's alignment causes the following fatal PHP error on save:

PHP Fatal error:  Call to a member function appendChild() on null in /Volumes/Users/foo/Sites/bar/framework/View/Parsers/ShortcodeParser.php on line 187

All other alignments seem to save ok.

Observed in the latest dev-master.

[clarkepaul]

I can confirm this

[tractorcow]

A lot of painful research reveals the problem to be https://github.com/silverstripe/silverstripe-framework/pull/1190

The feature this change attempted to address no longer seems to be necessary; Tinymce shortcode handling has advanced quite considerably since 3.x.

I'm tempted to choose between a huge revert, or just dropping our core shortcode handler and dropping in something like https://github.com/thunderer/Shortcode

[tractorcow]

It's hard to come up with a quick fix which isn't simply disable all dom element re-organisation.

[sminnee]

Summarising a discussion offline with @tractorcow:

The DOM reorganisation code was designed for when shortcodes were inserted at illegal places in the DOM, eg:

<p>hello there</p>
[this_shortcode_generates_a_div]
<p>goodbye there</p>

Allegedly, more recent implementations of shortcodes in TinyMCE actually handle this by placing a <div> in the WYSIWYG editor and replacing it with the shortcode on save.

If this is the case (and needs some testing) then we could get away with a simple replacement-based shortcode handler, and we could move to the thunderer library.

/cc @Firesphere @hafriedlander

[Firesphere]

Another option would maybe be to rewrite the current shortcode parser?

The downside would be, that we would need to maintain it ourselves. The upside would be it's probably easier to update and implement, than a third party module.

Digging deep into my memory, Thunderer wasn't really that easy to setup with SilverStripe. I may have some basic implementation lingering around in some unpushed branch, but it's not the holy grail I think.

[thunderer]

Hi everyone, I'm the author of mentioned Shortcode library. I'd like to address some of the concerns raised here and in #5535. I'll link the comments and answer them here:

• https://github.com/silverstripe/silverstripe-framework/issues/5535#issuecomment-219629328 I was also surprised that there are literally no generic shortcode parsers out there, they either copy WordPress regex or implement only a non-extensible set of BBCodes. That was the main reason to provide the proper solution using an actual parser, and that's Shortcode was born,
• https://github.com/silverstripe/silverstripe-framework/issues/5535#issuecomment-219629544 yes, I'm the only maintainer, but I'm committed to supporting it. This library is quite popular, nears 30k downloads in total and over 100 daily. It is also well-designed, very extensible, and I'm open to any suggestions on how to make it better, on the basis that those improvements serve the _shortcode processing problem_ for which it was created. Also, I guess that that _key feature_ is HTML awareness? I'll be happy to discuss what you need here, maybe there is something I can do?
• https://github.com/silverstripe/silverstripe-framework/issues/5535#issuecomment-220478558 I agree that this would be a substantial change, but it's definitely possible to use one of my parsers to extract the shortcodes and then just write ProcessorInterface compliant processor, that's exactly the reason I made the library so extensible,
• https://github.com/silverstripe/silverstripe-framework/issues/5535#issuecomment-220571935 I consider the current state of Shortcode to be stable, even though there is no 1.0.0 release. As for the <p> tag issue, please see my recent answer https://github.com/thunderer/Shortcode/issues/40#issuecomment-257632082 ,
• https://github.com/silverstripe/silverstripe-framework/issues/5987#issuecomment-256250145 simple replacement is definitely something my library can do, as for the preceding / following text removal, please see my recent comment on the issue: https://github.com/thunderer/Shortcode/issues/40#issuecomment-257632082 ,
• https://github.com/silverstripe/silverstripe-framework/issues/5987#issuecomment-256258404 my recommended way of setting up Shortcode in your project is to wrap the whole logic into the service and treat it as such by exposing only what's necessary - you pass the input, you get the replaced output. I'd be happy to help make it easier, so please let me know what problems did you encounter. I'd be very grateful for your feedback.

[dhensby]

I'd throw my weight behind @thunderer's package. It can't be worse than writing our own and there's nothing stopping us forking it if things go bad with it in the long term (no offence). If that happens we're just in the same place we are now, maintaining a shortcode parser.

Maybe if @thunderer was happy to recruit some more contributors it could alleviate our worries?

[thunderer]

@dhensby Thanks for your support. I understand your worries, but let me restate that Shortcode is well designed, feature complete, extensible, and maintained. It exists for more than one and half year and was battle-tested in several projects. I'll be more than happy to hear about new feature ideas or any difficulties you encountered. Also please note that it's already used (among others) by Corcel, Grav CMS, pingpong/shortcode, coduo/php-humanizer, and will be used in next major release of golonka/bbcodeparser.

When you talk about more contributors, do you mean more people contributing to code or just more repository owners? I think that there were so little contributions so far only because when I design a feature, I think about the whole abstraction so that you can extend what you need in your project and do not need to change the upstream. Open/Closed principle, if you wish. :) This comes directly from the "functional approach" idea I came across years ago. I even managed to talk about it on several conferences.

[tractorcow]

Hi @thunderer I firstly want to say thank you greatly for the amount of interest and thought you'd put into empathising with our issues. As an open source project we really benefit from having a positive relationship with other library developers, so it's great that we can collaborate in this way. :)

Given your feedback and suggested fixes, I see that any of the possible road-blocks we have to implementation have been addressed. (see https://github.com/thunderer/Shortcode/issues/40#issuecomment-257632082)

I would like to put my weight behind the conversion of this fix into a new proof of concept silverstripe shortcode replacement. @sminnee given your insight into the original work done on shortcode handling, do you feel likewise?

@thunderer if this became the case, perhaps I will myself be able to contribute back to your project, as is normally the case when we take on third party dependencies. :) We rarely find ourselves not contributing back, as we have to jquery-validate, composer, monolog, etc.... perhaps that was what @dhensby was hinting at.

[dhensby]

ping @sminnee

[tractorcow]

It's now popping up in CMS behat tests in insert media. Priority is now increased.

[tractorcow]

I've attempted to disable this feature at https://github.com/silverstripe/silverstripe-framework/pull/6349, although this simply disables the feature rather than fixes it.

[adrian-stein]

Any word on this issue and when it may be fixed, As it is still happening.

[tractorcow]

@adrian-stein which version of framework are you experiencing this on?

[adrian-stein]

3.5.0

[tractorcow]

That's a shame; It's going to be troublesome to disable this feature in 3.5 without semver repurcussions, and the actual issue itself is very difficult to fix properly.

@dhensby can you think of a semver-way to address this issue? The only fix I can suggest is #6349 which violates semver in 3.x.

[tractorcow]

Are you sure you're on 3.5.0 and not 3.5.x-dev?

It seems that https://github.com/silverstripe/silverstripe-framework/commit/a4760b8ee409c2c96a0e77445debf931259cb9aa causes this bug to appear in 3.x, but this isn't apparent until 3.5.1, as 3.5.0 doesn't have this patch yet.

@dhensby IMO this blocks 3.5.1 release.

[tractorcow]

Ok, so that patch only creates the crash in Left, on its own which is the default. Centered on its own has always been broken and remains so.

[adrian-stein]

Yup on .0

"silverstripe/cms": "3.5.0", "silverstripe/framework": "3.5.0",

I am needing "Centered on its own", "Left, on its own" doesnt produce the error.

[tractorcow]

Yeah, that makes sense. :) The issue always existed and now it's about to get worse unless it's patched before 3.5.1.

[tractorcow]

@sminnee could you please help us make the call, can we disable this feature on 3.5 given that it crashes?

[tractorcow]

By the way @adrian-stein thanks a lot for the prompt reply. You're a great help. :)

[adrian-stein]

No problem, happy to help. Me being quick helps you, and you fixing it, hopefully quick helps me :)

[adrian-stein]

Also you talk about disabling it. Does that mean we wont be able to centre media? As it is a fairly useful and important feature for me.

[tractorcow]

Oh, you'll be able to center of course. What will happen is that it will no longer attempt to wrap the shortcode in a container element, nor break it in the middle of parent elements.

[sminnee]

I recommend that we remove the use of ShortcodeParser::SPLIT and revert to the 3.4 use of ShortcodeParser::INLINE for both center and leftAlone images.

[tractorcow]

Thanks @sminnee , +1 for this, as it appears to be the least intrusive. Not urgent, but let's get this in before 3.5.1

[sminnee]

If 3.5.1 is already in RC then it's urgent. Can either @dhensby or @tractorcow pick it up? :-)

[tractorcow]

I'll tag it tomorrow.

[adrian-stein]

Thanks for the quick response guys. Il look forward to 3.5.1. Any ideas when it will be released?

[tractorcow]

Tomorrow! Just gonna make the call. :)

[tractorcow]

Minimal 3.5.1 fix at https://github.com/silverstripe/silverstripe-framework/pull/6436; Will use INLINE in place of SPLIT for now.

[sminnee]

Merged