-
-
Notifications
You must be signed in to change notification settings - Fork 9.6k
[PropertyAccess] Use ArrayAdapter in debug mode #21936
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
Conversation
58cfdfd
to
fed148b
Compare
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.
👍 ping @dunglas
@@ -1254,16 +1255,24 @@ private function registerCacheConfiguration(array $config, ContainerBuilder $con | |||
} | |||
|
|||
if (method_exists(PropertyAccessor::class, 'createCache')) { | |||
$propertyAccessDefinition = $container->register('cache.property_access', AdapterInterface::class); | |||
$propertyAccessDefinition = $container->register('cache.property_access'); |
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.
keep this line
fed148b
to
e6a6de4
Compare
$propertyAccessDefinition->setArguments(array(null, null, $version, new Reference('logger', ContainerInterface::IGNORE_ON_INVALID_REFERENCE))); | ||
} else { | ||
$propertyAccessDefinition->setClass(ArrayAdapter::class); | ||
$propertyAccessDefinition->setArguments(array(0)); |
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.
why this line?
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.
in fact, can't you add false as second arg? (to store unserialized)
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.
in fact, can't we add false
as second arg? (to store unserialized)
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.
Removing this line gives:
Symfony\Component\DependencyInjection\Exception\OutOfBoundsException: Cannot replace arguments if none have been configured yet.
symfony/symfony/src/Symfony/Bundle/FrameworkBundle/DependencyInjection/Compiler/CachePoolPass.php:74
CachePoolPass.php:74
tries to replace arguments that are not set yet, for ArrayAdapter at least. Should it be fixed?
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.
false
added as 2nd arg, works as well
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.
In fact, the ArrayAdapter should not have the cache.pool tag!
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.
indeed, fixed
3def569
to
63f5e75
Compare
63f5e75
to
dca3f36
Compare
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.
👍
Thank you @chalasr. |
This PR was merged into the 3.2 branch. Discussion ---------- [PropertyAccess] Use ArrayAdapter in debug mode | Q | A | ------------- | --- | Branch? | 3.2 | Bug fix? | yes | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | #21894 | License | MIT | Doc PR | n/a https://github.com/symfony/symfony/pull/21936/files?w=1 Commits ------- dca3f36 [PropertyAccess] Use ArrayAdapter in debug mode
Was there any profiling done to assess the performance impact of this change? (As opposed to stale checking.) |
Are you experiencing any issue? Please open new ticket, comments on closed can't be tracked. |
https://github.com/symfony/symfony/pull/21936/files?w=1