Skip to content
This repository has been archived by the owner on May 16, 2018. It is now read-only.

Fixed 'Undefined index' #695

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

ephemeralsnow
Copy link

Undefined index: xxx

ZendFramework-1.12.18/library/Zend/Controller/Router/Route.php:328
ZendFramework-1.12.18/library/Zend/Controller/Router/Route/Chain.php:118
ZendFramework-1.12.18/library/Zend/Controller/Router/Rewrite.php:410
ZendFramework-1.12.18/library/Zend/Controller/Front.php:911
ZendFramework-1.12.18/library/Zend/Test/PHPUnit/ControllerTestCase.php:194

@froschdesign
Copy link
Member

froschdesign commented May 12, 2016

@ephemeralsnow
Thanks for the PR! Can you demonstrate the problem with an unit test?

@ionux
Copy link

ionux commented May 12, 2016

Using isset() is faster than array_key_exists().

@ephemeralsnow
Copy link
Author

If the default value was null?

@ionux
Copy link

ionux commented May 12, 2016

Yes, but then your comment is confusing, imho:

// Empty variable? Replace with the default value.

In that case if the default value is null anyway, assigning the null character terminator "\0" might be a good idea to use if you want isset() to pass the check.

But, to my original comment, since isset() is a language construct it's much faster in benchmarks. Plus there's a quirk in the behavior of array_key_exits() that you'll have to watch out for:

Note:
For backward compatibility reasons, array_key_exists() will also return TRUE if key is a
property defined within an object given as array. This behaviour should not be relied
upon, and care should be taken to ensure that array is an array.

I'm not picking on your commit, though - I just had a very similar issue at work this week so the problem space is really fresh on my mind. ;)

@ephemeralsnow
Copy link
Author

Thank you for your reply. It was fixed.

@ephemeralsnow
Copy link
Author

@froschdesign
This is the result you have not applied the fix.

[root@5ff87e6bb8e0 tests]# /usr/local/php5621/bin/php /usr/local/php5621/bin/phpunit-5.3.4.phar Zend/Controller/AllTests.php
PHPUnit 5.3.4 by Sebastian Bergmann and contributors.

...............................................................  63 / 834 (  7%)
............................................................... 126 / 834 ( 15%)
...................................SSSSSSS..................... 189 / 834 ( 22%)
............S.S.SS............................................. 252 / 834 ( 30%)
............................................................... 315 / 834 ( 37%)
.......S....................................S.................. 378 / 834 ( 45%)
............................................................... 441 / 834 ( 52%)
............................................................... 504 / 834 ( 60%)
..................................................S....S....... 567 / 834 ( 67%)
............................................................... 630 / 834 ( 75%)
............................................................... 693 / 834 ( 83%)
............................................................... 756 / 834 ( 90%)
........................E...................................... 819 / 834 ( 98%)
......S........                                                 834 / 834 (100%)

Time: 2.8 seconds, Memory: 24.25Mb

There was 1 error:

1) Zend_Controller_Router_Route_ChainTest::testChainingStaticDynamicMatchToNoDefaults
Undefined index: bar

ZendFramework-1.12.18/library/Zend/Controller/Router/Route.php:328
ZendFramework-1.12.18/library/Zend/Controller/Router/Route/Chain.php:118
ZendFramework-1.12.18/tests/Zend/Controller/Router/Route/ChainTest.php:809
ZendFramework-1.12.18/tests/Zend/Controller/AllTests.php:62
ZendFramework-1.12.18/tests/Zend/Controller/AllTests.php:97

FAILURES!
Tests: 834, Assertions: 1804, Errors: 1, Skipped: 16.

Are there any other things you need?

@froschdesign
Copy link
Member

@ephemeralsnow

Are there any other things you need?

Yes, the test Zend_Controller_Router_Route_ChainTest ::testChainingStaticDynamicMatchToNoDefaults!
It does not help when the test is on your machine. We need this also in this repository.

@ephemeralsnow
Copy link
Author

Tests were added after I got the first comment.
0e32fe6

@froschdesign
Copy link
Member

@ephemeralsnow

Tests were added after I got the first comment.

Ah, sorry. I've overlooked that.

@froschdesign froschdesign added this to the 1.12.19 milestone May 26, 2016
@froschdesign froschdesign removed this from the 1.12.19 milestone Jul 7, 2016
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants