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.

Sign up for GitHub

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

Jump to bottom

refactor: fix proxy system and introduce psalm extension #704

Merged
merged 2 commits into from
Oct 22, 2024
Merged

refactor: fix proxy system and introduce psalm extension #704

merged 2 commits into from
Oct 22, 2024

Conversation

nikophil
Copy link
Member

@nikophil nikophil commented Oct 18, 2024 •
edited
Loading

fixes #701 #696

I've decided to completely remove all methods in PersistenceProxyObjectFactory, so we do not override @final methods anymore. We still need to figure out a solution for PersistenceObjectFactory::create(), I'll do this in a further PR.

Psalm does not understand the following:

/**
 * @extends PersistentProxyObjectFactory<User>
 */
class UserFactory extends PersistentProxyObjectFactory{}

/**
 * @template T of object
 * @extends PersistentObjectFactory<T&Proxy<T>>
 */
abstract class PersistentProxyObjectFactory extends PersistentObjectFactory{}

/**
 * @template T of object
 */
abstract class PersistentObjectFactory{}

it always consider the template to be User and not User&Proxy<User>. I think because we're breaking covariance (T&Proxy<T> is wider than T)

I've finally decided to introduce a psalm extension, we will be able to fix nearly every typing problem in here.

This was referenced Oct 18, 2024
Closed
Closed
@nikophil nikophil force-pushed the refactor/fix-proxy-system branch 3 times, most recently from e3a9d51 to cff5ae5 Compare October 19, 2024 15:28
@nikophil nikophil changed the title refactor: fix proxy system refactor: fix proxy system and introduce psalm extension Oct 19, 2024
kbond
kbond approved these changes Oct 19, 2024
View reviewed changes
Copy link
Member

@kbond kbond left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Looks good! I'm unfamiliar with psalm plugins, will this be auto-registered in projects using foundry and psalm?

@nikophil
Copy link
Member Author

I'm unfamiliar with psalm plugins, will this be auto-registered in projects using foundry and psalm?

yes, thanks to the change in composer.json

deluxetom
deluxetom approved these changes Oct 21, 2024
View reviewed changes
Copy link

@deluxetom deluxetom left a comment

Choose a reason for hiding this comment

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

Great solution, thank you!

nikophil
nikophil commented Oct 22, 2024
View reviewed changes
src/Factory.php
@@ -25,17 +25,18 @@
*/
abstract class Factory
{
/** @var Attributes[] */
/** @phpstan-var Attributes[] */
Copy link
Member Author

@nikophil nikophil Oct 22, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

everytime we use phpstan pseudo types, we should use @phpstan- annotations because Psalm seems to not understand it

and moreover, see this comment from stof here, I think it is relevant

I'll make another PR where I fix all of the related docblocks

nikophil
nikophil commented Oct 22, 2024
View reviewed changes
src/ObjectFactory.php
@@ -58,6 +56,7 @@ public function create(callable|array $attributes = []): object

$parameters = $this->normalizeParameters($parameters);
$instantiator = $this->instantiator ?? Configuration::instance()->instantiator;
/** @var T $object */
Copy link
Member Author

Choose a reason for hiding this comment

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

it seems like PHPStorm does not only interprets methods signature (like PHPStan would do) but it events guesses stuff from the methods' body. I had really hard times to find but it basically fixes 90% of auto-complete problems 🎉

nikophil
nikophil commented Oct 22, 2024
View reviewed changes
src/Persistence/PersistentObjectFactory.php
* @return T
*
* @throws \RuntimeException If no objects exist
*/
public static function first(string $sortBy = 'id'): object
{
return static::repository()->firstOrFail($sortBy);
/** @var T $object */
Copy link
Member Author

Choose a reason for hiding this comment

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

same, this is here to help auto-complete

@nikophil nikophil merged commit 0d570cc into 2.x Oct 22, 2024
@nikophil nikophil deleted the refactor/fix-proxy-system branch October 22, 2024 17:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@deluxetom deluxetom deluxetom approved these changes

@kbond kbond kbond approved these changes

Assignees
No one assigned
Labels
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Psalm errors with Attributes phpstan-type declaration
3 participants
@nikophil @kbond @deluxetom