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

$this->view->render() debug #13046

Closed
wydhit opened this Issue Aug 29, 2017 · 19 comments

Comments

Projects
None yet
5 participants
@wydhit

wydhit commented Aug 29, 2017

in controller

return $this->view->render("A",'B',['name'=>'sam','age'=>20]);

but in view I can not get $name $age
https://github.com/phalcon/cphalcon/blob/master/phalcon/mvc/view.zep#L787-L789

I find this code

this->_params = params;

I thind that code should be

this->_viewParams= params;

Details

  • Phalcon version: (3.2.2)
  • PHP Version: (5.6)
@Jurigag

This comment has been minimized.

Show comment
Hide comment
@Jurigag

Jurigag Aug 29, 2017

Member

Hmmm you might be right, i don't even see this->_params used anywhere except this line.

Member

Jurigag commented Aug 29, 2017

Hmmm you might be right, i don't even see this->_params used anywhere except this line.

@sergeyklay sergeyklay added this to the 3.2.x milestone Aug 29, 2017

@JABirchall

This comment has been minimized.

Show comment
Hide comment
@JABirchall

JABirchall Aug 30, 2017

I'm not sure if this is how its intended to be used?

I assign the params individually, I also select my view

$post = Posts::findByPostId($id);
$this->view->pick('post/view/featured');
$this->view-post = $post;
$this->view->author = $post->author;

or you can use $this->view->setVars(['name'=>'sam','age'=>20]);

you also don't need to return the view from the controller. After the method executions Phalcon automatically renders the view.

JABirchall commented Aug 30, 2017

I'm not sure if this is how its intended to be used?

I assign the params individually, I also select my view

$post = Posts::findByPostId($id);
$this->view->pick('post/view/featured');
$this->view-post = $post;
$this->view->author = $post->author;

or you can use $this->view->setVars(['name'=>'sam','age'=>20]);

you also don't need to return the view from the controller. After the method executions Phalcon automatically renders the view.

@wydhit

This comment has been minimized.

Show comment
Hide comment
@wydhit

wydhit Aug 30, 2017

$this->view->setVars(['name'=>'sam','age'=>20]);
in this way that no problem

--

wydhit commented Aug 30, 2017

$this->view->setVars(['name'=>'sam','age'=>20]);
in this way that no problem

--

@JABirchall

This comment has been minimized.

Show comment
Hide comment
@JABirchall

JABirchall Aug 31, 2017

in this way that no problem

So is your problem resolved?

JABirchall commented Aug 31, 2017

in this way that no problem

So is your problem resolved?

@Jurigag

This comment has been minimized.

Show comment
Hide comment
@Jurigag

Jurigag Aug 31, 2017

Member

I guess, he just mention here we have some property and method which is actually just some garbage which does nothing.

Member

Jurigag commented Aug 31, 2017

I guess, he just mention here we have some property and method which is actually just some garbage which does nothing.

@wydhit

This comment has been minimized.

Show comment
Hide comment
@wydhit

wydhit Sep 1, 2017

my problem resolved
@Jurigag yes , if this params is no used ,just remove

wydhit commented Sep 1, 2017

my problem resolved
@Jurigag yes , if this params is no used ,just remove

@sergeyklay sergeyklay referenced this issue Sep 24, 2017

Merged

Fixed Phalcon\Mvc\View::render #13087

3 of 3 tasks complete
@sergeyklay

This comment has been minimized.

Show comment
Hide comment
@sergeyklay

sergeyklay Sep 24, 2017

Member

Fixed in the 3.2.x branch. Feel free to open a new issue if the problem appears again. Thank you for contributing.

Member

sergeyklay commented Sep 24, 2017

Fixed in the 3.2.x branch. Feel free to open a new issue if the problem appears again. Thank you for contributing.

@sergeyklay sergeyklay closed this Sep 24, 2017

@dugwood

This comment has been minimized.

Show comment
Hide comment
@dugwood

dugwood Oct 18, 2017

Contributor

Just upgraded to 3.2.3, and hit a bug with this fix:

$router->add('/test-page/{bar:[0-9]+}', ['controller' => 'MyController']);
$this->view->setVar('bar', 'foo');

now returns the dispatcher's getParam('bar') value instead of the one set in the view. Shouldn't the dispatcher set its params before allowing setVar()? To me it's a breaking change, I'm to fix every controller which has the same parameters as the controller will set :-(

Contributor

dugwood commented Oct 18, 2017

Just upgraded to 3.2.3, and hit a bug with this fix:

$router->add('/test-page/{bar:[0-9]+}', ['controller' => 'MyController']);
$this->view->setVar('bar', 'foo');

now returns the dispatcher's getParam('bar') value instead of the one set in the view. Shouldn't the dispatcher set its params before allowing setVar()? To me it's a breaking change, I'm to fix every controller which has the same parameters as the controller will set :-(

@Jurigag

This comment has been minimized.

Show comment
Hide comment
@Jurigag

Jurigag Oct 18, 2017

Member

I guess there should be array_merge paras from dispatcher and viewParams then?

Member

Jurigag commented Oct 18, 2017

I guess there should be array_merge paras from dispatcher and viewParams then?

@Jurigag

This comment has been minimized.

Show comment
Hide comment
@Jurigag

Jurigag Oct 18, 2017

Member

Though it's weird, it happens already.

Member

Jurigag commented Oct 18, 2017

Though it's weird, it happens already.

@dugwood

This comment has been minimized.

Show comment
Hide comment
@dugwood

dugwood Oct 18, 2017

Contributor

It didn't happen in 3.2.2. So it's a breaking change in 3.2.3.

The issue is that setVars from dispatcher erase previously declared vars (in the controller). It may be useful to add a «noErase» option in setVars(), so that this behaviour is fixed. If I set something in the controller, I think it has to stay with this value.

Contributor

dugwood commented Oct 18, 2017

It didn't happen in 3.2.2. So it's a breaking change in 3.2.3.

The issue is that setVars from dispatcher erase previously declared vars (in the controller). It may be useful to add a «noErase» option in setVars(), so that this behaviour is fixed. If I set something in the controller, I think it has to stay with this value.

@dugwood

This comment has been minimized.

Show comment
Hide comment
@dugwood

dugwood Oct 18, 2017

Contributor

@Jurigag: the issue is not the array_merge, it's the fact that:
https://github.com/phalcon/cphalcon/blob/master/phalcon/mvc/application.zep#L351
is run after every view->setVar(). Hence dispatcher->getParams() will erase any variable set in the controller.

It was indeed a bug, but the fix creates a behaviour that makes the 3.2.3 a 3.3.0...

Contributor

dugwood commented Oct 18, 2017

@Jurigag: the issue is not the array_merge, it's the fact that:
https://github.com/phalcon/cphalcon/blob/master/phalcon/mvc/application.zep#L351
is run after every view->setVar(). Hence dispatcher->getParams() will erase any variable set in the controller.

It was indeed a bug, but the fix creates a behaviour that makes the 3.2.3 a 3.3.0...

@Jurigag

This comment has been minimized.

Show comment
Hide comment
@Jurigag

Jurigag Oct 18, 2017

Member

Yea tbh i don't see really a good way to fix it right now. This is beacause of automatic rendering which always is passing parameters. The only way i see is really to https://github.com/sergeyklay/cphalcon/blob/3f703832786c7fb7a420bcf31ea0953ba538591d/phalcon/mvc/view.zep#L432 add here argument like reverse = true(or other name, not sure). And if merge and this reverse is true then do:

let this->_viewParams = array_merge(params, this->_viewParams);

Which will fix this. And use this reverse as true only when adding vars from dispatcher(autoamtic rendering) so it won't override params already set to view.

Member

Jurigag commented Oct 18, 2017

Yea tbh i don't see really a good way to fix it right now. This is beacause of automatic rendering which always is passing parameters. The only way i see is really to https://github.com/sergeyklay/cphalcon/blob/3f703832786c7fb7a420bcf31ea0953ba538591d/phalcon/mvc/view.zep#L432 add here argument like reverse = true(or other name, not sure). And if merge and this reverse is true then do:

let this->_viewParams = array_merge(params, this->_viewParams);

Which will fix this. And use this reverse as true only when adding vars from dispatcher(autoamtic rendering) so it won't override params already set to view.

@dugwood

This comment has been minimized.

Show comment
Hide comment
@dugwood

dugwood Oct 18, 2017

Contributor

Clearly, as it wasn't working before, we should drop the injection in https://github.com/phalcon/cphalcon/blob/master/phalcon/mvc/application.zep#L351

Then we'll think it through, perhaps by setting it when the view is created.

@sergeyklay: can we please rollback this fix and release a new version? Should I open a new bug?

Contributor

dugwood commented Oct 18, 2017

Clearly, as it wasn't working before, we should drop the injection in https://github.com/phalcon/cphalcon/blob/master/phalcon/mvc/application.zep#L351

Then we'll think it through, perhaps by setting it when the view is created.

@sergeyklay: can we please rollback this fix and release a new version? Should I open a new bug?

@dugwood

This comment has been minimized.

Show comment
Hide comment
@dugwood

dugwood Oct 18, 2017

Contributor

Not a rollback, but just remove the injection here:
https://github.com/phalcon/cphalcon/blob/master/phalcon/mvc/application.zep#L351

I'm sending a patch right now.

Contributor

dugwood commented Oct 18, 2017

Not a rollback, but just remove the injection here:
https://github.com/phalcon/cphalcon/blob/master/phalcon/mvc/application.zep#L351

I'm sending a patch right now.

@Jurigag

This comment has been minimized.

Show comment
Hide comment
@Jurigag

Jurigag Oct 18, 2017

Member

As i told, first happens your view setting(like vars), then automatic rendering from application. There is no really way to reverse this process. But i think what i proposed could be good enough like reverse array_merge when adding parameters from dispatcher. So only any additional parameters are added to view, ones set by us by setVars or setVar will be not replaced this way.

Member

Jurigag commented Oct 18, 2017

As i told, first happens your view setting(like vars), then automatic rendering from application. There is no really way to reverse this process. But i think what i proposed could be good enough like reverse array_merge when adding parameters from dispatcher. So only any additional parameters are added to view, ones set by us by setVars or setVar will be not replaced this way.

@dugwood

This comment has been minimized.

Show comment
Hide comment
@dugwood

dugwood Oct 18, 2017

Contributor

@Jurigag the fact is the dispatcher's params were never read => we should ignore these, and that will keep this bug closed, but no breaking change. Then we can think of any other solution to inject dispatcher's params... even if it's useless I think.

Setting a param in a getRender() or in a partial is logical, forcing the dispatcher's params isn't.

Contributor

dugwood commented Oct 18, 2017

@Jurigag the fact is the dispatcher's params were never read => we should ignore these, and that will keep this bug closed, but no breaking change. Then we can think of any other solution to inject dispatcher's params... even if it's useless I think.

Setting a param in a getRender() or in a partial is logical, forcing the dispatcher's params isn't.

@Jurigag

This comment has been minimized.

Show comment
Hide comment
@Jurigag

Jurigag Oct 18, 2017

Member

Well maybe yea. I don't see any point of passing dispatcher params to view anyway to be honest. Those are diffrent things really. And since it wasn't working anyway we should just drop this dispatcher->getParams() this is other idea worth thinking.

Member

Jurigag commented Oct 18, 2017

Well maybe yea. I don't see any point of passing dispatcher params to view anyway to be honest. Those are diffrent things really. And since it wasn't working anyway we should just drop this dispatcher->getParams() this is other idea worth thinking.

@dugwood

This comment has been minimized.

Show comment
Hide comment
@dugwood

dugwood Oct 18, 2017

Contributor

Thanks for your input.

I'm opening a new bug and a patch.

Contributor

dugwood commented Oct 18, 2017

Thanks for your input.

I'm opening a new bug and a patch.

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