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

Handle PHP8.2 readonly classes #718

Merged

Conversation

Korbeil
Copy link
Member

@Korbeil Korbeil commented Apr 10, 2023

I'm not sure we should throw an exception here, the current behavior seems fine to me: when using a readonly class as object to populate, it's simply ignored.

Here is a sample mapper from the test I added in this PR

    public function &map($value, array $context = array())
    {
        if (null === $value) {
            return $value;
        }
        $result = $context['target_to_populate'] ?? null;
        if (null === $result) {
            if (\Jane\Component\AutoMapper\MapperContext::hasConstructorArgument($context, 'Jane\\Component\\AutoMapper\\Tests\\Fixtures\\AddressDTOReadonlyClass', 'city')) {
                $constructarg = \Jane\Component\AutoMapper\MapperContext::getConstructorArgument($context, 'Jane\\Component\\AutoMapper\\Tests\\Fixtures\\AddressDTOReadonlyClass', 'city');
            } else {
                $constructarg = $value['city'];
            }
            $result = new \Jane\Component\AutoMapper\Tests\Fixtures\AddressDTOReadonlyClass($constructarg);
        }
        return $result;
    }

@nikophil any feedback here ?

@Korbeil Korbeil self-assigned this Apr 10, 2023
@nikophil
Copy link
Contributor

nikophil commented Apr 10, 2023

I don't have a strong opinion on this, but I'm a little bit more in favor of throwing an exception in order to prevent some WTF moment for user which would not understand directly why the data is not mapped

@damienalexandre
Copy link
Contributor

Agreed, having a RuntimeException is better for DX in my opinion. The Exception should educate about the readonly status and about alternatives.

@Korbeil
Copy link
Member Author

Korbeil commented Apr 11, 2023

After thinking a bit more about this, I'll make it an exception by default and add an opt-out to disable it if required.
It could happen that someone is using some library that has a read-only class that you can't really change and I don't want him to be blocked by this in this case.

@Korbeil Korbeil force-pushed the fix/exception-readonly-classes-to-populate branch 4 times, most recently from ebccd27 to 9886b04 Compare April 12, 2023 12:06
@Korbeil
Copy link
Member Author

Korbeil commented Apr 12, 2023

Here is a sample generated Mapper after this changes:

final class Mapper_array_Jane_Component_AutoMapper_Tests_Fixtures_AddressDTOReadonlyClass extends \Jane\Component\AutoMapper\GeneratedMapper
{
    public function __construct()
    {
    }
    public function &map($value, array $context = array())
    {
        if (null === $value) {
            return $value;
        }
        $result = $context['target_to_populate'] ?? null;
        if (!($context['allow_readonly_target_to_populate'] ?? false) && is_object($context['target_to_populate'] ?? null)) {
            throw new Jane\Component\AutoMapper\Exception\ReadOnlyTargetException();
        }
        if (null === $result) {
            if (\Jane\Component\AutoMapper\MapperContext::hasConstructorArgument($context, 'Jane\\Component\\AutoMapper\\Tests\\Fixtures\\AddressDTOReadonlyClass', 'city')) {
                $constructarg = \Jane\Component\AutoMapper\MapperContext::getConstructorArgument($context, 'Jane\\Component\\AutoMapper\\Tests\\Fixtures\\AddressDTOReadonlyClass', 'city');
            } else {
                $constructarg = $value['city'];
            }
            $result = new \Jane\Component\AutoMapper\Tests\Fixtures\AddressDTOReadonlyClass($constructarg);
        }
        return $result;
    }
}

There is two levels where you can setup this check:

  • in the Generator class, you have an opt-out that adds a check if the target class is a readonly class which throw a ReadOnlyTargetException
  • in the context during runtime, you can add allow_readonly_target_to_populate field to opt-out this validation

@Korbeil Korbeil force-pushed the fix/exception-readonly-classes-to-populate branch from 9886b04 to 227fe2b Compare April 12, 2023 12:30
Copy link
Contributor

@nikophil nikophil left a comment

Choose a reason for hiding this comment

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

😎

@Korbeil Korbeil force-pushed the fix/exception-readonly-classes-to-populate branch from 227fe2b to c1648bf Compare April 14, 2023 07:58
@Korbeil Korbeil force-pushed the fix/exception-readonly-classes-to-populate branch from c1648bf to f288c36 Compare April 14, 2023 08:00
@Korbeil Korbeil merged commit 0339d65 into janephp:next Apr 14, 2023
8 checks passed
@Korbeil Korbeil deleted the fix/exception-readonly-classes-to-populate branch April 14, 2023 08:08
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.

None yet

3 participants