yiisoft / yii2

Yii 2: The Fast, Secure and Professional PHP Framework
http://www.yiiframework.com
BSD 3-Clause "New" or "Revised" License
14.24k stars 6.91k forks source link

Local Scope View Variables #14578

Closed KnightYoshi closed 7 years ago

KnightYoshi commented 7 years ago

When setting data from the controller to the views using $this->view->params I think it would be better if the params property was actually a method. That way it can be used to set the variable(s) and the scope of the variable.

$this->view->params(['something' => 'value', 'something_else' => 'another value'], ['page_view', 'layout_view']); Where the scope would be the second parameter passed to the method and it could either be an array or a string with the name(s) of the view(s) which the variable would be available to or it would be null, no scope. Without a scope the variable would created for each view rendered.

Then say if layout_view renders in a sidebar or some other view those other views won't see the something or something_else variables. Then you wouldn't need to use $this->params['something'] to access it since something would be a locally scoped variable to each view.

Edit~ Also variables defined in the render method would be scoped to only that view.

samdark commented 7 years ago

That's a bit too magical to my taste. Would be hard to track where the variable is from. Also how to deal with two identically named views?

KnightYoshi commented 7 years ago

The first question I can't really answer. Do you actually track where the variable is set from with the way it's currently handled, calling $this->params['something'? Since it would be method calls in the controller I imagine it'd be in the stack trace somewhere. For debugging purposes it would make sense that the View object would log the calling controller's name, method, and line number when logging the variable being set.

Well variables with the same name, but different scope would not conflict. If they're set with the same scope, the latter should override the former.

KnightYoshi commented 7 years ago

This came about because I have views that use the same data independently of each other (data is changeable and each can use a different value of the data) and the data has the same variable name. Before switching to Yii I started looking into the PlatesPHP library which has variable scoping. Instead of just adding Plates I thought it'd be better if the framework did this itself, since the View object does some other framework stuff too.

While I could simply use two variable names, I feel scoping variables would be better in the long run.

samdark commented 7 years ago

Do you actually track where the variable is set from with the way it's currently handled, calling $this->params['something'?

I'm not using params except when passing variables to layout. If I need variable in partial then I'd pass to this partial explicitly via render() call.

Well variables with the same name, but different scope would not conflict. If they're set with the same scope, the latter should override the former.

I'm talking about same named views, not same named variables.

KnightYoshi commented 7 years ago

Yes, but if you render a partial in the layout then you'd have to use the params property as well. If you render the partial in the view rendered by the controller and pass the variable you'd have to set the variable for the view rendered by the controller and then set the variable for the partial rendered by the main view. While setting the params property makes setting once, but it's accessible to all views at that point by access it $this->params['var_name']. By making it locally scoped it would be accessible to only that scope as just $var_name, much like how the render property creates variables.

Ah, named views.. I'm not sure I follow, what you mean then. Since to render a view wouldn't change, just scoping variables to the view. $this->render('path/to/view', ['locally_scoped_variable' => 'value only for this view']);

samdark commented 7 years ago

Yes, but if you render a partial in the layout then you'd have to use the params property as well.

No. You can pass these variables from the layout so in the view these will be accessed directly.

If you render the partial in the view rendered by the controller and pass the variable you'd have to set the variable for the view rendered by the controller and then set the variable for the partial rendered by the main view.

Yes.

By naming collisions I mean this:

  1. In PostWidget I have $this->render('view', [...]).
  2. In UserWidget I have $this->render('view', [...]).
  3. In both cases it's called as view. Both files are named view. These are different contexts.
KnightYoshi commented 7 years ago

You'd still have to pull them off of the params array at some point, though. Whereas if you set it in the controller directly to that view and it's not accessible to any other view. Or you could even set the variable scope to the layout and pass it to the partial then if desired.

This goes to the point I just made though, why set it multiple times when in the controller you could just set and if the scope is defined it would only be available to that view, otherwise it's available to every view.

You're talking about the scope which the variable would be set in views have the same name? So the scope would be the view file that's being loaded. Defining a variable for PostWidget currently isn't available in UserWidget is it? Which I believe is the current case; that's an example of a locally scoped variable. Loading the view itself should behave the same as it does now, it resolves to the ID (postwidget and userwidget from your example) and the variables passed to those render methods are only available to those views - the view specified as the first parameter of the render method.

What I'm talking about, though is being able to define variables for views without passing it through the render method.

However, to resolve view scope conflicts for using a method to set the scoped variable something like this could be done $this->view->params(['var1' => 'value', 'var2' => 'value'], '@app/components/userwidget/views/details') scoped to the UserWidget view 'details'

$this->view->params(['var3' => 'value', 'var4' => 'value'], '@app/components/postwidget/views/post') scoped to the PostWidget view 'post'

$this->view->params(['var5' => 'value', 'var6' => 'value'], '@app/views/blog/post') scoped to the a view 'post' file in the views/blog directory

$this->view->params(['var7' => 'value', 'var8' => 'value'], 'helloworld') available to all view files named helloworld when loaded, regardless of their directory

$this->view->params(['var9' => 'value', 'var10' => 'value'], 'id/something') where id resolves like when loading a view with render so if it's called inside of a module it would be modules/current_module/views/id/something , or a widget, or if it's a normal controller calling the method the standard views/controller/something path

$this->view->params(['var11' => 'value', 'var12' => 'value']) available to all views loaded

If that makes more sense?

Edit, like I said before the scope, second parameter, can be a string or an array to specify many view scopes

samdark commented 7 years ago

@yiisoft/core-developers thoughts?

githubjeka commented 7 years ago

If you need scope you should use it through render in places of this scope

'@app/components/userwidget/views/details' - in render run() of UserWidget '@app/views/blog/post' in render by actionPost of BlockController ....

The converting ['var11' => 'value', 'var12' => 'value'] to $var11 & $var12 it's bad because it provide to collisions with render local variables.

I suggest leaving it as it is until better times

KnightYoshi commented 7 years ago

For instance. I have a dynamic main navigation, it uses an AR. To pass it to the navigation partial I have to set it in the controller on the params property $this->view->params['navigation'] = ModeName; Then to access it I have to use $this->params['navigation']. There are a couple issues with this, if I have a side nav I have to give it a different index name and then I still have to access it from the params property instead of directly as a variable. Now granted this is a simple example and it could easily be identified as sidenav, but inside the side nav view identifying it as sidenav shouldn't be necessary.

Now let's say I have two different partials that use the same data, but in one view that can be value 1 and in the other it can be value 2. Instead of having just the name for it I then have to have a modifier attached to the name $this->params['p1_varname'] and $this->params['p2_varname']. Then in those views respectively I'd have to reference it by the name respective. Whereas, if it's locally scoped I can just say $this->view->params(['varname' => $model->type], 'p1') and reference it in that view specifically as $p1 same for 'p2'.

As for the conflict, any conflicting variable assigned that’s afterwards to a specific view will overwrite the shared variable.

KnightYoshi commented 7 years ago

That's no different than $this->view->params['something'] and then later on in another component being loaded $this->view->params['something'] overriding it. Except as I said above the variable assigned for that view will override variables without a scope.

githubjeka commented 7 years ago

Try produce it on your local and working with it. If you experience will be success please provide PR. Now it's magic, with which it is difficult to work in a large team or project. But I would leave it in the current implementation

dynasource commented 7 years ago

IMHO, View::$params is a bad design practice and should be removed from the framework

githubjeka commented 7 years ago

But what instead ?

githubjeka commented 7 years ago

Anyway I vote for delete it in Yii2.1 ! :)

KnightYoshi commented 7 years ago

What instead? Create actual variables for each view defined by the scope or all views if no scope is passed. :D

$this->view->params(['some_var'=>'some_val'], 'blog/post') $this->view->params(['navigation'=>[...]], ['site/desktop_nav', 'site/mobile_nav'])

Then it's as simple as accessing it from the created variable name $some_var and $navigation from within the views defined by the scope or shared variables. It basically would do what render does, creating variables for a view or multiple views, but without rendering the view. This is useful for setting the data to be used in other places than the rendered view by the controller.

Say for something like this

class SiteController extends \yii\web\Controller {
   public function about () {
      $this->view->params(['email'=>'ex@example.com'], 'site/contact_sidebar');
      $this->render('about', [...]);
   }
}

Where site/contact_sidebar is a partial loaded in that you want to pass data to. Instead of passing it to $this->render('about', ['email'=>'ex@example.com']) then in the about view passing it to the render method that's loading the partial just set it for the partial directly; which the partial may not even be loaded by the view rendered, it could be loaded by a layout so you couldn't even pass it from about view to the partial then since the about view doesn't render the partial itself.

dynasource commented 7 years ago

But what instead ?

It is less error prone to pass variables in a functional programming way like with render(). Another solution is to use services to hold specific data.

With respect to ActiveRecords, I am positive for having a Symfony like solution for the storage of $models in the form of Repositories. Perhabs @cebe has already thought this one out with his introduction of Collections.

SamMousa commented 7 years ago

I think this breaks encapsulation.

My controllers knows about a view and about a layout. It passes stuff to the view, whatever other views that view uses and what (part of the) data it passes to them is out of scope of the controller.

samdark commented 7 years ago

Agree. Sorry but we're not going to implement it.

KnightYoshi commented 7 years ago

Then by that logic, you shouldn't be able to set any data on view->params from the controller to be used by any other views such as the layout, because that breaks encapsulation. 👎

KnightYoshi commented 7 years ago

I'm going to address this again. Because I've once again come across a problem revolving around this.

I have a partial that's loaded when the page loads that uses a state from a model. I then have an AJAX request that goes to an action that loads the view as a partial based on a different state from user interactions.

In the former when the page is first loaded I use $this->views->params to set the state value. In the action that returns the partial for the AJAX request I could set it on the params property, but why should I have to when I could pass it to the partial directly.. It needs the data from params because it's loaded into the layout initially, not directly by itself from the AJAX request to get the partial.

Before it was argued, "It passes stuff to the view, whatever other views that view uses and what (part of the) data it passes to them is out of scope of the controller."

I present Plates as a library that has local scope variables, https://github.com/thephpleague/plates The view object knows about the views being loaded and the view object is what handles extracting the data into variables when a view is loaded. The controller is just passing data to the view object.

It's frustrating when I keep coming across this where I'm passing data to a view inconsistently, either as a property on params or as a variable. This is inconsistent behavior. I'd switch to using Plates, but then swapping out the View object itself an error is thrown. Which is fine if passing data to the views loaded was consistent.

samdark commented 7 years ago

That's why your partial should not access $this->view->params but instead should accept regular arguments:

Here is my partial in <?= Html::encode($model->state) ?> state.

It will be called like the following from AJAX-action:

return $this->renderPartial('my_partial', ['model' => $model]);

And like the following from layout:

return $this->renderPartial('my_partial', ['model' => Yii::$app->params->model]);
KnightYoshi commented 7 years ago

I agree! I fully agree! However, the partial when the page is first loaded is loaded by the layout. Not inside of the view being loaded.

I have to partials, comments and friends, being loaded, both use the same state data, but independently; regular and minimal.

samdark commented 7 years ago

It doesn't matter what's loading a partial. Inside the partial itself it's all the same code.

If you need two variants that's not related to how variables are passed. You're either passing additional flag such as $isMinimal, using a separate partial or wrapping some logic into a widget.

KnightYoshi commented 7 years ago

No it's not. In my action that loads the whole page. $this->render('index', ['state' => 'minimal']); The variable state that is created is not accessible in the partial loaded by the layout or the layout itself. It is accessible only inside of the 'index' view, which does not load the partial, so I cannot pass it to the partial.

SamMousa commented 7 years ago

Ah, you load the partial from the layout, which means in the layout you need to pass the params to the partial

KnightYoshi commented 7 years ago

But the layout doesn't have access to it either! My model needs to be accessed by the view being loaded by the action, the layout, and the partials being loaded.

To do so right now I have $this->view->params['model'] = $model. Then inside the layout depending on the model's template property I change styles and in the partials loaded the model's comment_style and friend_style is used to determine some of the HTML.

So then in those views I use it as: $this->params['model']->template, $this->params['model']->comment_style, $this->params['model']->friend_style

In the latter two views though, there's conflict with the partials being loaded since it doesn't need the model, because the data is coming from the user. So I can't even do the above and if I changed the above to add two additional values for the params property with the AJAX action I'd have to have additional logic to set the right value on the params (either comment_style or friend_style) vs being able to just pass the same variable name to both.

samdark commented 7 years ago

Well, you're going too far. It's OK to pass stuff to layout via $this->params but then you need to pass it explicitly. If you'll do so, you'll have uniform variables in your partials.

dynasource commented 7 years ago

I agree that support for proper $model sharing between viewFiles and layouts is missing

samdark commented 7 years ago

@dynasource what do you mean by proper?

dynasource commented 7 years ago

passing through data in a functional way is quite cumbersome imo, especially when you have many view files. Id prefer a solution in which found models are accessible in an Application service.

klimov-paul commented 7 years ago

support for proper $model sharing between viewFiles and layouts is missing

Such suport should not ever exist. Layout file is meant to be used for many view files, which internal structure is unknown for layout. It can not rely on view params passed to some particular view rendering as they might be missing at other view.

If you wish some page fragment to be rendered based on the params you pass to render you should do it inside the main view. You may as well extract some code to separated views and then render them as a part of other view, like CRUD generated by GII does:

<?php

/* @var $this yii\web\View */
/* @var $model app\models\db\Admin */

$this->title = Yii::t('admin', 'Create Admin');
$this->params['breadcrumbs'][] = ['label' => Yii::t('admin', 'Administrators'), 'url' => ['index']];
$this->params['breadcrumbs'][] = $this->title;
?>

<?php /* form is common part for several views and thus is rendered inside them */ ?>
<?= $this->render('_form', [
    'model' => $model,
]) ?>
klimov-paul commented 7 years ago

Also you may use View::beginContent()/View::endContent() to wrap some view into layout with passing parameters without interference of ViewContext:

<?php $this->beginContent($this->findViewFile('/layouts/some'), ['model' => $model]); ?>

Internal view code is here

<?php $this->endContent(); ?>
dynasource commented 7 years ago

my point is to share $models at a higher level

samdark commented 7 years ago

Would you please elaborate?

dynasource commented 7 years ago

well, @Knight-Yoshi clearly points out the usecase in which he needs to get access to the $model that is found by the Controller. This $model is than passed to the View but after that, no further structure is provided to the end user.

This leads to bad design choices like the use of $view->params to store models. The additional solution mentioned by @klimov-paul https://github.com/yiisoft/yii2/issues/14578#issuecomment-323320928, is already better because it is more clear how data is passed.

However, this solution is not really OO and leads to extra complexity and cumbersome layout maintenance. For example, if you have complex templates in which you are using >3 nested layouts, you get to the point in which you are tired of passing through the $model. In these scenario's you would love to have an application service helping you out to supply the models that are found by the Controller.

fcaldarelli commented 7 years ago

I think that never model should be accessed by the view (except data passed from Controller or Widget). At this point, if I was thinking at it, I believe that there is an error in the implementation.

KnightYoshi commented 7 years ago

I'm going to reference what Plates does. You pass data from the controller to the view. The data is an associative array of data to be extracted to variables, the second is a string or associative array that's used to define what views that data gets extracted to. At no point does the controller really know about the views being loaded. If the view isn't loaded then the data is never extracted for a view.

The view object knows about the views being loaded (when it's rendering them).

SamMousa commented 7 years ago

If you pass a map telling which subviews get which variables that implies that the controller has full knowledge of which subviews are used right?

Why would this be an advantage?

Getting tired of passing things around is not an argument, @dynasource; you should pass things around instead of creating globals / semi-globals / locators.

KnightYoshi commented 7 years ago

@SamMousa You can't "pass things" to a layout view or a partial loaded in any other view than the view loaded by the controller. You have to set it on view->params then access it from a view that is rendering another view to pass it to.

For instance, I have a BaseController component that extends yii\web\Controller all of my controllers extend BaseController, because BaseController sets data that's used on every page such as dynamic site navigation. Instead of being able to give it to the View object and telling the View object which view that variable should be created for I end up stick it on view->params at which point I cannot have any other values on view->params called navigation without overwriting it. Where as if it were possible to tell the View object which view that's extracted for then I could do something like $this->view->param(['nav'=>$nav_model], ['layouts/main']) $this->view->param(['nav'=>$side_nav_model], ['site/sidenav']) Where there's no name conflict. Then also that data is only extracted once for the specified views and if I want to pass additional data to those views it's not necessary to keep calling the method over and over, call it once for an associative array of data for the scope specified.

dynasource commented 7 years ago

Getting tired of passing things around is not an argument, @dynasource; you should pass things around instead of creating globals / semi-globals / locators.

a matter of taste I guess. I'm not a fan of pushing $models through all the layouts. Why should one actually, as the View already knows of its existance.

SamMousa commented 7 years ago

@Knight-Yoshi now I am starting to see what you mean..

One (partial) solution could be to just override render() in your base controller and inject whatever params you need there. It doesn't handle the naming collisions though.

KnightYoshi commented 6 years ago

Also to add to this, one way I see it is that the controller doesn't know about the views being loaded within a view. However the view object does and you're just passing data to the view object itself that would create localized variables when and if those views are loaded.

It still follows the same process, of deferring what objects handle what.

SamMousa commented 6 years ago

Isn't this something that can be implemented relatively easily in either a View component or even a behavior attached to the view?

SamMousa commented 6 years ago

https://gist.github.com/SamMousa/df84a0c3d0a3db00e93b695ecbeafd15

Example implementation that might just work.