This repository has been archived by the owner. It is now read-only.

Add error handling for __get and __call methods. #1517

Closed
wants to merge 1 commit into
from

Conversation

Projects
None yet
3 participants
@realityking
Contributor

realityking commented Sep 6, 2012

Adding a __get or __call method may mask errors about unexisting method or properties if we don't do our own error handling. This pull should provide appropriate errors for all of these.

Note: The test results will show one error in JTableNested that I suspect is an actual bug but I'm not sure how to fix.

@pasamio

This comment has been minimized.

Show comment Hide comment
@pasamio

pasamio Sep 23, 2012

Contributor

I'm guessing it should be $this->_unlock() instead since that is a valid method of JTable.

I found where it was added to the file here:
017f8f7#L26L367

Even at that commit, JDatabaseQuery doesn't support lock and for some reason it's having a database object fed into it as well which seems a bit strange. Given almost every other occurrence in the file has been removed I think swapping this back to $this->_unlock() is probably the correct change.

Contributor

pasamio commented Sep 23, 2012

I'm guessing it should be $this->_unlock() instead since that is a valid method of JTable.

I found where it was added to the file here:
017f8f7#L26L367

Even at that commit, JDatabaseQuery doesn't support lock and for some reason it's having a database object fed into it as well which seems a bit strange. Given almost every other occurrence in the file has been removed I think swapping this back to $this->_unlock() is probably the correct change.

@pasamio

This comment has been minimized.

Show comment Hide comment
@pasamio

pasamio Sep 23, 2012

Contributor

As an aside, I like this change. I've been burnt a few times by calling the wrong method somewhere and getting a null back when I expected it to be something else.

Contributor

pasamio commented Sep 23, 2012

As an aside, I like this change. I've been burnt a few times by calling the wrong method somewhere and getting a null back when I expected it to be something else.

@LouisLandry

This comment has been minimized.

Show comment Hide comment
@LouisLandry

LouisLandry Oct 9, 2012

Contributor

@realityking love the direction on this. Can you get it cleaned up and rebased so we can merge it? Do we still have an issue figuring out the problem with the unit test failure? Please re-open it when it is cleaned up. Thanks!

Contributor

LouisLandry commented Oct 9, 2012

@realityking love the direction on this. Can you get it cleaned up and rebased so we can merge it? Do we still have an issue figuring out the problem with the unit test failure? Please re-open it when it is cleaned up. Thanks!

@LouisLandry LouisLandry closed this Oct 9, 2012

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