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

DevKit updates for 1.x branch #468

Merged
merged 3 commits into from
Jan 5, 2022
Merged

DevKit updates for 1.x branch #468

merged 3 commits into from
Jan 5, 2022

Conversation

SonataCI
Copy link
Collaborator

@SonataCI SonataCI commented Jan 3, 2022

No description provided.

@jordisala1991
Copy link
Member

Maybe @phansys can review this changes, I do not know enough of this bundle to be sure if the changes make sense or not.

@VincentLanglet
Copy link
Member

Maybe @phansys can review this changes, I do not know enough of this bundle to be sure if the changes make sense or not.

It's not needed to understand how this library works. Personnaly I don't.

Priori to this change, the $entities property was an array of array or object.
And when needed the object were loaded.

But phpstan/psalm were complaining because it was requiring callable which support both array and object even if the callable were only use after object were loaded, so the callable was only used on object.

Instead of having both array and object in the same property, I used two different now.

is_array($entities[$key]) become isset($loadedEntities[$key]).

@jordisala1991
Copy link
Member

Maybe @phansys can review this changes, I do not know enough of this bundle to be sure if the changes make sense or not.

It's not needed to understand how this library works. Personnaly I don't.

Priori to this change, the $entities property was an array of array or object.

And when needed the object were loaded.

But phpstan/psalm were complaining because it was requiring callable which support both array and object even if the callable were only use after object were loaded, so the callable was only used on object.

Instead of having both array and object in the same property, I used two different now.

is_array($entities[$key]) become isset($loadedEntities[$key]).

Well, without this explanation it wasnt that easy to understand 😅.

Let me check the Pr again.

* Entity collection. If can be:
* - empty, if the collection has not been initialized yet
* - store entity
* - contain audited entity.
Copy link
Member

Choose a reason for hiding this comment

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

Maybe this comment is not right anymore? Both properties has the same , but you kinda splitted how the previous property works, right?

Copy link
Member

Choose a reason for hiding this comment

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

I tried to improve the comment

Copy link
Member

@jordisala1991 jordisala1991 left a comment

Choose a reason for hiding this comment

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

LGTM

@jordisala1991 jordisala1991 merged commit 645efc7 into 1.x Jan 5, 2022
@jordisala1991 jordisala1991 deleted the 1.x-dev-kit branch January 5, 2022 22:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants