Skip to content

Conversation

smalyshev
Copy link
Contributor

No description provided.

@smalyshev
Copy link
Contributor Author

Lots of test fails currently because master is messed up :(

@Ocramius
Copy link
Contributor

Any details on the affected classes in this PR? Can it be limited to internal classes with private properties only?

@smalyshev
Copy link
Contributor Author

@Ocramius I don't see how you could make such limitation. Would you scan each class each time to see if there are no private properties?

@Ocramius
Copy link
Contributor

Obviously not, I'd rather suggest having a whitelist of allowed classes
instead
On Apr 28, 2015 8:00 PM, "Stanislav Malyshev" notifications@github.com
wrote:

@Ocramius https://github.com/Ocramius I don't see how you could make
such limitation. Would you scan each class each time to see if there are no
private properties?


Reply to this email directly or view it on GitHub
#1253 (comment).

@jpauli
Copy link
Member

jpauli commented May 12, 2015

A whitelist is not a good idea. It has to be statically coded, then updated when needed, etc...
I prefer we forbid rebinding for all internal classes, as this may lead to undesired behaviors

@jpauli
Copy link
Member

jpauli commented May 12, 2015

Merged

@jpauli jpauli closed this May 12, 2015
@nikic
Copy link
Member

nikic commented May 12, 2015

I'm uneasy about this change - I feel like this might have similar fallout to the initial implementation of the unserialization changes. @Ocramius Can you say if / how this will affect you?

@Ocramius
Copy link
Contributor

@nikic it won't affect me, but users that rely on properties of internal classes may have broken logic when using libs that do re-binding for hydration/dehydration. I'm thinking about things such as ArrayObject and similar horrors.

We can probably use a different approach in the libraries, but this is indeed a breakage.

@Ocramius
Copy link
Contributor

I'm simply tempted to not support internal classes in those libraries (at all) btw.

Internal classes are just too inconsistent with the rest of PHP to be used anywhere.

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

Successfully merging this pull request may close these issues.

4 participants