Skip to content

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 requested review from Korbeil and joelwurtz May 14, 2024 06:59
@nikophil nikophil force-pushed the fix/private-property branch 3 times, most recently from 5a0b579 to c15bf93 Compare May 14, 2024 07:15
public function testMapToClassWithPrivateProperty(): void
{
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
@nikophil nikophil force-pushed the fix/private-property branch from 27f48f7 to 8afc811 Compare June 5, 2024 09:47
@joelwurtz joelwurtz merged commit 20c3beb into jolicode:main Jun 5, 2024
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