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 issue with toArray #80

Merged
merged 1 commit into from
Jul 15, 2024
Merged

Fix issue with toArray #80

merged 1 commit into from
Jul 15, 2024

Conversation

WendellAdriel
Copy link
Owner

This fixes the issue described in #78
When using toArray (or any other method to transform the DTO into another structure), there was an inconsistency with the data from the DTO property and the resulting structure.

This PR introduces a fix and a test case to cover the scenario.

@WendellAdriel WendellAdriel merged commit 93721ce into main Jul 15, 2024
16 checks passed
@WendellAdriel WendellAdriel deleted the hotfix/to-array branch July 15, 2024 10:40
@xHeaven
Copy link

xHeaven commented Oct 10, 2024

Hey @WendellAdriel!

We just realized at the team that this fix broke something else.

Lets pretend we have this DTO:

class TestDTO extends SimpleDTO
{
    #[Map(data: 'data')]
    public array $foobar;

    protected function defaults(): array
    {
        return [];
    }

    protected function casts(): array
    {
        return [];
    }
}

Lets use this DTO like this:

TestDTO::fromArray(['data' => ['12345']]);

In 3.6.0, this code outputs this:

App\TestDTO {
    +foobar: [
      "12345",
    ],
}

However, after 3.6.1, the output is this:

App\TestDTO {
    +foobar: [],
}

This is a quite breaking change, as we can't really tell random API providers to not provide a key called data. We also wouldn't like to manually map the data key into something else, I mean that's the entire point of having a DTO that does this for us in the first place.

Can we get this fixed, or is there a workaround that doesn't involve manual mapping?

Thanks!

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