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(bundle): use map_private_properties when configuring ReflectionExtractor #129

Merged
merged 1 commit into from
Jun 5, 2024

Conversation

nikophil
Copy link
Collaborator

@nikophil nikophil commented May 14, 2024

configuration map_private_properties was not used to configure service automapper.property_info.reflection_extractor whereas without the bundle, \AutoMapper\Configuration::$mapPrivateProperties is actually used, this PR fixes this.

I had to introduce a way to have several configurations available for the fixture bundle, I'm using KernelTestCase::createKernel() + an additional config file path for this.

waiting for #137

@nikophil nikophil force-pushed the fix/private-property branch 3 times, most recently from 5a0b579 to c15bf93 Compare May 14, 2024 07:15
{
static::bootKernel();
static::bootKernel(['additionalConfigFile' => __DIR__ . '/Resources/config/with-private-properties.yml']);
Copy link
Collaborator Author

@nikophil nikophil May 14, 2024

Choose a reason for hiding this comment

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

when making ReflectionExtractor aware of map_private_properties, it has to be set to true for this test to pass.

but it seems wrong to me: even if the property is private, it could be written, because it is in the constructor:

class ClassWithPrivateProperty
{
    public function __construct(
        private string $foo
    ) {
    }
}

WDYT?

Copy link
Member

Choose a reason for hiding this comment

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

⚠️ We need to fix this behavior before merging this pull request !

Copy link
Member

Choose a reason for hiding this comment

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

It depends, it only work if we write to it, but not if we read from it

Copy link
Collaborator Author

@nikophil nikophil May 15, 2024

Choose a reason for hiding this comment

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

it only work if we write to it, but not if we read from it

it is indeed IMO the expected behavior, with map_private_properties: false, but that's not what happens

Copy link
Member

Choose a reason for hiding this comment

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

Yes, but not sure we can resolve this easily on our side ?

Does this bug happens because we cannot found the property (not listed) or because we don't find a mutator for it ?

AFAIK, this would require having a different service when we try to get the write mutator on constructor, so we could configure that one to work with private properties

Or do a bug fix in symfony ?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

not sure we can resolve this easily on our side ?

it seems to be a pretty complex problem indeed

Does this bug happens because we cannot found the property (not listed) or because we don't find a mutator for it ?

my understanding of the problem is that we're looping over $this->propertyInfoExtractor->getProperties($target) and it does not return the private property. Then we don't even try to find a mutator.

this would require having a different service when we try to get the write mutator on constructor, so we could configure that one to work with private properties

I got the same feeling: in a "from target" or "source target" mode, we need to investigate a little bit more around constructor's parameters. Or maybe detect properties in a different way: loop over all properties (including private ones) and only keep the ones for which we can find a PropertyWriteInfo thanks to PropertyWriteInfoExtractorInterface::getWriteInfo(). But due to how this is made in property info, this would require two instances of ReflectionExtractor: one which can always access private properties, and another one which is configured with map_private_properties

Or do a bug fix in symfony ?

it seems to me that property info is working correctly here, at least, it does what it is expected to do 😅
Maybe a nice improvement to the property info component would be to leverage the $context argument and allow to override the access flag from there...

Copy link
Collaborator Author

@nikophil nikophil May 15, 2024

Choose a reason for hiding this comment

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

ok, forget about that all :)

I've found a simpler solution

I've roll-backed the current change, and the tests will pass once #137 is merged

Copy link
Member

@Korbeil Korbeil left a comment

Choose a reason for hiding this comment

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

👍


class ServiceInstantiationTest extends WebTestCase
{
protected function setUp(): void
public static function setUpBeforeClass(): void
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

would you mind this change? on my computer it greatly improves test performances:

  • before: Time: 01:53.571
  • after: Time: 01:11.453

@nikophil nikophil force-pushed the fix/private-property branch 2 times, most recently from 7fe4191 to 27f48f7 Compare June 4, 2024 06:25
joelwurtz added a commit that referenced this pull request Jun 5, 2024
@joelwurtz joelwurtz merged commit 20c3beb into jolicode:main Jun 5, 2024
5 checks passed
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