“Textarea field: Saving formatted value is broken” – Issues – Discuss

Textarea field: Saving formatted value is broken

A bug in 2.2.2, submitted by michael-e on 11 June 2011

Announcement

Symphony's issue tracker has been moved to Github.

Issues are displayed here for reference only and cannot be created or edited.

Browse

Closed#654: Textarea field: Saving formatted value is broken

- michael-e
- Comment #1
- 11 Jun 11, 2:23 am

The textarea field does no longer correctly save textformatter (e.g. Markdown) data to the database.

If you write this in a Markdown enabled textarea:

    <div>hello</div>

Markdown will output:

<pre><code>&lt;div&gt;hello&lt;/div&gt;
</code></pre>

but Symphony will save the following as "formatted value":

<pre><code><div>hello</div>
</code></pre>

This is definitely wrong.

I tracked this down to this commit.

(Please note: This commit did one change in line 160 which has already been reverted by a pull request. Still it is not working properly.)

- brendo
- Comment #2
- 12 Jun 11, 11:47 am

And the expected result is?

<pre><code>&lt;div&gt;hello&lt;/div&gt;</code></pre>

- michael-e
- Comment #3
- 12 Jun 11, 6:26 pm

Yes, cause this is what Markdown delivers. Any post processing at this point should only be done if the output of the textformatter (i.e. $result) is not valid XML. This should rarely be the case.

As far as I see, the old logic provided a two-step attempt to transform the output to valid XML just for these rare cases.

I suggest to revert this commit.

- brendo
- Comment #4
- 12 Jun 11, 10:54 pm

Fixed, the whole business is a bit of a mess IMO

- michael-e
- Comment #5
- 13 Jun 11, 2:26 am

Any reason why you omitted one of the "fallback steps" which would try to run the formatter on sanitized data? This is how the function looked formerly:

protected function __applyFormatting($data, $validate=false, &$errors=NULL){

    if($this->get('formatter')){
        $tfm = new TextformatterManager($this->_engine);
        $formatter = $tfm->create($this->get('formatter'));

        $result = $formatter->run($data);
    }

    if($validate === true){

        include_once(TOOLKIT . '/class.xsltprocess.php');

        if(!General::validateXML($result, $errors, false, new XsltProcess)){
            $result = html_entity_decode($result, ENT_QUOTES, 'UTF-8');
            $result = $this->__replaceAmpersands($result);

            if(!General::validateXML($result, $errors, false, new XsltProcess)){

                $result = $formatter->run(General::sanitize($data));

                if(!General::validateXML($result, $errors, false, new XsltProcess)){
                    return false;
                }
            }
        }
    }

    return $result;
}

And this is the current state:

protected function __applyFormatting($data, $validate=false, &$errors=NULL){
    if($this->get('formatter')) {
        $tfm = new TextformatterManager($this->_engine);
        $formatter = $tfm->create($this->get('formatter'));

        $result = $formatter->run($data);
    }

    if($validate === true) {
        include_once(TOOLKIT . '/class.xsltprocess.php');

        if(!General::validateXML($result, $errors, false, new XsltProcess)){
            $result = html_entity_decode($result, ENT_QUOTES, 'UTF-8');
            $result = $this->__replaceAmpersands($result);

            if(!General::validateXML($result, $errors, false, new XsltProcess)){
                return false;
            }
        }
    }

    return $result;
}

- brendo
- Comment #6
- 13 Jun 11, 11:06 am

Because if you look at the processrawfielddata function it will do that logic anyway.

- michael-e
- Comment #7
- 13 Jun 11, 6:01 pm

Short answer: No, it's not the same. You actually changed the behaviour, but the current solution is more logical (so I prefer it!).

Long answer: Consider the following content in your textarea:

Good

    <div>morning</div>

<p>Michael

Since the __applyFormatting function is first used by checkPostFieldData, this will render an error in the backend upon saving with the current solution:

"Text" contains invalid XML. The following error was returned: loadXML(): Opening and ending tag mismatch: p line 8 and rootelement in Entity, line: 18

The former solution of __applyFormatting would allow the user input to validate! But when saving (in processRawFieldData), the formatter would be applied with validation set to false, and the result would be sanitized:

General::sanitize($this->__applyFormatting($data)

(This, BTW, is not the same as $formatter->run(General::sanitize($data).)

The resulting formatted value would have been:

<p>Good</p>
<pre>
<code>&amp;lt;div&amp;gt;morning&amp;lt;/div&amp;gt;
</code>
</pre>
<p>&lt;p&gt;Michael</p>

That is no good at all. (I don't remember when this behaviour has been implemented, but I remember that we once had "invalid XML" errors; we somehow lost them on the way.)

I like the current solution. Invalid XML should not be saved. Please leave it as it is now.

This issue is closed.

Symphony.