API Deprecate getDescription method to prevent it overriding db fields

[michalkleiner]

Complimentary PR to #785

Issue

• Remove the getDescription method from BaseElement #1106

[GuySartorelli]

Since getDescription is still available IMO it should still be called here. Developers will be used to their descriptions being pulled through for the TypeNice and may rely on it.

[michalkleiner]

It'll be deprecated, so it shouldn't be used going forward.
If people rely on it, it will work until it's removed in 5.0.
The change here is a duplicate of what's in the method without any functional difference and is included here as agreed in the original PR.

[GuySartorelli]

It's a duplicate for the method as declared in this base class, but if someone has overridden the getDescription method in some subclass and are expecting the returned result of their overridden method to be used in TypeNice then this will represent a breaking change for them.

I don't know how common that is and whether it's a real cause of concern, just something to consider.

[michalkleiner]

Yep, that's fair, though when they find that their custom method is not working anymore after upgrading, they will see the deprecation and adjust their code.

What do you think @emteknetnz, should I keep the code change or remove it and leave just the deprecation?

[emteknetnz]

Perhaps update getDescription() in this method to simply return '';

Retain $description = $this->getDescription(); in this method, and then add a conditional if ($description === '') ... then $description = $this->config()->uninherited('description'); etc

That way if any projects have overridden getDescription() it will continue to be used for TypeNice so no BC breakage

I'm not sure how annoying the deprecation warning will be. I'm also not sure if it appears on a subclassed element that implements getDescription(). We do want the method deprecated, though at the same time we don't really want it annoying a developer on their local box with deprecated warning on since elemental 4 is likely to be around for a long time

[michalkleiner]

Yep, I got the suggestion.
My counterpoint was that even with that, if custom project code overrides the getDescription method to return an empty string instead of getting the config value, the suggested way would trump that and the custom code would have to go and override the getTypeNice method instead.

[michalkleiner]

Since the type hint is only in the doc and not defined as the return type by PHP, can we extend it to return null from getDescription and check for null instead? Custom code returned empty string would still work.

[michalkleiner]

    /**
    * @deprecated
    * @return string|null
    */
    public function getDescrption()
    {
        return null;
    }

    public function getTypeNice()
    {
        $description = $this->getDescription(); // allow a subclass to override the old function
        if ($description === null) {
            $description = $this->config()->uninherited('description');        
            if ($description) {
                 $description = _t(__CLASS__ . '.Description', $description);
            }
        }
        $desc = ($description) ? ' <span class="element__note"> &mdash; ' . $description . '</span>' : '';
    }

[emteknetnz]

OK right, the use case being the custom code specifically wants a blank description

Yeah I suppose it's fine, if you wanted to be particularly strict (it does save on arguments about semver, and sometimes weird edge cases), perhaps something like

/** @var string */
private $fallbackToConfig = '__fallback_to_config__';

/** @return string */
public function getDescription()
{
    reutrn $this->fallbackToConfig;
}

[GuySartorelli]

I still think we need to retain the following two use cases for BC:

1. Developers are calling getDescription() directly, or are using $Description in their templates
   • The output of this needs to not be changed by this PR.
2. Developers are overriding getDescription() in their subclass and expecting their overridden content to be used.
   • Their content must be used in all the same situations where it was used before.

Yes developers can update their code the match the changes - but that's no different from just deleting this method outright. The point of deprecating the method is so that developers can make changes when they have capacity to do so - they should be able to update without making code changes, and then make the code changes as a dedicated intentional (and crucially, separate) effort.

[GuySartorelli]

@michalkleiner I've added a comment to this - sorry it's gone so long without being touched. Are you still interested in getting this done? If so please make changes so that the use cases I've outlined are respected in a BC way.

[michalkleiner]

I'll see this week if I can push it forward. If not I'll close this.

[GuySartorelli]

I was going to co-opt the PR and make the requested changes myself but I don't have permission to edit the PR.

[michalkleiner]

Can a PR be retrospectively adjusted to allow maintainers push?

[GuySartorelli]

I don't know for sure, but I don't think so unfortunately.
If you won't get time to work on this I'm happy to open a new pr for it though.

[michalkleiner]

I looked at the PR and tbh I'm lost on what was going on and what is needed to get it over the line, so will just close it for now. If you can open a new one with what's needed @GuySartorelli that would be appreciated. Thanks!

[GuySartorelli]

Replaced by #1196