Implemented Developer feature: Improve entity creation process

[Xon]

Creating various entities is a fairly common task, and requires manual fixing of type hinting.

Code like this is very common.

/** @var \XF\Entity\Phrase $phrase */
$phrase = $this->_em->create('XF:Phrase');

or

$phrase = $this->_em->create('XF:Phrase');
assert($phrase instanceof \XF\Entity\Phrase );

In either case, a stringy identifier is being used and the full type is repeated again before/after. phpstorm's metadata can sometimes help, but not in all cases.

There is however a better way, but requires some tweaks to the EntityManager::create function call chain.

Suppose there was a static function \XF\Entity\Phrase::create() that returned the type static. Type analyzers will expand this to the called class name, and in php 8.0+ the static return type does the same thing.

A static method could be added to the \XF\MVC\Entity\Entity base class such as:

/**
 * @return static
 * @noinspection PhpMissingReturnTypeInspection
 * @noinspection PhpIncompatibleReturnTypeInspection
 * @noinspection PhpReturnDocTypeMismatchInspection
 */
public static function create()
{
    $app = \XF::app();
    static $rootClass = null;
    $fullClassName = $rootClass ?? $app->extension()->resolveExtendedClassToRoot(static::class);

    return $app->em()->create($fullClassName);
}

EntityManager::getEntityClassName function would need to handle the full class name, not just the short class name. But this also means you can then do \XF::em()->create(\XF\Entity\Phrase::class); and no longer need to use the short-name concept which involves stringy pseudo classname strings.

 

[Kirby]

I am pretty sure I am missing smth. (esp. as it is already late), but I don't understand the purpose of this suggestion (yet).

But this also means you can then do \XF::em()->create(\XF\Entity\Phrase::class)

This is already possible but doesn't help for type hinting?

 

[Xon]

I am pretty sure I am missing smth. (esp. as it is already late), but I don't understand the purpose of this suggestion (yet).

It has two things:

• Stop requiring stringy-typed mutated classnames which the overall php ecosystem doesn't understand.
• Allow automatic typehinting of the creator function via the php return type (v8.0+ only) and/or phpdocs (any supported version of php) that the wider php ecosystem understands and supports.

This is already possible but doesn't help for type hinting?

The getStructure and related code will run multiple times, as the two strings map to different cache keys when the following code runs:

$foo = \XF::em()->create(\XF\Entity\Phrase::class);
$bar = \XF::em()->create('XF:Phrase');

At least it no longer causes a class redefining error like it did in XF2.x

 

[RisteDimitrievski]

I disagree completely with this idea, as creating entities like this

\XF::em()->create('addonid\XF:Entity');
or
    $this->em()->create('addonis\XF:Entity');

Is the best way and easiest to create entity.

 

[Kirby]

• Allow automatic typehinting of the creator function via the php return type (v8.0+ only) and/or phpdocs (any supported version of php) that the wider php ecosystem understands and supports.

I still don't understand how this would work

Right now, \XF\Mvc\Entity\Manager::create doesn't have a return type at all.

How can this be changed so that the correct return type (e.g an instance of \XF\Entity\Phrase) can be detected automatically when calling \XF::em()->create(\XF\Entity\Phrase::class)?

The getStructure and related code will run multiple times, as the two strings map to different cache keys when the following code runs:

$foo = \XF::em()->create(\XF\Entity\Phrase::class);
$bar = \XF::em()->create('XF:Phrase');

Hmm ... I don't see Phrase::getStructue being called multiple times for this example code, nor do I get class redefining errors with XF 2.2.10 / PHP 8.0?

 

[Xon]

I still don't understand how this would work

Right now, \XF\Mvc\Entity\Manager::create doesn't have a return type at all.

The type hint @return static is what does it. This tells static type analyse tools that the return type is of the type of the called class at call-time.

php 8 introduces this explicitly as a return type.

How can this be changed so that the correct return type (e.g an instance of \XF\Entity\Phrase) can be detected automatically when calling \XF::em()->create(\XF\Entity\Phrase::class)?

Generics in PHP using PHPDocs

phpstan.org

PhpStorm 2021.2: Generics, Enums, Array Shapes, Inspections, Refactorings, and More | The PhpStorm Blog

PhpStorm 2021.2 is now available! This major release introduces preliminary support for generics in PHP, enums for PHP 8.1, one-line array shapes, improved automatic formatting of PHP code, new ins

blog.jetbrains.com

/**
 * @template T
 * @param class-string<T> $classname
 * @return T
 */
function create($classname);

Various static type analyse tools will now infer the return type matches what is provided in $classname.

Hmm ... I don't see Phrase::getStructue being called multiple times for this example code, nor do I get class redefining errors with XF 2.2.10 / PHP 8.0?

I know repositories don't work with this pattern, I might need to trace the entity creation again to see if it works better than it used to.

 

[PaulB]

Is the best way and easiest to create entity.

Why? You lose out on a lot of IDE and static analysis capabilities.

 

[PaulB]

How can this be changed so that the correct return type (e.g an instance of \XF\Entity\Phrase) can be detected automatically when calling \XF::em()->create(\XF\Entity\Phrase::class)?

@template T of self
@param class-string<T> $className
@return T

 

[Kirby]

@template T of self
@param class-string<T> $className
@return T

Hmm ...

If method \XF\Mvc\Entity\Manager::create was annoted as suggested, wouldn't the template only accept instances of \XF\Mvc\Entity\Manager?

If it was instead annoted as

/**
 * @template T
 * @param class-string<T> $classname
 * @return T
 */

(like posted by @Xon), static code analyzers could infer type \XF\Entity\Phrase when calling \XF::em()->create(\XF\Entity\Phrase::class).

However (at least to my understanding) they could not infer the actual class (like \MyVendorPrefix\MyAddonId\XF\Entity\Phrase)?

 

[Xon]

However (at least to my understanding) they could not infer the actual class (like \MyVendorPrefix\MyAddonId\XF\Entity\Phrase)?

Essentially yeah. What I (and some others do) is immediately do assert($phrase instanceof \MyVendorPrefix\MyAddonId\XF\Entity\Phrase); to instruct static code analyzers what the correct type is after the assert statement.

 

[PaulB]

If method \XF\Mvc\Entity\Manager::create was annoted as suggested, wouldn't the template only accept instances of \XF\Mvc\Entity\Manager?

No; the of self constraint indicates that T must be self or a subclass thereof. Type constraints with template parameters would be useless otherwise.