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

__destruct test PR #471

Closed
wants to merge 1 commit into from
Closed

__destruct test PR #471

wants to merge 1 commit into from

Conversation

odahcam
Copy link

@odahcam odahcam commented Jun 29, 2019

No description provided.

@Ocramius
Copy link
Owner

This is the correct way to design the test, but it doesn't seem to reproduce the original issue

$factory = new \ProxyManager\Factory\LazyLoadingValueHolderFactory($configuration);

$proxy = $factory->createProxy(Destructable::class, function (& $wrapped, $proxy, $method, array $parameters, & $initializer) {
$initializer = null;
Copy link
Owner

Choose a reason for hiding this comment

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

Maybe this should throw to reproduce the issue?

Copy link
Author

Choose a reason for hiding this comment

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

I will have to ask @mnapoli about how container proxies works, because it only happens with this option enabled. 🤔

Copy link

Choose a reason for hiding this comment

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

Hi, I'm not familiar with the issue here sorry. But I don't see why an exception should be thrown, especially if the test case reproduces the issue.

Copy link
Owner

Choose a reason for hiding this comment

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

@odahcam the issue is not reproduced here:

1) /home/travis/build/Ocramius/ProxyManager/tests/language-feature-scripts/lazy-loading-__destruct-throws.phpt
Failed asserting that string matches format description.
--- Expected
+++ Actual
@@ @@
-%SUncaught Error:%sCall to a member function __destruct() on null in %a
/home/travis/build/Ocramius/ProxyManager/tests/language-feature-scripts/lazy-loading-__destruct-throws.phpt:26

@mnapoli I'm also having trouble in understanding the full scenario for this issue, sorry...

Copy link
Author

Choose a reason for hiding this comment

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

Yesterday I started to think this is a issue with PHP-DI, since the problem only occours when I enable the option $container->writeProxiesToFiles(true, 'folder/path'). If @mnapoli could explain what that method does I would understand it better.

@Ocramius maybe here PHP-DI/PHP-DI#669 you can find more information.

At the end I don't know what exactly is happening either, I'm just facing the problem, where one of the proxies generated by PHP-DI using this library throws inside a __destruct method because the instance of the class the proxy is for is null inside the proxy class. Yes confusing and strange.

As you can see, things are null here:

image

And then the error occours:

image

Maybe you understand better than me whats happening.

Copy link
Owner

Choose a reason for hiding this comment

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

What's the initializer there? It got called, but not reset to null, which to me smells like it threw an exception.

Copy link
Author

Choose a reason for hiding this comment

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

<?php
$this->initializer5d18ea48bcba2345610290 && $this->initializer5d18ea48bcba2345610290->__invoke($this->valueHolder5d18ea48bcb97124004552, $this, '__destruct', array(), $this->initializer5d18ea48bcba2345610290);

image

Is there a way for me to check if it throwed?

Copy link
Owner

Choose a reason for hiding this comment

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

I don't store such information, but I'm pretty sure that the engine will aggressively call __destruct() anyway.

I'd use a step debugger and put breakpoints on any of the API of the generated code.

@Ocramius Ocramius closed this Dec 20, 2020
@Ocramius Ocramius deleted the branch Ocramius:master December 20, 2020 16:06
@Ocramius
Copy link
Owner

Note: this was closed due to master being removed, but the issue stays valid and requires more investigation :-\

@odahcam
Copy link
Author

odahcam commented Dec 21, 2020

Maybe we should open an issue pointing to this PR.

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.

3 participants