silverstripe / silverstripe-framework

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

Text field output no longer converts newlines #1942

Closed simonwelsh closed 11 years ago

simonwelsh commented 11 years ago

In 3.0, due to changes to Convert::raw2xml, Text no longer converts newline characters to <br> tags when being outputted to a template.

2.4 behaviour: DBField::create('Text', "Some string\nwith newline")->forTemplate() -> Some string<br />\nwith newline

3.0 behaviour: DBField::create_field('Text', "Some string\nwith newline")->forTemplate() -> Some string\nwith newline

tractorcow commented 11 years ago

In DBField.php

    public function forTemplate() {
        return $this->XML();
    }

    public function HTML(){
        return Convert::raw2xml($this->value);
    }

    public function XML(){
        return Convert::raw2xml($this->value);
    }

I propose that the HTML function be changed to either:

DBField::forTemplate could still return $this->XML, but for StringField this could be overridden to call $this->HTML instead.

wilr commented 11 years ago

Not sure it if would help but been playing around with having DBField methods return DBField objects so you could chain for the behaviour to be more flexible (e.g Content->FirstSentence()->StripTags()->LimitCharacters(10) in vhttps://github.com/wilr/sapphire/commit/df657955b09c683270b898c387a14b74c09aaa3f . Perhaps something like that would make it less 'magic' as to how something like FirstSentence handles XML / Newlines etc..

tractorcow commented 11 years ago

I had thought a bit more about this; How about adding an optional second parameter to raw2xml that toggles newline behaviour? E.g.

Convert::raw2xml($input); // Leaves newlines Convert::raw2xml($input, true); // Replace newline with <br />

chillu commented 11 years ago

The raw2xml() description reads: "Ensure that text is properly escaped for XML.". IMHO newline conversion doesn't fall into this category, since \n doesn't need to be escaped as part of the XML/HTML standards in the same way that < and > do.

In most cases, newline conversion is what the dev wants though when inserting plaintext into HTML. So I'm happy to go back to the original behaviour through nl2br($this->XML()). Will's chaining fix will make this easier eventually, of course :)

tractorcow commented 11 years ago

Makes sense to me, as does the explanation to my suggestion, thanks.

kinglozzer commented 11 years ago

@chillu Just looking to confirm what you wanted to go with:

    //DBField.php
    public function forTemplate() {
        return nl2br($this->XML());
    }

or

    //DBField.php
    public function HTML() {
        return nl2br($this->XML());
    }

or something entirely different as I've misunderstood :stuck_out_tongue_closed_eyes:? Also, I assume this'll be against 3.1 not master?

chillu commented 11 years ago

@tractorcow @wilr Can either one of you make a call on this? I've got too much other 3.1 stuff to look at.

tractorcow commented 11 years ago

I'd like to go back to the original behaviour.

Will the following work?

My content is $TextContent.XML

Or will we need to use $TextContent.HTML to get newlines?

simonwelsh commented 11 years ago

The way I see it, the .XML/.HTML methods make the value safe for raw inserting into those types of locations, so shouldn't be returning tags.

So I'm calling it. It should be in forTemplate(), and just on StringField. @kinglozzer wanna make a PR against 3.1? Including tests so this regression doesn't popup again.

@tractorcow You'd just use $TextContent, leaving .HTML/.XML for use when you want escaping but not newline conversion (i.e., inside a <textarea>)

tractorcow commented 11 years ago

The problem is that the standard practise is to choose the correct encoding for each field usage. I'd typically always use one of .RAW, .XML, or .ATT whenever I'm using a value. There should be some explicit tag that the default simply falls back to.

Simpling putting it in forTemplate is probably good enough for 3.1 RC1 though, it's not a big thing, just a matter of style. :) I agree that we do this and leave it for a future discussion.

bbt-mahfuz commented 10 years ago

Could you please tell me how can I output new lines as
tags?

Thanks.

tractorcow commented 10 years ago

Check my response in https://github.com/silverstripe/silverstripe-framework/issues/3520

Newlines will be converted to <br /> by default in templates when not using any encoding specification.