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

Drop entry acl #6

Open
wants to merge 6 commits into
base: main
Choose a base branch
from
Open

Drop entry acl #6

wants to merge 6 commits into from

Conversation

ewgRa
Copy link
Contributor

@ewgRa ewgRa commented Sep 10, 2015

Q A
Bug fix? yes
New feature? no
BC breaks? yes
Deprecations? no
Tests pass? -
Fixed tickets symfony/symfony#2376
License MIT
Doc PR

As I notice in symfony/symfony#2376, Acl have memory leak.

This is kind of bad design of code, that use something like this:
$this->entry = new Entry(this);

as a result Acl have refcount=2, and second refcount from object itself. As a result when we do unset($acl) it is not free memory at all. This PR - fix for this.

@linaori
Copy link
Contributor

linaori commented Sep 11, 2015

Can you add an UPGRADE-3.0.md file which states exactly which changes are done that break backwards compatibility? This will help people upgrade.

Example: https://github.com/symfony/symfony/blob/master/UPGRADE-3.0.md

@ewgRa
Copy link
Contributor Author

ewgRa commented Sep 14, 2015

done.
Also fix another places in code and add test for releaseMemory. Review please

@ewgRa
Copy link
Contributor Author

ewgRa commented Sep 14, 2015

@iltar ClassUtils not related to this PR. It was just local quick fix for test run that I accidentally commit and then revert it back. Problem with ClassUtils must be fixed in PR #3

@ewgRa
Copy link
Contributor Author

ewgRa commented Sep 14, 2015

2.8 tests are broken now, due to ClassUtils. When I merge your PR about ClassUtils to my PR, all tests passed.

@nicolas-grekas nicolas-grekas changed the base branch from master to main November 19, 2020 12:30
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.

2 participants