Skip to content

Conversation

@dg
Copy link
Member

@dg dg commented Sep 7, 2015

No description provided.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not ReflectionObject?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why ReflectionObject?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because $this is an instance, not a type name.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It works, ReflectionClass accepts objects.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But ReflectionObject is semantically more correct, and also extends ReflectionClass.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's the difference between ReflectionClass and ReflectionObject anyway? I couldn't find anything worth mentioning in the docs.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that ReflectionObject::getProperties returns real object properties and ReflectionClass::getProperties returns default properties from a class.

@dg dg force-pushed the reflection branch 2 times, most recently from 1c59caf to 84ebe7d Compare September 7, 2015 20:51
@dg dg force-pushed the master branch 5 times, most recently from 8e097dc to e4eb640 Compare October 2, 2015 14:25
@dg dg force-pushed the master branch 2 times, most recently from 7f051bf to f87df33 Compare December 3, 2015 04:09
@dg dg force-pushed the master branch 2 times, most recently from 9725b1d to 9869e52 Compare January 13, 2016 18:03
@dg dg force-pushed the master branch 4 times, most recently from 18f376d to 3fe619f Compare January 22, 2016 03:07
@dg dg force-pushed the master branch 2 times, most recently from 20a93ca to 08cbdeb Compare February 8, 2016 13:39
@dg dg force-pushed the master branch 9 times, most recently from 41da593 to 2c1ae3b Compare April 26, 2016 21:54
@dg dg force-pushed the master branch 15 times, most recently from 8f2b6ff to 83f19ce Compare May 3, 2016 06:22
@dg dg merged commit 7ee1327 into nette:master May 3, 2016
@dg dg deleted the reflection branch May 3, 2016 15:22
@enumag
Copy link
Contributor

enumag commented May 3, 2016

@dg What about composer.json?

@dg
Copy link
Member Author

dg commented May 3, 2016

In next version.

@Majkl578
Copy link
Contributor

Majkl578 commented May 3, 2016

Why? It's not needed by this package so it should not be there, only in suggests section. If anyone was using it directly, they've been shooting themselves in the foot -- they should've had it in their composer.json.

@dg
Copy link
Member Author

dg commented May 3, 2016

Because back compatibility

@Majkl578
Copy link
Contributor

Majkl578 commented May 3, 2016

I don't get it. This package doesn't need that package anymore so it should just drop the dependency entirely. Noone should ever depend on a library indirectly through another dependency because it is just not what dependency means.
Also it's ok to remove nette/security, but not nette/reflection? Doesn't make sense.

@dg
Copy link
Member Author

dg commented May 3, 2016

It is directly used in code.

@enumag
Copy link
Contributor

enumag commented May 4, 2016

Even if this affects anyone its easily fixable by one line in composer.json. Its ok to put a minor BC break like that into 2.4 in my opinion.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants