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

Supported exporting closures with use() variables #7

Merged
merged 1 commit into from
Jan 23, 2020
Merged

Supported exporting closures with use() variables #7

merged 1 commit into from
Jan 23, 2020

Conversation

jasny
Copy link
Contributor

@jasny jasny commented Jan 22, 2020

Added the CLOSURE_USE_AS_VAR CLOSURE_SNAPSHOT_USE option to allow exporting closures with use() variables. Reflection is used to get the (current) value of these variables. These are exported by the exporter, parsed and inserted as statements.

Example

$planet = 'world';
echo VarExporter::export([
    'callback' => function() use ($planet) {
        return 'Hello, ' . $planet . '!';
    }
], VarExported::CLOSURE_SNAPSHOT_USE);
[
    'callback' => function () {
        $planet = 'world';
        return 'Hello, ' . $planet . '!';
    }
]

Added ClosureExporter::getParser() method, not to create multiple parsers for exporting a single closure.

@jasny jasny mentioned this pull request Jan 22, 2020
@BenMorel
Copy link
Member

Hi, this feature looks great, thank you!

A few questions:

  • What happens if the use statement is a reference? i.e. use (& $foo)? I guess we should throw an exception (and document this)?
  • maybe CLOSURE_USE_AS_VAR would be better named CLOSURE_SNAPSHOT_USE?

throw new ExportException("The closure has bound variables through 'use', this is not supported.", $path);
}

$static = $reflectionFunction->getStaticVariables();
Copy link
Member

Choose a reason for hiding this comment

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

I didn't know that getStaticVariables() returned use statements, too. Is this documented anywhere? The manual doesn't say anything about it, so this makes me a bit chilly to rely on it!

This also made me curious: what happens if you have both a use statement and a static variable with the same name? It looks like use wins, so at least we should be safe here:

https://3v4l.org/jbGIf

Copy link
Contributor Author

Choose a reason for hiding this comment

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

getStaticVariables() returns op_array.static_variables. This is intended for variables bound with static, but closures also use this array for the use variables.

I asked @nikic; This is an implementation detail and might be changed in the future (PHP 8). However, it should always be possible to get these vars using reflection. So if it's changed, a method like ReflectionFunction::getUseVariables() would be added.

Copy link
Member

Choose a reason for hiding this comment

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

Alright, with good unit tests we should be covered anyway. Maybe it's time to start testing against nightly (with allow_failures), so that we know right away if this implementation detail changes?

@jasny
Copy link
Contributor Author

jasny commented Jan 23, 2020

I don't think we should do anything special with referenced use variables like use (& $foo) since varexporter always ignores references. Eg;

$foo = "Foo";
$var = ['x' => &$foo, 'y' => &$foo];

VarExporter::export($var);
[
    'x' => 'Foo',
    'y' => 'Foo'
]

Naming the option CLOSURE_SNAPSHOT_USE also suggests this behavior.

@BenMorel
Copy link
Member

I don't think we should do anything special with referenced use variables like use (& $foo) since varexporter always ignores references.

Makes sense, I guess. I'm never really comfortable with references.

README.md Outdated Show resolved Hide resolved
@jasny jasny requested a review from BenMorel January 23, 2020 11:08
Added `CLOSURE_SNAPSHOT_USE` option.
@BenMorel BenMorel merged commit ce1be1d into brick:master Jan 23, 2020
@BenMorel
Copy link
Member

Awesome work, thank you! 👍 Let's move on to #8.

@BenMorel
Copy link
Member

Released in 0.3.1 🚀

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