Cphalcon: Variable scope when is set as view param and rendered by getRender()

[slechtic]

When I set view params and call getRender, the variables are visible in the view but out of their scope (in action) too.

$di->set('view', function () use ($config) {

    $view = new View();

    $view->setViewsDir($config->application->viewsDir);

    $view->registerEngines(array(
        '.volt' => function ($view, $di) use ($config) {

            $volt = new VoltEngine($view, $di);

            $volt->setOptions(array(
                'compiledPath' => $config->application->cacheDir,
                'compiledSeparator' => '_'
            ));

            return $volt;
        },
        '.phtml' => 'Phalcon\Mvc\View\Engine\Php'
    ));

    $view->setParamToView('paramFromService', 'Param from service');

    return $view;
}, true);

public function indexAction()
{
    $content = $this->view->getRender('index', 'index', [
        'viewParam' => 'Test view param'
    ]);
    var_dump($content);
    var_dump($viewParam);
    var_dump($paramFromService);
    die;
}

• Variable $content is corrent.
• Variable $viewParam contains string 'Test view param'. I expected notice Undefined variable.
• Variable $paramFromService is assign to view in service.php and is accessible in this action too. I expected notice Undefined variable.

Testing environment

• Phalcon version: 3.0.4
• PHP Version: 5.6.24 as Apache module, 7.0.16. as Fcgi
• Operating System: Windows 10
• Server: Apache 2.4

Source files

https://github.com/slechtic/phalcon_variable_scope

[Jurigag]

Does this happen for 5.6.24 too? I thought it's only php 7 problem.

[slechtic]

Yes, for both version of PHP.

[jesseforrest]

@sergeyklay : I was wondering if Bug - Medium refers to medium complexity or medium priority. If it's related to priority, I would like to push that this is high-priority for an MVC framework. imo, variable scope for views seems like an important topic for an MVC framework to get right.

Phalcon might be performing faster than it actually should be, since it likely is not be making copies of variables for each view, which may be required to resolve this problem. Either way, I just want to voice that I think this one is larger than it might seem (in complexity and priority), in order to do it right where it doesn't add a lot of overhead.

[Jurigag]

Well tbh as far as i see this https://github.com/phalcon/cphalcon/blob/master/phalcon/mvc/view/engine/volt.zep#L114

The current symbol table is method i think here. This is causing this problem. I guess the workaround could be something like create anonymous function which will just accept parameters and create it and do render there?

[jesseforrest]

I should also note, that I am not using Volt. I am using vanilla phtml. I'm assuming the code may be similar, but wanted to note that this is not just a Volt issue.

[Jurigag]

Yes - code is similar, i will try to work on it, and if my assumption will be correct i will do PR with fix.

[Jurigag]

This issue is fixed on php 5, but still happens on php 7.

On php 7 create_symbol_table isn't implemented at all in zephir.

[Jurigag]

Can we add here label php7? @sergeyklay

[sergeyklay]

Fixed in the 3.2.x branch.

[piit79]

@sergeyklay Unfortunately, it seems this issue has not actually been fixed in PHP 7 and is still present in 3.3.1.
The unit test for this issue is incorrect and doesn't catch the failure - see PR https://github.com/phalcon/cphalcon/pull/13288

I couldn't find the fix for this issue in the PR https://github.com/phalcon/cphalcon/pull/12782 referenced above - is it possible that a commit got lost somehow?

[sergeyklay]

@piit79 It is very strange.. I'll try to sort out as soon as I can. Let's use #13288 for discussion

[piit79]

@sergeyklay Thanks a lot for your attention to this issue. This has bitten us when we moved from PHP 5 to PHP 7. Once confirmed, wouldn't it make sense to reopen this issue?

[sergeyklay]

I hope we managed to sort out with this issue finally here https://github.com/phalcon/cphalcon/pull/13606. All necessary changes are already in the appropriate branch. We will try to release the next version in the coming days. Thank you for your patience, the report, and for helping us make Phalcon better.

[piit79]

@sergeyklay Just wanted to point out that the test for this failure condition still seems to be broken, unless it was fixed somewhere else and I missed it: https://github.com/phalcon/cphalcon/blob/caeac66e7db02fd54c44a27cf22a6381654af2ee/tests/unit/Mvc/ViewTest.php#L672

My pull request https://github.com/phalcon/cphalcon/pull/13288 with the fix was closed without merging for some reason...

[sergeyklay]

@piit79 Your branch had conflicts so I couldn't merge it automatically. I merged these changes by hand into 3.4.x branch. The 3.3.x branch is no longer supported. We'll try to release 3.4.x ASAP.

[sergeyklay]

https://github.com/phalcon/cphalcon/blob/ad71cf6f388f10555906f6061325887af657e699/tests/unit/Mvc/ViewTest.php#L670-L675

[piit79]

Perfect, thanks! I was looking in the wrong place then. And sorry for the double post :-)