silverstripe / silverstripe-framework

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

HTMLText doesn't honour the `shortcodes` options #8963

Open maxime-rainville opened 5 years ago

maxime-rainville commented 5 years ago

Affected Version

SS 4.4, but probably all the other ones as well

Description

HTMLText supports a shortcode options, but doesn't honour it. I think that's because the the model.yml injector settings take precedence over the $db settings.

https://github.com/silverstripe/silverstripe-framework/blob/ed7aaff7da61eefa172fe213ec25e35d2568bc20/_config/model.yml#L27-L30

Steps to Reproduce

In a DataObject db definition, add an HTML field like this:

<?php
use SilverStripe\ORM\FieldType\DBHTMLText;
use SilverStripe\ORM\DataObject;
use SilverStripe\Forms\HTMLEditor\HTMLEditorField;

class Dummy extends DataObject
{
    private static $db = [
        'NoShortCode' => 'HTMLText(array("shortcodes"=>false))'
    ];

    public function getCMSFields()
    {
        $fields =  parent::getCMSFields();
        $fields->addFieldToTab(
            'Root.Main',
            new HTMLEditorField('NoShortCode', 'NoShortCode', $this->NoShortCode)
        );
        return $fields;
    }
}

Edit the content of NoShortCode via a WYSIWYG and add an image. Get the content of NoShortCode rendred in a template.

Expected results: The short code should be outputted as-is in plain text. Actual results: The short code is converted to an image.

If you define your DB field using the FQNS of HTMLText, it works:

private static $db = [
     'NoShortCode' => DBHTMLText::class . '(array("shortcodes"=>false))'
];
kinglozzer commented 5 years ago

Does defining your property with HTMLFragment achieve the same effect?

private static $db = [
     'NoShortCode' => 'HTMLFragment'
];

If so, it may be easier to deprecate the array("shortcodes"=>false) approach

maxime-rainville commented 5 years ago

We use the same gimmick for HTMLVarchar, which default to shortcode => false. Only because, it's not explicitly define in the Injector YML, so 'NoShortCode' => 'HTMLVarchar(255,array("shortcodes"=>true))' works just fine.

I don't think it's a big issue because DBHTMLText::class works just fine and provide a suitable workaround ... and because it's pretty unusual to want to disable shortcodes in a HTMLText field.

tractorcow commented 5 years ago

This is also a problem at the Injector level; When providing constructor arguments, these can be provided not only at the string level (string arguments in the injector specifier, e.g. Injector::inst()->get("HTMLText(['shortcodes' => true])") but also provided by injector directly. E.g. Injector::inst()->get('HTMLText', ['shortcodes' => true]).

Injector resolves this by joining both lists of arguments together. You would end up with the shortcodes argument being passed as the second and third argument (after the field $name).

The real fix here is to safely document and capture incorrect arguments being passed into HTMLText (or other field types), and to encourage / document the use of HTMLFragment where user arguments are necessary.

michalkleiner commented 5 years ago

Just found a related issue. HTMLVarchar misses the whitelist functionality that HTMLText has, even though it's mentioned in the docblock of the setOptions method. The problem seems to be that the HTML fields don't inherit from the same ancestor where this kind of functionality would reside.

maxime-rainville commented 5 years ago

@michalkleiner That's sounds like a distinct problem. Would you mind raising it a separate issue?

michalkleiner commented 5 years ago

Extracted into #9104