yiisoft / yii

Yii PHP Framework 1.1.x
http://www.yiiframework.com
BSD 3-Clause "New" or "Revised" License
4.85k stars 2.28k forks source link

change resolveName and getIdByName to replace namespce "\" #129

Closed qiangxue closed 11 years ago

qiangxue commented 12 years ago

in yii 1.1.8 CGridView has a error when use the ajax filter sometimes if the class name has namespace, such as "\User\Model\Test";

It's better to be:

<?php
class CHtml {

public static function resolveName($model,&amp;$attribute)
    {
        $className = str_replace('\\', '_', get_class($model));
        if(($pos=strpos($attribute,'['))!==false)
        {
            if($pos!==0)  // e.g. name[a][b]
                return $className.'['.substr($attribute,0,$pos).']'.substr($attribute,$pos);
            if(($pos=strrpos($attribute,']'))!==false &amp;&amp; $pos!==strlen($attribute)-1)  // e.g. [a][b]name
            {
                $sub=substr($attribute,0,$pos+1);
                $attribute=substr($attribute,$pos+1);
                return $className.$sub.'['.$attribute.']';
            }
            if(preg_match('/\](\w+\[.*)$/',$attribute,$matches))
            {
                $name=$className.'['.str_replace(']','][',trim(strtr($attribute,array(']['=&gt;']','['=&gt;']')),']')).']';
                $attribute=$matches[1];
                return $name;
            }
        }
        return $className.'['.$attribute.']';
    }

}

Migrated from http://code.google.com/p/yii/issues/detail?id=2996


earlier comments

alexander.makarow said, at 2011-12-04T01:38:09.000Z:

Can you provide code to reproduce the issue?

alex.the.mrix said, at 2011-12-12T03:57:35.000Z:

Similar problem 1. User models namespace is "application\models\user\User" 2. CHtml::activeTextField($user, 'name') generates text field with ID attribute "application\models\user\User_name" 3. HTML validator (http://validator.w3.org/check) says 'Line 97, Column 63: character "\" is not allowed in the value of attribute "id"'

mdomba said, at 2011-12-16T14:41:55.000Z:

The class name is returned by resolveName() so that we can use it in the controller like: $model->attributes = $_POST[User]; In the case that the model is app\models\user\User.php, and we add the conversion of \ to _ the model class would become app_model_user_User instead of User, so this solution is not good as it would mean that developers has to manually ask for the underscore model like: $model->attributes = $_POST[app_model_user_User]; To solve this... how about adding/using a function that would return just the model name even if namespaces are used. The function (from php doc comments) would be: then instead of using get_class() inside the resolveName we would use get_class_name()

qiang.xue said, at 2012-01-01T03:37:10.000Z:

set for 1.1.10 milestone

qiang.xue said, at 2012-01-01T03:37:36.000Z:

set for 1.1.10 milestone

andy.s.kos said, at 2012-01-11T12:12:28.000Z:

I started to use namespaces in Yii and I would like to see this issue fixed as soon as possible, because generated input names (and probably ids?) are just ugly.

Ragazzo commented 12 years ago

@mdomba so if it was 5 moths ago why it does not solved yet? Could my solution be applied in this case or not? As i see this problem is still open.

mdomba commented 12 years ago

Yes this problem is still open that's why i closed the other issue to not have duplicates.

It's not solved probably because nobody else did participate to this discussion by giving ideas, testing different solutions,.... so this issue had/has a low priority.

Your solution is to remove the namespace part and use only the last part (i.e. class name)... in theory it can happen that a user has two same name classes in two different namespaces (that's why they are used)... so in your solution if those two classes would be used on the same form there would be a problem.

Ragazzo commented 12 years ago

@mdomba yes, i was thinking about this problem (2 same class names in 2 different namespaces), but i don't know how to fix it yet. I think that if the user has this problem, the only way to fix it for user is to use 'name' attribute in CHtml inputs methods, like this

$form->labelEx($userForm, 'name',array('for' => 'UserForm[name]')); ?>

Because if user uses 2 models with same names he already knows about that he has 2 models with same names, and i think it must be work of user to resolve name collision. Because suggested method resolveModelName can not know if the user using in some place of code 2 same models or not, or more models with same name. What do u think of this?

mdomba commented 12 years ago

I would like to hear others for suggestions/ideas...

btw. your code could be simplified to just

private function resolveModelName($model)
{
   $c = explode('\\', get_class($model));
   return end($c);
} 
bwoester commented 12 years ago

I don't think stripping the namespace is a good idea (because of possible collisions). Instead, maybe it would be possible to turn unqualified names into qualified ones behind the scenes?

For the problem with the id attribute: Guess it's better to convert '\' to '-' instead of '\' to '_', because underscores are valid characters for classes. '.' or ':' would also be possible, but I read that jQuery has problems with IDs that contain such characters.

Ragazzo commented 12 years ago

@bwoester

turn unqualified names into qualified ones behind the scenes?

for what?can u please write some code of this. @mdomba Yes, hm) great refactoring )) btw. your code could be simplified to just

function ($model) { return end(explode('\\', get_class($model)));} 

just joke )

mdomba commented 12 years ago

@Ragazzo

I now you are joking... but it's a matter of fact that the code I wrote could not be simplified more ;) Try it :D

mdomba commented 12 years ago

@bwoester regarding your suggestion please read my comment above under "mdomba said, at 2011-12-16T14:41:55.000Z:"

Ragazzo commented 12 years ago

@mdomba i've tried. it's worked.

class A {}
$model = new A;
$nameResolver =  function ($model) { return end(explode('\\', get_class($model)));};
var_dump($nameResolver($model));

but it's little offtop. I want to know if this issue will be solved in near future? just dont want to write model name for each attribute, but have to.

mdomba commented 12 years ago

It will be solved when we find a solution that works for all or most of all use cases... we cannot make a solution that works just for few of developers into the framework, that's why I'm waiting for others opinions on this.

O.T. yes it works but it generates a warning that you don't see because you don't have E_NOTICE enabled. The warning is generated because end() needs a variable passed by reference.

Ragazzo commented 12 years ago

@mdomba yes, i understand, other opinions is important for me too. ok. O.T. php 5.3.3 error_reporting = E_ALL but yes you right end() is need to pass var by reference. will no more offtop)

bwoester commented 12 years ago

@Ragazzo

turn unqualified names into qualified ones behind the scenes?

for what?can u please write some code of this.

I can't help with any actual code, but I can try to explain a bit more what I meant:

The whole issue is, that namespaced classes currently produce invalid html markup (id="application\models\user\User_name"). One suggestion to address this issue is to strip namespace part. I fear that this might introduce problems due to collisions (by the way: how will yii 2.0 handle this?). The other suggestion is to somehow encode/replace the invalid backslashes. Here I prefer to use dashes instead of underscores, again to prevent clashes with other classes (underscores sometimes are used in class names, dashes not I think). One argument against this approach is the lengthy statements needed to work with such class names ($model->attributes = $_POST[app_model_user_User];). This is the place where I think yii could eventually help a bit behind the scenes. It's just an idea, as I've said, unfortunately, I don't have code for this. Isn't there a way to allow the developer to write:

$model->attributes = $_POST['User'];

even if the originally submitted data is named "app_model_user_User"? Maybe the array can be prepared and filled with alias keys, similar to what CUrlManager does?

Or maybe introduce a helper:

$model->attributes = $model->attributesFrom( $_POST );

So this is basically what I meant: don't force the user to write fully qualified names to retrieve data, but still use fully qualified names internally to prevent collisions.

alex-code commented 12 years ago

What is the likelihood of having two namespaced models with the same name used in the same form?

Possible workaround, Based off bwoester comment,

public static function resolveName($model,&$attribute)
{
    $class = str_replace('\\', '-', get_class($model));

    if(($pos=strpos($attribute,'['))!==false)
    {
        if($pos!==0)  // e.g. name[a][b]
    return $class.'['.substr($attribute,0,$pos).']'.substr($attribute,$pos);
        if(($pos=strrpos($attribute,']'))!==false && $pos!==strlen($attribute)-1)  // e.g. [a][b]name
        {
    $sub=substr($attribute,0,$pos+1);
    $attribute=substr($attribute,$pos+1);
    return $class.$sub.'['.$attribute.']';
        }
        if(preg_match('/\](\w+\[.*)$/',$attribute,$matches))
        {
    $name=$class.'['.str_replace(']','][',trim(strtr($attribute,array(']['=>']','['=>']')),']')).']';
    $attribute=$matches[1];
    return $name;
        }
    }

    return $class . '[' . $attribute . ']';
}
public function attributesFrom(array $from)
    {
        $class = str_replace('\\', '-', get_class($this));
        $this->setAttributes($from[$class]);
    }
bkuhl commented 12 years ago

I've documented some details on the challenges faced with doing this, as well as my solution: http://blog.benkuhl.com/2012/11/how-to-yii-namespaced-forms-with-clean-field-idsnames-in-v1-1/

Ekstazi commented 11 years ago

Changes for this issue and similars(sorry for my bad git knowledge) CHtml:

 public static function resolveName($model,&$attribute)
 {
  $className=end(explode('\\',get_class($model)));
  if(($pos=strpos($attribute,'['))!==false)
  {
   if($pos!==0)  // e.g. name[a][b]
    return $className.'['.substr($attribute,0,$pos).']'.substr($attribute,$pos);
   if(($pos=strrpos($attribute,']'))!==false && $pos!==strlen($attribute)-1)  // e.g. [a][b]name
   {
    $sub=substr($attribute,0,$pos+1);
    $attribute=substr($attribute,$pos+1);
    return $className.$sub.'['.$attribute.']';
   }
   if(preg_match('/\](\w+\[.*)$/',$attribute,$matches))
   {
    $name=$className.'['.str_replace(']','][',trim(strtr($attribute,array(']['=>']','['=>']')),']')).']';
    $attribute=$matches[1];
    return $name;
   }
  }
  return $className.'['.$attribute.']';
 }

CForm:

    public function loadData()
    {
        if($this->_model!==null)
        {
            $parts=explode('\\',get_class($this->_model));
            $class=end($parts);
            if(strcasecmp($this->getRoot()->method,'get'))
            {
                if(isset($_POST[$class]))
                    $this->_model->setAttributes($_POST[$class]);
            }
            elseif(isset($_GET[$class]))
                $this->_model->setAttributes($_GET[$class]);
        }
        foreach($this->getElements() as $element)
        {
            if($element instanceof self)
                $element->loadData();
        }
    }

CActiveForm:

    public static function validate($models, $attributes=null, $loadInput=true)
    {
        $result=array();
        if(!is_array($models))
            $models=array($models);
        foreach($models as $model)
        {
            $parts=explode('\\',get_class($model));
            $className=end($parts);
            if($loadInput && isset($_POST[$className]))
                $model->attributes=$_POST[$className];
            $model->validate($attributes);
            foreach($model->getErrors() as $attribute=>$errors)
                $result[CHtml::activeId($model,$attribute)]=$errors;
        }
        return function_exists('json_encode') ? json_encode($result) : CJSON::encode($result);
    }

CActiveDataProvider:

    public function __construct($modelClass,$config=array())
    {
        if(is_string($modelClass))
        {
            $this->modelClass=$modelClass;
            $this->model=CActiveRecord::model($this->modelClass);
        }
        elseif($modelClass instanceof CActiveRecord)
        {
            $parts=explode('\\',get_class($modelClass));
            $this->modelClass=end($parts);
            $this->model=$modelClass;
        }
        $this->setId($this->modelClass);
        foreach($config as $key=>$value)
            $this->$key=$value;
    }
cebe commented 11 years ago

@Ekstazi why not open a pull request? There is no problem with learning new things ;)

Ekstazi commented 11 years ago

I try, but without results yet

resurtm commented 11 years ago

@Ekstazi, feel free to contact with me via email or Skype. I could try to help you on creating pull requests. :smiley:

cebe commented 11 years ago

You might also want to read this:

https://help.github.com/articles/fork-a-repo https://help.github.com/articles/using-pull-requests

Ekstazi commented 11 years ago

https://github.com/yiisoft/yii/pull/1808

pgaultier commented 11 years ago

Well removing the namespace part is not a good idea.

Turning the \ to _ in resolveName like Qiang did solve first part of the problem.

To solve the other part (fetching data in $_POST easily) we should use late static binding :

add a method to CModel which return the "correct" class :

class CModel extends CComponent {
    /** cmodel code ***/
    /**
     * using late static binding, we can retrieve the real classname
     */
    public static function getResolvedName() {
        return str_replace('\\', '_', get_called_class());
    }
    /**
     * retrieve POST data
     */
    public static function getRawPostData() {
        return isset($_POST[static::getResolvedName()])?$_POST[static::getResolvedName()]:null;
    }
    /*** cmodel code ***/
}

As namespace and php late static bind are available in 5.3, there won't be any compatibility problem.

This way we can code this way (when using namesapce) :

/*** code ***/

if(isset($_POST[MyModel::getResolvedName()]) === true) {
    // handle post data
}

or in a easier way :

/*** code ***/

if( ($postData = MyModel::getRawPostData()) !== null) {
    // use the postdata stuff
}
klimov-paul commented 11 years ago

PR #2225 introduces fine solution, I just waiting for its rework.

pgaultier commented 11 years ago

Well this is the same for namespaces.

No namespaces - no lsb

This is why I thought solution was fine.

cebe commented 11 years ago

No namespaces - no lsb

thats true but it should still work when you do not use namespaces, the whole thing has to be compatible ;)

pgaultier commented 11 years ago

Well in this case, this should be easy to fix. I'll propose some code to keep compatibility

pgaultier commented 11 years ago

@klimov-paul, I have read lastDragon-ru proposal and I rewrote it(keeping credits for him of course).

You can check everything here : https://github.com/yiisoft/yii/pull/2282

If this proposal is not properly formatted, or if you prefer to wait for lastDragon-ru, just reject this pull request.

Cheers.

Ekstazi commented 11 years ago

Up