Accounting for ReflectionClass::getName() Bug #808

Merged
merged 1 commit into from Jan 31, 2013

Conversation

Projects
None yet
4 participants
Contributor

ericcholis commented Jan 30, 2013

Related to https://bugs.php.net/bug.php?id=63256.

A ReflectionException can be thrown due to the ReflectionClass name getter incorrectly returning false.

I'm unsure of the various OS and PHP version scenarios where this is possible, but I discovered on this on Windows 7, running IIS 7.5 and PHP 5.4.2.

Member

blainesch commented Jan 30, 2013

Can you add a test with a valid use case?

Contributor

ericcholis commented Jan 30, 2013

Honestly, I'm not sure how I would mock that. Here's a sample of what is happening:

class Test {
    public $reflection = null;

    public function test() {
        $this->reflection = new ReflectionClass(get_class($this));
    }
}

$test = new Test();
$test->test();

//Returns false
echo $test->reflection->getName();

//Returns "Test"
echo $test->reflection->name;

So far, I've tested it using http://www.ignite.io and MAMP running 5.4.4. Windows seems to be the only one acting up.

Member

blainesch commented Jan 31, 2013

QA fixes may conflict with #807 since we both did qa on Inspector.

Contributor

ericcholis commented Jan 31, 2013

I can close this and wait for #807 to pass and be merged.

Seems that the impact of this PHP bug is limited. I'll re-submit once your Pull Request has been merged to dev.

ericcholis closed this Jan 31, 2013

Member

blainesch commented Jan 31, 2013

I wouldn't have closed it. If yours got accepted first I could have just rebased your changes. Can you re-open? Mine's not a bugfix so I assume your pr has a higher priority.

ericcholis reopened this Jan 31, 2013

Contributor

jails commented Jan 31, 2013

Looks like to be more a conflict with APC: https://bugs.php.net/bug.php?id=61384
Since this bug only occurs for a couple of PHP + APC depending on versions, the best would be to test this bug on a recent PHP with the last APC to see if this patch is really needed.

Contributor

ericcholis commented Jan 31, 2013

Agreed @jails. I upgraded my windows box to PHP 5.4.11, which still experienced the error. Upgrading to APC 3.1.13 (beta) did the trick. Beta was installed to mirror my production boxes.

I don't mind having this closed. @tmaiaroto was the only other person to notice this error, back in June (http://lithify.me/bot/logs/li3-core/2012-06-15)

Owner

nateabele commented Jan 31, 2013

I'm okay with merging it. All other things being equal, if we can make a small change to prevent a few people from getting obscure errors, I'm all for it.

Just do me a favor and rebase off the current dev branch and squash your commits. Also, be sure to check your whitespace. There's a trailing space after && (might have been a li3_quality false positive, just ignore it), there's a newline at the end of the file, and ? :' should be ?:.

Thanks!

Owner

nateabele commented Jan 31, 2013

Also, I'm really not sure how you'd write a test for this, since it's dependent on a version conflict. At best, we could do additional Travis builds targeting the affected PHP/APC versions. @blainesch Worth it to you?

Member

blainesch commented Jan 31, 2013

Not for a specific version. If all the current tests pass I'd say it's fine.

Can we get rid of the qa commit though, @ericcholis? Anther commit by @jails fixed the need for it #809

Contributor

ericcholis commented Jan 31, 2013

I thought something got screwed up when I rebased and squashed, looks fine here though.

Owner

nateabele commented Jan 31, 2013

Yeah, just try to retain a meaningful commit message next time. ;-)

@nateabele nateabele added a commit that referenced this pull request Jan 31, 2013

@nateabele nateabele Merge pull request #808 from ericcholis/patch-1
Accounting for ReflectionClass::getName() Bug
5bf799c

@nateabele nateabele merged commit 5bf799c into UnionOfRAD:dev Jan 31, 2013

1 check passed

default The Travis build passed
Details

ericcholis deleted the unknown repository branch Jan 31, 2013

Contributor

ericcholis commented Jan 31, 2013

Thanks @nateabele, Newb mistake, got stuck in my normal git flow and forgot that I was rebasing/squashing...no more commit history :-(

Owner

nateabele commented Jan 31, 2013

Yeah, not a big deal. Just something to keep in mind.

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