Fixes #959: CConsoleApplication: bug where non-lowercase keys cannot be found in $commandMap fixed #1423

Merged
merged 4 commits into from Sep 27, 2012

Projects

None yet

5 participants

@resurtm

Fixes #959: CConsoleApplication: bug where non-lowercase keys cannot be found in $commandMap fixed

Rework of #1421.

@mdomba

it's more readable if you put the braces on the foreach...

Yii Software LLC member

wouldn't be better (more readable, faster) if if(isset($this->commands[strtolower($name)])) would be used ?

I think you don't understand the whole problem. Suppose we have the following command map:

'commandMap'=>array(
    'testaction1' => 'application.components.TestActionCommand',
    'testAction2' => 'application.components.TestActionCommand',
    'TestAction3' => 'application.components.TestActionCommand',
    'Testaction4' => 'application.components.TestActionCommand',
),

Last three entries will not work with your suggestion, because hash array access operator [] is case sensitive by PHP design. Even if we make $name lower cased the isset($this->commands[strtolower($name)]) expression for last three entries of the commandMap would be false.

Sample command: ./yiic Testaction4
Lowercased $name: testaction4
And surely, there is no such key as testaction4 in commandMap.

My solution offers something that can be called 'case insensitive hash array access operator'. This keeps BC and solves the problem.

it's more readable if you put the braces on the foreach...

Right. Will fix it.

Yii Software LLC member

But would there be a case for same command with different case like testcommand1 and TestCommand1

What I suggested was to first check for the lowercase and only if that is not found then check for "case insensitive hash array access"...

What I suggested was to first check for the lowercase and only if that is not found then check for "case insensitive hash array access"...

Sorry, i misunderstood your point of view. :) Yes, that makes sense and i agree with you. Will do that soon.

Yii Software LLC member

There are 2 other possible solutions I can think of... maybe they could be tested to find the optimal one (good readability and performance)

First is to use array_change_key_case() - http://php.net/manual/en/function.array-change-key-case.php, when assigning the commandMap
and the second is to use array_search(strtolower($search), array_map('strtolower', $array));

Perf. tests: http://codepad.org/kER3Cl33 http://codepad.org/ujy69e9Y http://codepad.org/wxoPHJeS — i'm going with array_change_key_case().

@cebe cebe was assigned Sep 18, 2012
@samdark
Yii Software LLC member

I think it's OK to merge it. The only thin is extra : in changelog line.

@kidol kidol was assigned Sep 27, 2012
@samdark samdark was assigned Sep 27, 2012
@samdark samdark merged commit e5e421d into yiisoft:master Sep 27, 2012
@samdark
Yii Software LLC member

Merged. Thanks.

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