New BC-safe API to support CMSMain refactoring

[GuySartorelli]

DO NOT SQUASH. There are two commits here:

1. Deprecate DBEnum::flushCache() in favour of DBEnum::reset()
2. Introduce the new DataObject.class_description config and its associated methods (replacing SiteTree.description)

The CMS 6 PRs have additional context for these changes if you need it.

Issue

• Don't hardcode SiteTree in CMSMain silverstripe-cms#2947

[GuySartorelli]

Using PHPDoc for this and i18n_classDescription()'s return types for BC. Since these methods already exist on SiteTree without strict return types, we can't add them here.

[GuySartorelli]

BC protection for custom classes where devs have added localisations for the description. Will be removed in 6.

[GuySartorelli]

Replacing $this->singular_name() with $singularName is unrelated tidyup, since that variable is defined above.

Adding CLASS_DESCRIPTION here ensures text collector knows how to collect the value for this key.

[emteknetnz]

This variable usage is weird, can't we just using an empty string default e.g. _t(static::class . '.CLASS_DESCRIPTION', ''); and then test to see if it's truthy?

[GuySartorelli]

Unfortunately not, because _t() does a falsy check against $default and throws an error

silverstripe-framework/src/i18n/i18n.php

Lines 169 to 171 in 1f2ae2a

	if (!$default && i18n::config()->uninherited('missing_default_warning')) {
	user_error("Missing default for localisation key $entity", E_USER_WARNING);
	}

[emteknetnz]

OK. $placeholder is a little confusing since it's being used to test there's no value, rather than as an actual placeholder.

Change it to $notDefined == 'NOT_DEFINED'; so that we can go if ($baseDescription === $notDefined) { later on

[GuySartorelli]

Done

[emteknetnz]

Suggested change

	Deprecation::notice(
	'5.4.0',
	'The ' . static::class . '.DESCRIPTION' . ' i18n localisation key is deprecated. Use '
	. static::class . '.CLASS_DESCRIPTION' . ' instead.',
	Deprecation::SCOPE_GLOBAL
	);
	$descKey = static::class . '.DESCRIPTION';
	$classDescKey = static::class . '.CLASS_DESCRIPTION';
	Deprecation::notice(
	'5.4.0',
	"The $deskKey i18n localisation key is deprecated. Use $classDescKey instead."
	);

Should use SCOPE_CLASS (the default) not SCOPE_GLOBAL?

[GuySartorelli]

The class isn't deprecated, so SCOPE_CLASS isn't appropriate. SCOPE_CLASS would mean the deprecation notice says something like "DataObject is deprecated" which just isn't true.

[GuySartorelli]

Also SCOPE_METHOD is the default - and is also not appropriate because the method isn't deprecated either.

[emteknetnz]

Right. I don't think we should be deprecating translation keys even though this is a high use area. I don't think we've ever done this before as we'll simply stop using old translation keys. It seems like its setting a new precedent/policy which I don't think needs to be there.

The new keys are just duplicates of the old keys, so the only risk of something going wrong is that any new translation updates won't be received.

If you really want to keep this, then you'll need to update the CMS 6 PR to throw exceptions if the old keys are used otherwise there seems like there is very little point in doing this

[GuySartorelli]

We don't throw exceptions when things are deprecated. We use deprecation notices. We clearly need to discuss this in person lol it seems like this is being blown out of proportion a bit.

Throwing an exception because of something that - as you've pointed out - is low risk is a really bad idea. We should instead log a warning. A warning that says to stop using the old thing. At that point we're just reinventing deprecation notices.

I don't think we've ever done this before as we'll simply stop using old translation keys.

We're going to stop using the old one and start using something else. I would like people to know when they're trying to use an old localisation key, so that they can update to use the new one.

Would you prefer if I emit a user_error() instead and remove the word "deprecation"?

[emteknetnz]

Agree offline to keep the fallback though remove the deprecation notices

[GuySartorelli]

Done

[emteknetnz]

Suggested change

	public function i18n_classDescription()
	public function getI18nClassDescription()

[emteknetnz]

This is mixing snake_case AND camelCase :-)

We shouldn't be adding new API that's not following modern PHP coding standards

[GuySartorelli]

I don't want to change these names. I'm not adding new API, I'm just moving it up the class hierarchy. Renaming the methods which exist on SiteTree is not the intention here.

getI18nClassDescription is especially bad since it's then not going to pop up with I go $this->i18n to find the i18n methods like i18n_pluralise(). It should explicitly and intentionally start with i18n - or we should rename the other i18n methods which is not in scope for this PR.

[emteknetnz]

OK