Skip to content
This repository has been archived by the owner on Jan 8, 2020. It is now read-only.

replacing array_key_exists with faster isset calls #6562

Closed
wants to merge 3 commits into from

Conversation

pine3ree
Copy link
Contributor

should we also change has checkers with isset() || array_key_exists() ?

weierophinney and others added 2 commits August 11, 2014 10:50
should we also change has checkers with isset() || array_key_exists() ?
@Ocramius
Copy link
Member

@pine3ree yes, it should be using || array_key_exists()

@Ocramius
Copy link
Member

Actually, never mind: if isset() fails, then null is returned, so || array_key_exists() is not required.

@pine3ree
Copy link
Contributor Author

When using getter it's always preferable using isset alone, since it covers all the cases.
With hassers I tend to use both (isset 1st): an array may "have" an element for a particular key and that element can be NULL.

@Ocramius
Copy link
Member

Yeah, the patch is good as-is. The trailing spaces are still caught by the CS checks in the test suite though...

@pine3ree
Copy link
Contributor Author

oh, yeah...sorry. it always happend to me when editing directly on github (due to my obsession of having a blank line b4 return). There's still a couple of assert equal errors in tests/ZendTest/Feed/Reader/Entry/AtomTest.php:148
tests/ZendTest/Ldap/Converter/ConverterTest.php:104
in 5.4 - 5.5 - 5.6
will look into it.

@pine3ree
Copy link
Contributor Author

mmm shouldn't be related to form elements at all.

@Ocramius
Copy link
Member

@pine3ree yeah, build is ok for what is related to this PR now. No further action needed

@weierophinney
Copy link
Member

Actually... empty strings are valid labels in this case, and should not return null. Since isset('') returns false, array_key_exists() is the more appropriate call here.

@pine3ree
Copy link
Contributor Author

@weierophinney

??

empty strings array values returns TRUE

$arr = array(
    'key' => '',
);

var_dump(isset($arr['key']));

echoes bool(true)

@Ocramius
Copy link
Member

@weierophinney you probably misread this one :-) Re-opening

@Ocramius Ocramius reopened this Aug 12, 2014
@Ocramius Ocramius removed the wontfix label Aug 12, 2014
@Ocramius Ocramius self-assigned this Aug 12, 2014
@pine3ree
Copy link
Contributor Author

@weierophinney

Honestly, i still don't understand what you mean or where isset('') does apply.

I just changed the getters for labelOptions and attributes arrays with a very common optimization and I am applying isset to arrays not to strings that can be empty.

since i have only used isset like this:

isset($arr[$key]);

applying your use case that would mean $arr[$key] could be '', which returns TRUE if checked with isset.

Can you give me any example of the case you had in mind? What am I missing?

kindly.

@Ocramius
Copy link
Member

@pine3ree it was just a PEBKAC, not intentional :-)

@pine3ree
Copy link
Contributor Author

don't we all suffer from it? :-)
...meaning that I was so concentrated on another project that I read your reply to weierophinney as a weierophinney reply to me, and i replied back....

@Ocramius Ocramius added this to the 2.3.4 milestone Nov 22, 2014
Ocramius added a commit that referenced this pull request Nov 22, 2014
@Ocramius Ocramius closed this in b6926ef Nov 22, 2014
@Ocramius
Copy link
Member

This was merged manually, thanks @pine3ree!

master: b6926ef
develop: c4709ca

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