Add Nice() to DBField

[lekoala]

Description

Currently, Nice() is only implemented in subclasses. This means that if you have something like dbObject('MyField')->Nice() phpstan will complain about it.
My proposal is to add as a base method a "Nice" function returning a string (could be a docblock type for now for no bc break, but i'm pretty much sure the use case is to get a string at the end of the day) returning the value or a string if null.

Acceptance Criteria

• The DBField class has a new Nice() method, with a strict return type of ?string
• The default implementation of DBField::Nice() returns $this->forTemplate()
• Subclasses of DBField which already implements a Nice() method respect the new strict return type
• The logic in GridFieldDataColumns::castValue() is updated to remove the nl2br and raw2xml function calls (i.e. remove sanitisation) - Nice() is expected to always be safe to use.
• The CMS 6 changelog calls out this change, and states that developers are responsible for making any implementations of the Nice() method they include in their own projects or modules output safe values.
• There is no regression of GridField escapes HTML content inappropriately #11191

PRs

• ENH Add Nice to DBField #11184

[GuySartorelli]

What would the method return by default?

[lekoala]

I would go with a non nullable string, since the goal is often to use it for display purpose

DBBoolean : yes/no
DBCurrency : number formatted + symbol
DBDate : formatted using local format
DBDecimal : number_format
DBInt : sprintf

This way, you can be sure you get a string value out of any DBField object

[GuySartorelli]

That makes sense for the return type, but what I meant was what should the method on DBField itself actually return? What should that value be?
It can be added as an abstract method in a major release, which might be a good idea. But if your intention is to add it in a minor release it can't be abstract.

[michalkleiner]

In a minor release it could throw an exception as that would mean it's called on something where the subclass doesn't implement the method.

[lekoala]

oh for the DBField instance?

I was going to propose

    /**
     * @return string
     */
    public function Nice()
    {
        return (string) $this->value;
    }

[GuySartorelli]

Hmm.

I think throwing an exception is effectively the same as just not adding the method in the first place - either way you can't call it on subclasses that don't implement it without an error being thrown. Presumably the main use case for this is in templates or in summary_fields config, etc, where you don't have the facility to check method_exists etc.

On the other hand I feel like returning just the raw value isn't really desirable behaviour for Nice() - that's not always (maybe not even usually) going to be a nice format.

I think for CMS 5 it might make sense to just implement it in concrete subclasses of DBField that don't have it implemented (if there are any), and add an abstract method fo DBField itself in CMS 6.
Does that sound appropriate?

[lekoala]

Not having the method in the base class is the main issue here: a lot of methods are typed against a DBField and therefore don't know if they can rely on the Nice method being present

eg:

silverstripe-framework/src/Forms/GridField/GridFieldDataColumns.php

Lines 263 to 269 in 5ab9ded

	if (method_exists($value, 'Nice')) {
	// If it has a "Nice" method, call that & make sure the result is safe
	$value = nl2br(Convert::raw2xml($value->Nice()) ?? '');
	} else {
	// Otherwise call forTemplate - the result of this should already be safe
	$value = $value->forTemplate();
	}

maybe the Nice method should be by default an alias of ->forTemplate since that's already how it's being used anyway. Then in subclasses it can be overridden accordingly.
that will prevent having to do method_exists checks and make phpstan happy :)

[GuySartorelli]

Yeah, that'd probably be okay. I can live with that.

[GuySartorelli]

Gonna keep this open and raise a CMS 6 PR to make the method abstract.

[GuySartorelli]

Actually nevermind, fortemplate is good enough. 😅

[GuySartorelli]

This work needed to be reverted - see #11191 for details.
Reopening so we can address this in CMS 6 - see newly added "Acceptance Criteria" in the issue description.