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

Variable scope when is set as view param and rendered by getRender() #12648

Closed
slechtic opened this Issue Feb 24, 2017 · 12 comments

Comments

Projects
None yet
6 participants
@slechtic

slechtic commented Feb 24, 2017

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

This comment has been minimized.

Show comment
Hide comment
@Jurigag

Jurigag Feb 27, 2017

Member

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

Member

Jurigag commented Feb 27, 2017

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

@slechtic

This comment has been minimized.

Show comment
Hide comment
@slechtic

slechtic Feb 27, 2017

Yes, for both version of PHP.

Yes, for both version of PHP.

@jesseforrest

This comment has been minimized.

Show comment
Hide comment
@jesseforrest

jesseforrest Mar 15, 2017

@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.

@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

This comment has been minimized.

Show comment
Hide comment
@Jurigag

Jurigag Mar 15, 2017

Member

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?

Member

Jurigag commented Mar 15, 2017

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

This comment has been minimized.

Show comment
Hide comment
@jesseforrest

jesseforrest Mar 16, 2017

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.

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

This comment has been minimized.

Show comment
Hide comment
@Jurigag

Jurigag Mar 16, 2017

Member

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

Member

Jurigag commented Mar 16, 2017

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 comment has been minimized.

Show comment
Hide comment
@Jurigag

Jurigag Mar 17, 2017

Member

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.

Member

Jurigag commented Mar 17, 2017

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

This comment has been minimized.

Show comment
Hide comment
@Jurigag

Jurigag Mar 24, 2017

Member

Can we add here label php7? @sergeyklay

Member

Jurigag commented Mar 24, 2017

Can we add here label php7? @sergeyklay

@sergeyklay

This comment has been minimized.

Show comment
Hide comment
@sergeyklay

sergeyklay Apr 9, 2017

Member

Fixed in the 3.2.x branch.

Member

sergeyklay commented Apr 9, 2017

Fixed in the 3.2.x branch.

@sergeyklay sergeyklay closed this Apr 9, 2017

@sergeyklay sergeyklay added this to the 3.2.0 milestone Apr 9, 2017

piit79 pushed a commit to ca-asm/cphalcon that referenced this issue Jan 30, 2018

piit79 pushed a commit to piit79/cphalcon that referenced this issue Jan 30, 2018

piit79 pushed a commit to piit79/cphalcon that referenced this issue Jan 30, 2018

piit79 pushed a commit to piit79/cphalcon that referenced this issue Jan 30, 2018

piit79 pushed a commit to piit79/cphalcon that referenced this issue Jan 30, 2018

piit79 added a commit to piit79/cphalcon that referenced this issue Jan 30, 2018

piit79 added a commit to piit79/cphalcon that referenced this issue Jan 30, 2018

@piit79

This comment has been minimized.

Show comment
Hide comment
@piit79

piit79 Jan 30, 2018

@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 #13288

I couldn't find the fix for this issue in the PR #12782 referenced above - is it possible that a commit got lost somehow?

piit79 commented Jan 30, 2018

@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 #13288

I couldn't find the fix for this issue in the PR #12782 referenced above - is it possible that a commit got lost somehow?

@sergeyklay

This comment has been minimized.

Show comment
Hide comment
@sergeyklay

sergeyklay Jan 31, 2018

Member

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

Member

sergeyklay commented Jan 31, 2018

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

@piit79

This comment has been minimized.

Show comment
Hide comment
@piit79

piit79 Feb 8, 2018

@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?

piit79 commented Feb 8, 2018

@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?

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