Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with
or
.
Download ZIP

Loading…

[Enhancment] Allow model to change the name used on forms. #470

Closed
ronaldfenner opened this Issue · 38 comments

8 participants

@ronaldfenner

Currently when using the Active elements for a form generation they use the class name of the model. It would be nice if we could specify a name to be used rather than the class name. Reason for this is where you might have several models in different modules that come in through a generic ajax action.
The action takes care of identifying the module and then calling the modules handler for the action. By being able to override the class name used on the form the generic action can perform various checks for the form data being there and not have to bother continuing to locate the module especially if the modules name is included as a hidden field. In my case i'm storing an id for a record in the database that actually contains the module the form belongs to.

Implementation would be adding a new function to CModel that by default returns the class name of the instance. Change CHtml's resolveName function to call the new function to get the name to use on the form. Subclasses of CModel would then be able to override the CModel's function to change the class name for the form.

From a security standpoint this allows us to also hide the exact name we use in the application for our models. It would also allow us if we have a really long model name to use a shorter name on the form.

@ronaldfenner ronaldfenner referenced this issue from a commit in ronaldfenner/yii
Ronald Fenner Added formName to CModel and changed CHtml::resolveName to use it whi…
…ch implements Enh #470.
c607543
@mdomba
Collaborator

I would like to see this in the core, nice improvement.

@qiangxue , @samdark opinions? Maybe a better name for the method formName() ?

@qiangxue
Owner

This is a good move. But I have some concerns.

  • If we want to use the same type of model for two forms on the same page (e.g. two login forms), how should we differentiate their formName?

  • How to deal with namespaced classes? If we choose to support formName, I suggest we do basename(get_class($this)) as default formName.

  • There are possibly other places that need to be updated as well, e.g. code generated by Gii.

@mdomba
Collaborator

Like @phpnode and @cebe commented on the pull request... this is more a "prefix" then a form name... any better name for this method?

and there are CForm::loadData() and CACtiveForm::validate that use get_class() currently, so this should be adjusted too.

Because of the problem that @qiangxue explained - "same model for two forms on same page" - maybe a set/get method would be better than overriding so that we can to something like this in the controller

$model1 = new Model;
$model2 = new Model;

$model1->formName = 'FormPrefixForFirstForm";
$model2->formName = "FormPrefixForSecondForm";

@samdark
Owner

From security standpoint this change will not add anything. Security through obscurity isn't useful.

@samdark
Owner

Also I don't like the fact that form-specific method is added to the model.

@mdomba
Collaborator

@samdark it's not about security but a way to differentiate the attribute names if the same model is used more than once or to just use something else as a prefix for the attribute name then the class name.

@mdomba
Collaborator

This could be seen as something else then a formName... this can be a model ID or NAME or HASH

it can be seen as a way to differentiate two objects created by the same class... I see the usability of using it in the active forms... but maybe can be used elsewhere too

@phpnode

it would also be nice to be able to set this value to false, so that prefixing is turned off completely. This is useful for e.g. nicer URLs when submitting search forms. I agree a getter / setter is a better way to go

@ronaldfenner
@phpnode

@ronaldfenner your suggestion means this will only work when using CForm, I think your original solution is better, it just needs to be a getter / setter than you can set per model instance, and it needs a better name.

@samdark I think in practice it's fine to put this in the model, it's something that relates to individual model instances, not form instances. I think the alternatives would be restrictive.

@samdark
Owner

@phpnode yes, you're right. But I still think we should not name it like formName or formSomething.

@ronaldfenner ronaldfenner referenced this issue from a commit in ronaldfenner/yii
Ronald Fenner changes for #470 to make modelName a getter/setter
Changes made for calls to get_class where necessary.
ea45476
@ronaldfenner
@samdark
Owner

Yes, yiilite is generated automatically. No need to change it.

@ronaldfenner ronaldfenner referenced this issue from a commit in ronaldfenner/yii
Ronald Fenner #470 update docs to reflet modelName is for an alternative form name.
Also made changes for using null instead of false.
f5de735
@mdomba
Collaborator

As this is not the model name but a prefix... how about naming it modelPrefix?

How about adding one more helper method populateAttributes that would be a helper for easier code writing and reading

if(isset($_POST[get_class($model)]))
   $model->attributes=$_POST[get_class($model)];

So in the controller we would just have $model->populateAttributes(); for massive assignment

@cebe
Owner

@mdomba I think that should be discussed in a separate issue.
And why is it a prefix? It is the name for the model to be used instead of class name.

@ronaldfenner
@mdomba
Collaborator

I'm just thinking to not confuse users... as class_name would give one value but modelName would give another one if set manually... that's why I would rather not call it name as we are not changing the model/class name (one model is one class)

If prefix is not good maybe call it as simple as tag?

$model->tag -> very simple, very short, has it's meaning

About the additional method, I don't see why not discuss it here as it's related to this addition, but I agree we can open a new discussion for that after we solve the initial implementation for this.

@phpnode

@mdomba tag is really confusing imho, and will likely break quite a lot of people's code, i personally have models and behaviors with getTag() methods that refer to completely different functionality. Tag is also used a lot in other parts of the framework to refer to html tags, so it becomes even more confusing, especially as we'd call this in CHtml itself.

@ronaldfenner
@mdomba
Collaborator

right, tag is not a good choice as it means something else... I personally dislike to add the word "model" to this why write "model" two time like

$model->modelXXX

why not just

$model->XXX

formName is not good too we already wrote that above... so we just need to find a good name for this...

@qiangxue
Owner

I think either modelName or modelID is fine. Note that this IS a property of a single model instance, not the whole model class. The name tag or any other simple names are not good because it may cause conflict with model attribute names.

@qiangxue
Owner

Note that one of the main reasons for this enhancement is because we want to differentiate two instances of the same model class when they are used to build two forms.

@mdomba
Collaborator

Just one more suggestion from my part, if not good then let's go with @qiangxue idea of modelName or modelID

My idea is to call it "identifier"

$model->identifier

@qiangxue
Owner

Just found another place that may need to be changed: CActiveDataProvider line 82:

$this->setId($this->modelClass);

This probably should be changed to

$this->setId($this->model->getModelName());
@ronaldfenner ronaldfenner referenced this issue from a commit in ronaldfenner/yii
Ronald Fenner #470 Changed to use basename for namespaces handling and changed to u…
…se getter explicitly
b03e2d0
@qiangxue
Owner

How about uniqueId, like we are doing in CController? Let's brainstorm for a while to think out a good name. :)

@mdomba
Collaborator

"uniqueId" is very appealing... it's just what this is meant for... I like it the most of all other suggestions

@ronaldfenner
@cebe
Owner

@mdomba we need an other issue to not mix discussion and not loose overview, I have some arguments to say about your new method but that would mess up this discussion, please open a new issue for that.

uniqueId stands in some conflict to the primaryKey when we use ActiveRecord, so I would not vote for that.
It also is not really a name which we use to differentiate between two instances of a class, this is in case of AR done by primary key. We use this name to avoid conflicting, because we have the same thing in one scope.
Crazy hard to describe in english, what I mean. hope you got it :-)

@mdomba
Collaborator

not sure what you mean regarding the primary key... the controller unique id returns the controller ID prefixed with the module ID - http://www.yiiframework.com/doc/api/1.1/CController#getUniqueId-detail

@qiangxue
Owner

I agree with what cebe said. How about reverting back to formName? Anyway, this is mainly used in form.

@mdomba
Collaborator

we made a trip around the world to get to the starting point :D

At this point any name is good for me, just to implement this and document it good.

@mdomba mdomba was assigned
@klimov-paul
Collaborator

As for me this implementation breaks the MVC.
Any HTML code belongs to the “View” layer or at least to “Controller” layer, but not to the “Model”.
Instead of allowing model to specify its HTML name, you should allow “CHtml”, “CForm” and “CActiveForm” to use internal parameter for this.
I mean method “CHtml::resolveName()” , should have parameter, which specifies the model HTML name:


public static function resolveName($model,&$attribute,$modelHtmlName=null) {
    …
    return ($modelHtmlName===null) ? get_class($model).'['.substr($attribute,0,$pos).']'.substr($attribute,$pos) : $modelHtmlName;
}

So “CActiveForm”, “CForm” and so on should pass their internal setting to this method (and methods, which use this method).

So widget creation will look like:


beginWidget('CActiveForm', array(
    'modelHtmlName'=>'MyModelName',
…

Such solution will solve the issue, while keeping the MVC.

No offence, but if we will evolve current suggested solution, the next step will be removing “CActiveForm” and allowing “CModel” to generate HTML for its attribute inputs on its own:


$model->labelEx(‘some_attribute’);
$model->textField (‘some_attribute’);
@samdark
Owner

@klimov-paul makes sense.

@mdomba
Collaborator

@klimov-paul note that this is not about HTML code... it's about a unique model representation (formname, modelname, modelId, whatever we name it)...

This is not used only in the view for rendering the form, but even in the controller to validate the models... in your example if you set the modelHtmlName when calling the CActiveForm the controller would not know what is the model name to make for example the massive assignements...

@klimov-paul
Collaborator

I am disagreed.
This issue IS about an HTML, unless you can found an example of its usage inside the console application.
This issue has risen because the poor MVC layers separation: you allowed view and controller be driven by the model, while the controller should drive the model and the view.
When controller setup model attributes from the POST it MUST use the name, which is used inside the HTML form. Actually a controller should remember this name: name of the key inside the POST and name of the input inside HTML.

In my example you can create a field inside the controller, which should specify the name, which is used to client-server data transfer:


class MyController extends CController {
    public $itemModelHtmlName = ‘ItemHtmlForm’;

    public function actionAdd() {
        …
        $model->attributes = $_POST[$this->itemModelHtmlName];
    }

}

Than you can use this property inside the view file and pass it to the widget:


$form=$this->beginWidget('CActiveForm', array(
    'modelHtmlName'=>$this->itemModelHtmlName,

View and controller should work around the model, not through it!

The problem is the data transfer form should be an artifact at the controller layer, should have representation at the view layer and fill up the model.


class MyController extends CController {
    public $itemModelHtmlName = ‘ItemHtmlForm’;

    public function actionAdd() {
        …
        $form = $this->getItemForm();
        $model->attributes = $form->fetchDataFromPost();
    }

}

// View file
….
$form = $this->getItemForm();
$form->textField (‘some_attribute’,$model);

@samdark samdark closed this
@mikehaertl

Well, if we don't allow it in the model, then there should at least be something instead of no solution at all. I don't really agree to @klimov-paul: MVC is nice as a general rule of thumb but should not be taken to such an extreme that it's like the holy grail no one can ever touch: If there's a simple solution without any obvious harm and it breaks MVC, i'd go for it!

I would take this issue here even further: In some scenarios i want my input fields to be named query instead of SearchForm[query]. Simply because i may have a search form and want nice user friendly URLs after submission. It should be up to me as a developer if i want to change that - i'll know what i'm doing. Currently there's no way at all. Everything is hardwired into CHtml. I could live with a solution like $model->getInputName('query'), which i could override in some models if i wanted to.

I know, that this is not pure MVC - but it's a pragmatic solution. And similar to what we already do with form labels.

The only other solution i could think of, is to use some sort of helper class which i can feed into all CHtml::active* methods, which craetes form input names for me. But this sounds like overkill.

@klimov-paul
Collaborator

@mikehaertl, you may wonder, but this entire issue has risen, because Yii has poor MVC.
There is no explicit layer of "View", which should handle the form input names.
"View" layer is mixed up with the controller and split around static helper classes.
If the CHtml was an application component, which can be extendable and replacable, you would be able to override method "CHtml::resolveName()" in your project and design it as you like, even implementing the "Form name" approach.

At its born Yii framework has no care about the input names and this was harcoded in its architecture inside "CHtml".
Now we can not just make framework to "care" without changing its architecture.

At the moment you can alwasy walk around your issue extending the "CActiveRecord" class, overridding its input creating methods using lower level of "CHtml" functions.

@mikehaertl

@klimov-paul I agree that a View layer would make a lot of sense. As far as i know Yii 2.0 will implement such a view class. I also agree that the root of these problems lies in CHtml. In my opinion this class is way too complex - it should focus on HTML rendering only and not be interwoven with Yii's form handling so much (read: should not have all these active*() methods).

All form related mechanics could be moved to a HtmlForm widget or something. I once described it in a forum post here

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.