Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

TASK: Use generics via @template instead of PHPSTORM_META #3222

Merged
merged 5 commits into from
Nov 9, 2023

Conversation

mhsdesign
Copy link
Member

Since the php universe evolved im gonna try #2753 again ;)

Adds typings to:

\Neos\Flow\ObjectManagement\ObjectManagerInterface::get()

and

\Neos\Flow\Core\Bootstrap::getEarlyInstance()

by usage of the @template tag: https://phpstan.org/writing-php-code/phpdocs-basics#generics

This feature is supported by phpstorm, psalm and phpstan and also used widely in Neos 9

Upgrade instructions

Review instructions

Checklist

  • Code follows the PSR-2 coding style
  • Tests have been created, run and adjusted as needed
  • The PR is created against the lowest maintained branch
  • Reviewer - PR Title is brief but complete and starts with FEATURE|TASK|BUGFIX
  • Reviewer - The first section explains the change briefly for change-logs
  • Reviewer - Breaking Changes are marked with !!! and have upgrade-instructions

@mhsdesign
Copy link
Member Author

Im not sure i got all the places where generics make sense. But its a good start :D

Copy link
Member

@kdambekalns kdambekalns left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, why not…

Neos.Flow/Classes/Reflection/ReflectionService.php Outdated Show resolved Hide resolved
Copy link
Member

@bwaidelich bwaidelich left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice one, I like!

@mhsdesign mhsdesign force-pushed the task/objectMangerGenerics branch from 9548d29 to 433e6af Compare November 9, 2023 08:03
@mhsdesign mhsdesign requested a review from kdambekalns November 9, 2023 08:03
Copy link
Member

@mficzel mficzel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I generally like this a lot and probably am an explanation about virtual objects away from aproving.

Copy link
Member

@bwaidelich bwaidelich left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added suggestions to support virtual objects using the PHPStan notation for Conditional return types

See https://phpstan.org/r/1017a495-661c-4929-9f33-8f83065ac1f4

@mhsdesign
Copy link
Member Author

@bwaidelich i was about to say: good point with virtual objects but we should avoid that ^^ - but your solution seems quite clever! Question is if phpstorm/psalm also supports this or do we have to use @phpstan-return conditional stuff and @return T

@bwaidelich
Copy link
Member

Question is if phpstorm/psalm also supports this

I gave it a shot and it seems to work:
image

An additional @return object|null would do no harm though

@mhsdesign
Copy link
Member Author

mhsdesign commented Nov 9, 2023

See https://phpstan.org/r/1017a495-661c-4929-9f33-8f83065ac1f4

Fyi it has to be @return ($objectName is class-string<T> (not @return ($objectName is class-string ) otherwise phpstan doesnt work.

    /**
     * @template T of object
     * @param class-string<T>|string $objectName The object name
     * @return ($objectName is class-string<T> ? T : object) The object instance
     */

your previous code would always say object (also for actual passed classes) - will test this also in phpstorm ^^

@mhsdesign
Copy link
Member Author

mhsdesign commented Nov 9, 2023

Hmm @bwaidelich and @mficzel it seems that the proposed super magic cow powder syntax is not supported by phpstorm yet: (newGetInstance works but not crazyNewGetInstance)

/**
 * @template T of object
 * @param class-string<T> $objectName The object name
 * @return T The object instance
 */
public function newGetInstance($objectName)
{
    return $this->objects[$objectName]['i'] ?? throw new \Exception();
}

/**
 * @template T of object
 * @param class-string<T>|string $objectName The object name
 * @return ($objectName is class-string<T> ? T : object) The object instance
 */
public function crazyNewGetInstance($objectName)
{
    return $this->objects[$objectName]['i'] ?? throw new \Exception();
}

i would propose that people have to write this in the super rare cases, when they access an actual virtual object:

/** 
 * @var MyVirtualObject $virtual
 * @phpstan-ignore-next-line
 */
$virtual = $objectManager->get('My.Virtual');

…ject

> Parameter #1 $objectName of method ObjectManager::get() expects class-string<Some.Package:VirtualObject>, string given.

instead we will return `object`

Phpstorm doesnt support his conditional return type so we have a simple `@return T` in place which allows for autocompletion
@mhsdesign
Copy link
Member Author

With d4ed048 it will also work with virtual object by just returning object in those cases.

Copy link
Member

@bwaidelich bwaidelich left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Works like a charm, thanks for taking care!

PHPStan:

https://phpstan.org/r/795a1071-4d3f-43bc-b578-6b5c53fd116c

PHPStorm:

image

Note: For virtual objects, the IDE interprets the string as class name, but that's not worse than before I think:

image

Copy link
Member

@kitsunet kitsunet left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

very nice!

@kitsunet kitsunet merged commit 21bf506 into 9.0 Nov 9, 2023
5 checks passed
@kitsunet kitsunet deleted the task/objectMangerGenerics branch November 9, 2023 11:14
mhsdesign added a commit to neos/neos-development-collection that referenced this pull request Nov 9, 2023
neos-bot pushed a commit to neos/contentrepositoryregistry that referenced this pull request Nov 9, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants