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

fix: constructor with default values as objects #188

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

MrMeshok
Copy link
Contributor

While working on #187 I noticed what you can't map to a class that has constructor with default values as objects if source missing these arguments.
Seems like a complex issue to fix, so I'll start with the tests

@MrMeshok MrMeshok marked this pull request as draft September 13, 2024 14:44
@joelwurtz
Copy link
Member

Indeed, this will be hard to fix, there is 3 possible things to do :

  • disallow this (throw an exception if it happens so we dont let user in weird state)
  • allow this only if value can be cloned (but we will have new issue with sub property that have object and may be not cloned)
  • having a way to parse this code and reproduce it when generating (reproducing is easy, parsing i don't have a way in mind right now but should be feasible)

wdyt @Korbeil @nikophil ?

@MrMeshok
Copy link
Contributor Author

Actually, there is a simple way.
Instead of this:

$constructorarg_1 = $source->author ?? Author::__set_state();

$item = new Post(author: $constructorarg_1)

It should be this:

$constructorargs = [];
if (isset($source->author)) {
    $constructorargs['author'] = $source->author;
}

$item = new Post(...$constructorargs)

@MrMeshok
Copy link
Contributor Author

@Korbeil Sorry for unrelated question, I am just not sure how to reach out.
I was wondering when is the next release? I need some fixes from main, but don't really want to lower minimum stability in composer and use commit/branch

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.

2 participants