Skip to content

Conversation

heyanlong
Copy link

For details please refer to bug report 77326

@carusogabriel
Copy link
Contributor

Adding an exception is a BC IMHO 👎

@krakjoe
Copy link
Member

krakjoe commented Dec 27, 2018

CI is failing ... and I'm not convinced you have justified the broken compatibility, plenty of other reflection API methods do return false on failure, and that's what is expected ...

@KalleZ
Copy link
Member

KalleZ commented Dec 27, 2018

I think it should be everything or nothing. This for sure can't go into anything but PHP 7.4 at earliest.

Instead of changing them one by one, it should either all be throwing exceptions or returning false. In my fair opinion in regards to this specific one, I think returning false is perfectly suitable.

@nikic nikic added the BC Break label Dec 27, 2018
@petk
Copy link
Member

petk commented Dec 27, 2018

Since this is opened, wouldn't be exceptions way to go into the future for PHP next versions? For example, when using strict types (way where all programming goes more or less sooner or later), usually one might expect that certain function returns certain value. And if the returned value type is array like in this particular case, returning false from some method is what is unexpected actually. And wrapping core functions is then the only way to be completely sure that certain function returns certain type. Returning false is even difficult to describe in the php documentation itself - mixed or with current types state ?array. I might be wrong though and there is some better practice.

@heyanlong
Copy link
Author

Sorry, if it is not bug, please close this request

@heyanlong heyanlong closed this Dec 27, 2018
@KalleZ
Copy link
Member

KalleZ commented Dec 27, 2018

@petk it would be for sure be the way to go for the future. I just think that we might as well do it for everything at the same go, instead of doing it gradually

@KalleZ
Copy link
Member

KalleZ commented Dec 27, 2018

@heyanlong the report is perfectly valid, but like I said, we should consider doing it for all the methods instead of just doing them one by one

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

Successfully merging this pull request may close these issues.

6 participants