-
Notifications
You must be signed in to change notification settings - Fork 15
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
Conversation
5a0b579
to
c15bf93
Compare
{ | ||
static::bootKernel(); | ||
static::bootKernel(['additionalConfigFile' => __DIR__ . '/Resources/config/with-private-properties.yml']); |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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 ?
There was a problem hiding this comment.
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...
There was a problem hiding this comment.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
c15bf93
to
a86eb4d
Compare
|
||
class ServiceInstantiationTest extends WebTestCase | ||
{ | ||
protected function setUp(): void | ||
public static function setUpBeforeClass(): void |
There was a problem hiding this comment.
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
7fe4191
to
27f48f7
Compare
fixes problem raised [here](#129 (comment))
27f48f7
to
8afc811
Compare
configuration
map_private_properties
was not used to configure serviceautomapper.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