Skip to content

Conversation

@ghost
Copy link

@ghost ghost commented Dec 11, 2017

Fixed issue #364. File does not written because autoload executing much earlier than boolean returned.

@Ocramius
Copy link
Owner

Requires a test case

@Ocramius Ocramius assigned ghost Dec 11, 2017
@Ocramius Ocramius added the bug label Dec 11, 2017

$strategy = $this->configuration->getGeneratorStrategy();

if ($strategy instanceof FileWriterGeneratorStrategy) {
Copy link
Owner

Choose a reason for hiding this comment

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

This couples the factory implementation to an implementation of the GeneratorStrategy - please don't do this. The only assumption is that you get a generator strategy, but you can't upcast and check it.

Copy link
Author

Choose a reason for hiding this comment

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

@Ocramius. Please check, it is updated.

Fixed issue #364. File does not written because autoload executing much earlier than boolean returned.
@Ocramius
Copy link
Owner

@MadLexx you adapted the broken tests, but didn't introduce a test verifying the issue around #364 :-\

@ghost
Copy link
Author

ghost commented Dec 12, 2017

@Ocramius. There is problem with loaders like as ZF1 they doesn't check is the file exists and try to include him. So when you use class_exists() second parameter autoload=true by default it's tried to load proxy file than is not generated yet. And error on including this not found file will happen. Unfortunately I don't not how to register the autoload function that will make error in test. Adding autoload function must be not in tests - another not will not work, if you know how it can be simulated i'll be grateful.

@Ocramius
Copy link
Owner

@MadLexx check https://github.com/Ocramius/ProxyManager/tree/master/tests/language-feature-scripts for the absolute simplest way to implement such a test.

You can register an autoloader in a .phpt test.

@ghost
Copy link
Author

ghost commented Dec 12, 2017

@Ocramius These tests seem to work. Thanks. Please check.

@ghost
Copy link
Author

ghost commented Dec 13, 2017

@Ocramius Please check is this realization is good for you? Thanks.


spl_autoload_register(function ($name) use ($fqcn) {
if ($name === $fqcn) {
throw new \Exception(sprintf("Cannot load class %s", $name));
Copy link
Owner

Choose a reason for hiding this comment

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

Oh, now I understand what the bug is about. I don't think this needs fixing, tbh - this is a broken autoloader, not a broken ProxyManager detail.

The simplest solution for this is to have the proxy autoloader on top of the autoload stack, but overall the sanest solution is to trash the broken autoloader. Autoloaders should have a :bool signature and only crash on fatals...

Copy link
Author

Choose a reason for hiding this comment

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

@Ocramius. Thanks for comment. I understand this, but many old projects has "bad" autoloader, and this fix can make them work. In another side if proxies does not generated before script run we will have error even if the proxy autoloader are registered at top. ZF1 one of "wrong" loader examples. Unfortunately not all projects has composer autoloader or has custom autoloader that followed by best practices.

Copy link
Author

Choose a reason for hiding this comment

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

With this patch they will generate proxy if they don't generated for "file write" strategy without the work break.

@ghost ghost closed this Jan 25, 2018
@Ocramius
Copy link
Owner

Was this supposed to be closed?

This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants