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

Update AbstractFactory.php #1860

Closed
wants to merge 2 commits into from
Closed

Update AbstractFactory.php #1860

wants to merge 2 commits into from

Conversation

vasiljok
Copy link

Remove reflection from code!

Remove reflection from code!
@kandy
Copy link
Contributor

kandy commented Sep 12, 2015

Currently Magento is supports php from version 5.5, but as you mentioned in the comment your code is working only from php 5.6. But thanks you for contribution anyway.

@antonkril
Copy link
Contributor

We should postpone merge of this PR till minimum supported version is PHP 5.6

@magento-cicd2
Copy link
Contributor

We have automated a Magento Contributor License Agreement verifier for contributions sent to our GitHub projects.
Please see the CLA agreement in the Pull Request comments below.

@benmarks
Copy link
Contributor

benmarks commented Nov 9, 2015

@vasiljok Can you check the CLA requirement please?

@KrystynaKabannyk
Copy link

Hello @vasiljok, are you still interested in this pull request? If yes, could you please sign the CLA agreement?

@vasiljok
Copy link
Author

vasiljok commented Apr 4, 2016

Hello @KrystynaKabannyk I have signed CLA

return $reflection->newInstanceArgs($args);
// TODO:remove if after end support 5.5 version of php
if (version_compare(phpversion(), '5.6.0', '>='))) {
return new $type(... $args); // will work in PHP >= 5.6

Choose a reason for hiding this comment

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

This is invalid syntax for PHP < 5.6 so this will lead to fatal error. When minimal supported version of Magento became 5.6 this PR may be merged if last commit will be rolled back.

So since minimal supported PHP version became greater than 5.6 method body should contains only
return new $type(... $args);
Until that time there is no way to avoid reflection (any workaround will not have performance benefits)

@orlangur
Copy link
Contributor

This PR is obsoleted by #4766.

/cc @vkublytskyi

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.

9 participants