Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly
Appearance settings

Property access reuse #3488

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 4 commits into from
Mar 10, 2016
Merged

Property access reuse #3488

merged 4 commits into from
Mar 10, 2016

Conversation

Koc
Copy link
Contributor

@Koc Koc commented Dec 12, 2015

Reuse existing property access instance for performance improvement.

Reasons:

  1. starting from Symfony v2.3.35 there is internal cache for this component (cache inside request) symfony/symfony@284dc75 . Cache working using class property and we should use same instance to get it works properly.
  2. there is unmerged yet PR that adds cache outside request [PropertyAccess] Add PSR-6 cache symfony/symfony#16838 . For getting it working we should use configured instance with passed cache layer inside it instead of creating new one.
  3. in two classes you instanciate property accessor inside foreach.

Koc
Koc reviewed Dec 12, 2015
View reviewed changes
Admin/Admin.php
@@ -945,7 +945,7 @@ protected function buildForm()
if ($this->isChild() && $this->getParentAssociationMapping()) {
$parent = $this->getParent()->getObject($this->request->get($this->getParent()->getIdParameter()));

$propertyAccessor = PropertyAccess::createPropertyAccessor();
$propertyAccessor = $this->getConfigurationPool()->getContainer()->get('property_access');
Copy link
Contributor Author

Choose a reason for hiding this comment

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

We can replace it with setter, but who should call this setter?

@Koc Koc force-pushed the property-access-reuse branch 4 times, most recently from 9c08fa9 to 66a0787 Compare December 14, 2015 19:18
@Koc Koc force-pushed the property-access-reuse branch 3 times, most recently from 1f330c0 to de11ed6 Compare January 8, 2016 16:33
@Koc Koc changed the title [WIP] Property access reuse Property access reuse Jan 8, 2016
@Koc Koc force-pushed the property-access-reuse branch from de11ed6 to 759becc Compare January 8, 2016 16:51
@Koc
Copy link
Contributor Author

Koc commented Jan 8, 2016

Looks like tests are finally pass. Please review.

@Koc
Copy link
Contributor Author

Koc commented Jan 28, 2016

@rande @OskarStark @soullivaneuh can anybody says anything about this PR?

@OskarStark
Copy link
Member

Status: Reviewed

Looks good to me in general, but I'm not very familar with this.

@core23 can you please have a look?

@rande I'm 👍 for merging this

@Koc thank you for your work!

@OskarStark OskarStark self-assigned this Jan 29, 2016
@core23
Copy link
Member

core23 commented Jan 29, 2016

Looks okay to me, but I am not that deep in the caching stuff.

Think @rande should have a final review.

@OskarStark
Copy link
Member

Thank you @core23

@Koc
Copy link
Contributor Author

Koc commented Mar 2, 2016

So does anything block this?

OskarStark
OskarStark reviewed Mar 9, 2016
View reviewed changes
Admin/Pool.php
@@ -23,7 +25,7 @@ class Pool
/**
* @var ContainerInterface|null
Copy link
Member

Choose a reason for hiding this comment

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

php doc wrong, if not null

@OskarStark
Copy link
Member

@Koc after my comments i will merge this

@Koc Koc force-pushed the property-access-reuse branch from 73e457b to 710e059 Compare March 9, 2016 21:27
@Koc
Copy link
Contributor Author

Koc commented Mar 9, 2016

@OskarStark PR rebased and I've reflected last changes, introduced in #3553. CS also have been fixed. Travis is green.

@Koc Koc force-pushed the property-access-reuse branch from 710e059 to 9d48a4a Compare March 9, 2016 21:39
@Koc Koc force-pushed the property-access-reuse branch from 9d48a4a to 33d9692 Compare March 9, 2016 21:46
OskarStark added a commit that referenced this pull request Mar 10, 2016
@OskarStark
Merge pull request #3488 from Koc/property-access-reuse
9681619 
Property access reuse
@OskarStark OskarStark merged commit 9681619 into sonata-project:master Mar 10, 2016
@franmomu franmomu mentioned this pull request Nov 24, 2020
Closed
3 tasks
franmomu added a commit to franmomu/SonataAdminBundle that referenced this pull request Nov 27, 2020
@franmomu
Deprecate Pool::getPropertyAccessor
bd57d62 
PropertyAccesor was added to Pool in order to reuse it for
performance reasons in sonata-project#3488

In symfony/symfony#16838 cache was added
to the component, so there is no need to reuse it thought the Pool
service and it can be injected into the services.
franmomu added a commit to franmomu/SonataAdminBundle that referenced this pull request Nov 27, 2020
@franmomu
Deprecate Pool::getPropertyAccessor
5f1e06c 
PropertyAccesor was added to Pool in order to reuse it for
performance reasons in sonata-project#3488

In symfony/symfony#16838 cache was added
to the component, so there is no need to reuse it thought the Pool
service and it can be injected into the services.
@franmomu franmomu mentioned this pull request Nov 27, 2020
Merged
franmomu added a commit to franmomu/SonataAdminBundle that referenced this pull request Nov 27, 2020
@franmomu
Deprecate Pool::getPropertyAccessor
2a8e162 
PropertyAccesor was added to Pool in order to reuse it for
performance reasons in sonata-project#3488

In symfony/symfony#16838 cache was added
to the component, so there is no need to reuse it thought the Pool
service and it can be injected into the services.
franmomu added a commit to franmomu/SonataAdminBundle that referenced this pull request Nov 29, 2020
@franmomu
Deprecate Pool::getPropertyAccessor
74c15ad 
PropertyAccesor was added to Pool in order to reuse it for
performance reasons in sonata-project#3488

In symfony/symfony#16838 cache was added
to the component, so there is no need to reuse it thought the Pool
service and it can be injected into the services.
franmomu added a commit to franmomu/SonataAdminBundle that referenced this pull request Nov 29, 2020
@franmomu
Deprecate Pool::getPropertyAccessor
477d5d4 
PropertyAccesor was added to Pool in order to reuse it for
performance reasons in sonata-project#3488

In symfony/symfony#16838 cache was added
to the component, so there is no need to reuse it thought the Pool
service and it can be injected into the services.
VincentLanglet pushed a commit that referenced this pull request Nov 29, 2020
@franmomu @VincentLanglet
Deprecate Pool::getPropertyAccessor
ea9f3a4 
PropertyAccesor was added to Pool in order to reuse it for
performance reasons in #3488

In symfony/symfony#16838 cache was added
to the component, so there is no need to reuse it thought the Pool
service and it can be injected into the services.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@OskarStark OskarStark

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@Koc @OskarStark @core23
Morty Proxy This is a proxified and sanitized view of the page, visit original site.