Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix behaviour of ReflectionClass::export/ReflectionObject::export to match behaviour of core classes #397

Closed
wants to merge 2 commits into from

Conversation

muglug
Copy link
Contributor

@muglug muglug commented Feb 2, 2018

Roave\BetterReflection\Reflection\ReflectionClass::export and Roave\BetterReflection\Reflection\ReflectionObject::export would always return values. However, ReflectionClass::export and ReflectionObject::export echo values to stdout by default,.

This PR changes the default behaviour of the BetterReflection classes to match up with the core ones. Now the return functionality is hidden behind an explicit bool $return argument.

It also allows ReflectionClass::export to take an object instance, like its core equivalent.

Copy link
Member

@Ocramius Ocramius left a comment

Choose a reason for hiding this comment

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

I'd rather adapt the behavior of the Roave\BetterReflection\Adapter\*::export() symbols to what internal reflection does, and completely deprecate Roave\BetterReflection\*::export() methods, which shouldn't be used in first place, in my opinion.

The idea of exporting reflection is not really core to the package, and I think this is a mistake that we initially did in designing the package overall by implementing Reflector on Roave\BetterReflection\Reflection\*.

@asgrim do you remember why we did that? And can we chop its head off, since we now have the entire Adapter namespace?

@theofidry
Copy link
Contributor

ping @asgrim

@asgrim
Copy link
Member

asgrim commented Jun 8, 2018

@Ocramius @theofidry indeed the export functions were written before we wrote the Adapter namespace, I'd also favour fixing/moving that behaviour into just the adapters and deprecating/removing the export functions from the Better Reflections.

With that said, this change looks good, but only if we can remove it from the main BR reflections and just do it in export. May be some scope difficulty to overcome, but I'm sure we can find our way aroudn things :D

@asgrim
Copy link
Member

asgrim commented Jan 25, 2019

Gah, Better Reflection class implements \Reflector which mandates static function export ();, so just moving them into the \Roave\BetterReflection\Reflection\Adapter\ReflectionClass::export and \Roave\BetterReflection\Reflection\Adapter\ReflectionObject::export isn't going to wash :/

@asgrim
Copy link
Member

asgrim commented Jan 25, 2019

I mean - we can implement the export methods in the adapters and just leave the actual BR implementations as exception throwing, that would work, just leaves an annoying unusable function lying aroud meh.

@Ocramius
Copy link
Member

Ocramius commented Feb 9, 2019

@asgrim what about introducing a BC Break where we move the Reflector interface from the BetterReflection\Reflection\* instances (and only keep it in BetterReflection\Reflection\Adapter\*)?

@asgrim
Copy link
Member

asgrim commented Feb 11, 2019

@Ocramius actually yes that seems like a reasonable approach, it's a BC break regardless, so I'm +1 on that. I'll see if I can get to that at some point!

@asgrim
Copy link
Member

asgrim commented Jun 5, 2019

Only took like 4 months - opened a new PR in #479 as I don't think I can push to this one.

@asgrim asgrim closed this Jun 5, 2019
@Ocramius Ocramius added this to the 4.0.0 milestone Jun 7, 2019
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.

None yet

4 participants