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

[DependencyInjection] Fix serialization of \Closure in RemoveUnusedDefinitionsPass #30048

Merged
merged 1 commit into from Feb 1, 2019

Conversation

Projects
None yet
3 participants
@XuruDragon
Copy link
Contributor

XuruDragon commented Jan 31, 2019

Q A
Branch? master
Bug fix? yes
New feature? no
BC breaks? no
Deprecations? no
Tests pass? yes
Fixed tickets #29694
License MIT
Doc PR n/a

Fix the issue #29694

@nicolas-grekas

This comment has been minimized.

Copy link
Member

nicolas-grekas commented Jan 31, 2019

If I remember correctly, we're using closures here to run their logic lazily, because doing it earlier has other unwanted side effects.
If that's confirmed (GitHub history to the rescue) then this patch is not correct.
Could you please have a look?

@XuruDragon

This comment has been minimized.

Copy link
Contributor Author

XuruDragon commented Jan 31, 2019

ok, will have a look

@nicolas-grekas What about using the __sleep() method and resolves all potentials closures by calling the getErrors() (before my patch) inside it ?

@XuruDragon XuruDragon force-pushed the XuruDragon:issue-29694 branch 5 times, most recently from 02f5ada to 9e8f450 Jan 31, 2019

@nicolas-grekas

This comment has been minimized.

Copy link
Member

nicolas-grekas commented Jan 31, 2019

That could work! A test case would definitely prove it :)

@XuruDragon

This comment has been minimized.

Copy link
Contributor Author

XuruDragon commented Jan 31, 2019

I've to change some, __sleep have a strange behavior look at appveyor result...

So I've change to the \Serializable interface and I'm currently implementing the two methods

@nicolas-grekas

This comment has been minimized.

Copy link
Member

nicolas-grekas commented Jan 31, 2019

We're trying to kill Serializable, we should try understand what's being this strangeness before resorting to the interface.

@nicolas-grekas

This comment has been minimized.

Copy link
Member

nicolas-grekas commented Jan 31, 2019

Ok, it's not strange, it's the specifications of __sleep(), see its doc.
We need to use a dynamic property and implement __wakeup instead.

@XuruDragon XuruDragon force-pushed the XuruDragon:issue-29694 branch 6 times, most recently from 1eb0ebd to ee55e20 Feb 1, 2019

@nicolas-grekas nicolas-grekas changed the title [DependencyInjection] Fix #29694 - cannot serialize \Closure [DependencyInjection] Fix serialization of \Closure in RemoveUnusedDefinitionsPass Feb 1, 2019

@XuruDragon XuruDragon force-pushed the XuruDragon:issue-29694 branch 2 times, most recently from dd32ace to 741fa88 Feb 1, 2019

@XuruDragon XuruDragon changed the base branch from master to 4.2 Feb 1, 2019

@XuruDragon XuruDragon force-pushed the XuruDragon:issue-29694 branch from 741fa88 to 092796b Feb 1, 2019

@XuruDragon XuruDragon requested review from dunglas , lyrixx and sroze as code owners Feb 1, 2019

@XuruDragon XuruDragon requested a review from xabbuh as a code owner Feb 1, 2019

@XuruDragon XuruDragon changed the base branch from 4.2 to master Feb 1, 2019

@XuruDragon XuruDragon force-pushed the XuruDragon:issue-29694 branch from 092796b to d3d02f0 Feb 1, 2019

@nicolas-grekas nicolas-grekas added this to the next milestone Feb 1, 2019

[DependencyInjection] Fix serialization of \Closure in RemoveUnusedDe…
…finitionsPass

Signed-off-by: Anthony MARTIN <anthony.martin@sensiolabs.com>

| Q             | A
| ------------- | ---
| Branch?       | master
| Bug fix?      | yes
| New feature?  | no
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | #29694
| License       | MIT
| Doc PR        | n/a

Fix the issue #29694

@XuruDragon XuruDragon force-pushed the XuruDragon:issue-29694 branch from d3d02f0 to b092502 Feb 1, 2019

@nicolas-grekas

This comment has been minimized.

Copy link
Member

nicolas-grekas commented Feb 1, 2019

Thank you @XuruDragon.

@nicolas-grekas nicolas-grekas merged commit b092502 into symfony:master Feb 1, 2019

1 of 3 checks passed

continuous-integration/travis-ci/pr The Travis CI build could not complete due to an error
Details
continuous-integration/appveyor/pr Waiting for AppVeyor build to complete
Details
fabbot.io Your code looks good.
Details

nicolas-grekas added a commit that referenced this pull request Feb 1, 2019

bug #30048 [DependencyInjection] Fix serialization of \Closure in Rem…
…oveUnusedDefinitionsPass (XuruDragon)

This PR was merged into the 4.3-dev branch.

Discussion
----------

[DependencyInjection] Fix serialization of \Closure in RemoveUnusedDefinitionsPass

| Q             | A
| ------------- | ---
| Branch?       | master
| Bug fix?      | yes
| New feature?  | no
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | #29694
| License       | MIT
| Doc PR        | n/a

Fix the issue #29694

Commits
-------

b092502 [DependencyInjection] Fix serialization of \Closure in RemoveUnusedDefinitionsPass

@XuruDragon XuruDragon deleted the XuruDragon:issue-29694 branch Feb 1, 2019

@XuruDragon XuruDragon restored the XuruDragon:issue-29694 branch Feb 8, 2019

@XuruDragon XuruDragon deleted the XuruDragon:issue-29694 branch Feb 8, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment