Declaration of CCookieCollection::add() should be compatible with that of CMap::add()

[twisted1919]

After a fresh update to 1.1.11 i get the following error when trying to login:

Declaration of CCookieCollection::add() should be compatible with that of CMap::add()

Seems the problem is of course in CCookieCollection::add() method, which currently looks like:

public function add($name,$cookie=null)

but changing its declaration to

public function add($name,$cookie)

seems to fix the issue.
However, i don't really know the other implications if changing the method declaration, maybe it'll break other classes.

[twisted1919]

Also, the declaration of CCookieCollection::remove() is wrong and it'll result in a fatal error.
This is a real mess overall, should be fixed asap.

[qiangxue]

Which PHP version are you using?

[twisted1919]

Apache/2.2.21 (Win32) mod_python/3.3.1 Python/2.5.4 PHP/5.2.17 Yii Framework/1.1.11

Sorry for not posting this, i was in a hurry when i posted the issue.

[qiangxue]

Thanks. Do you know what is wrong with remove()?

[twisted1919]

well, about remove, as long as the method declaration is not the same as the parent implementation, i can only assume it will result in a fatal error when the method is called.

[cebe]

seems when error_reporting is E_STRICT it is not allowed to overide a method and change default values for parameters.
http://stackoverflow.com/questions/3115388/declaration-of-methods-should-be-compatible-with-parent-methods-in-php#comment3197378_3115398

[qiangxue]

Did you actually try it? PHP only checks the number of required parameters.

[cebe]

See also this behavior: seems you can add params, but not remove any:
https://bugs.php.net/bug.php?id=46851

[suralc]

Atleast here with php 5.4-latest with E_ALL I don't see any warnings or errors, aslong as I don't remove or add any params.

[qiangxue]

add() is definitely problematic, but not remove() I think.

[cebe]

problem introduced in d3b0bc0, and yes, remove shouldn't be a problem.

[qiangxue]

I'm wondering if this will cause problem as long as the CHttpRequest.php file is included. If so, this is a big issue and we should fix it asap.

[cebe]

@qiangxue it only occurs, when you are trying to call the method.

[suralc]

Even if I can't reproduce this with 5.4(E_ALL) and 5.3(E_ALL | E_STRICT). I see two options:

Back out that change or change the signature of CMap::add().

I'm not sure what would be better, but setting second param of CMap::add() to null shouldn't brake code, as it should have been called with all params set prior to this change. Also this should not brake ArrayAccess, as offsetSet is still requiring two params.

Also this would allow users to override CMap::add in simliar way as it is done in CCookieCollection, which could be quite usefull in some scenarios.

[qiangxue]

No, we should definitely not change CMap::add() because it would break other child classes if they override this method.

[mdomba]

original issue that introduced this "problem" - #120

[suralc]

@qiangxue
Atleast this is not throwing any errors on my local machine (Win 7 with 5.4/5.3) on cli
So removing a default value should work. But as the original issue did not show up on my local machine (with E_ALL(5.4) set per ini and per code). This should not been taken as a yes.

<?php
// E_STRICT included in E_ALL in PHP 5.4
error_reporting(E_ALL);
class t{
    public function add($name, $value = null){
        return $name;
    }
}

class tt extends t{
    public function add($name,$value){
        return parent::add($name).$value;
    }
}
$tt = new tt;
var_dump($tt->add('Name ', 'Value')); // string(10) "Name Value"
$t = new t;
var_dump($t->add('Name2')); // string(5) "Name2"

[twisted1919]

@suralc - try with error_reporting(-1)

[qiangxue]

PHP 5.3 seems to be fine, but I was able to reproduce the problem in 5.2.

[suralc]

@twisted1919 @qiangxue
Maybe my environment is freaked up, but this works on PHP 5.3 and 5.4 for me.
On 5.2(just downloaded) I also fail ;(

D:\dev\www\php-5.2>php  < D:\dev\www\vhosts\tests\test\test.php

Strict Standards: Declaration of tt::add() should be compatible with t::add($nam
e, $value = NULL) in D:\dev\www\vhosts\tests\test\test.php on line 13
string(10) "Name Value"
string(5) "Name2"

[twisted1919]

@suralc well, it's obviously a 5.2.x problem then, and as long as Yii 1.1.x branch is targeted to 5.2.x this will cause allot of issues :)

[suralc]

1eb01b3 and 87f17a0 will revert add as it was before. And introduce a new method allowing to add a cookie without specifing the name twice.

This method would also allow to retrieve a cookie on a safeway, without having to worry about fatals.

I'll open seperat pull requests for this.

[suralc]

@twisted1919 I opened #1091 and #1092 to solve this.

Do you have a better solution to fix this issue?

I'd go with someting like (I got to this after I opend the pr, so I let them open, but presenteing this to discussion)

<?php
function add($cookie, $cookieObject)
{
    if($cookie instanceof CHttpCookie)
         //ignore second paramterer, but be sure that it was set
}

[twisted1919]

@suralc
If it were for me, i'd drop this feature and go back in the way things were in the previous version.
I don't really have a solution for now, maybe @qiangxue or any other core developer can come up with something.

Anyway, in the future, tests should be made on multiple php version (5.2.x/5.3.x branches) with strict error reporting so that we can avoid things like this.

[suralc]

@twisted1919

Well, this was added before travis support was there.

I'd go for the solution I posted above, this would fix the issue, and would only mean to introduce a minor bc issue (which most will not notice, as 1.1.11 is young)

[qiangxue]

I reviewed your PR's. I think it's better we simply revert the whole changeset.
$cookies->add($cookie,false);
is not any better than the original
$cookies->add($cookie->name, $cookie);

[suralc]

Sadly,

it would still eleminate the issue of mismatching the names, even if this is something that just should not happen. I'll reopen #1091.

Changelog and update should be adjusted I guess.

[suralc]

@qiangxue Booth open prs should fix the issue. I'd personally prefer #1093 over #1091, but thats not mine to decide.

[mdomba]

Closing this issue as @qiangxue already merged the #1091

Unfortunately PHP5.1 restricts this solution and there is no other way to solve the "send name only once" in addition makes no sense to have a second parameter that can be anything (#1093).

Only solution that makes sense is to revert the change.