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

Small improvement to the eval() call #1162

Closed
wants to merge 1 commit into from
Closed

Conversation

dracony
Copy link

@dracony dracony commented Apr 2, 2015

http://php.net/manual/en/function.eval.php

eval() returns NULL unless return is called in the evaluated code,
in which case the value passed to return is returned.
If there is a parse error in the evaluated code,
eval() returns FALSE and execution of the following code continues normally.

A 'return' statement seems a more elegant way of handling this than using $result =. The behavior remains unchanged. If an error happens eval() will return False so $return will retain it's False if that happens,

http://php.net/manual/en/function.eval.php

> eval() returns NULL unless return is called in the evaluated code,
> in which case the value passed to return is returned.
> If there is a parse error in the evaluated code,
> eval() returns FALSE and execution of the following code continues normally.

A 'return' statement seems a more elegant way of handling this than using `$result = `. The behavior remains unchanged. If an error happens eval() will return `False` so $return will retain it's `False` if that happens,
@otoolec otoolec added the PS label Apr 2, 2015
@otoolec otoolec self-assigned this Apr 2, 2015
@otoolec
Copy link
Contributor

otoolec commented Apr 2, 2015

Thoughts on trying to remove the eval entirely? It looks like the code is using eval to try to process boolean logic for filters and filter groups. Rewriting the code to avoid eval would be both safer and easier to maintain/read.

@dracony
Copy link
Author

dracony commented Apr 2, 2015

I'm not really a magento person, so I wouldn't know really =) But if it's not eval-ing user editable content there is really no danger to it imho. eval() is not that different to require and include in that case.

@nevvermind
Copy link
Contributor

@dracony - I still wouldn't use it. It has a bad reputation. If there's now way out of using it, yeah, sure. But there usually is. Oh, and because of the Suhosin Extension and security admins. Why would Magento stop working because someone decided against eval?

@vrann
Copy link
Contributor

vrann commented Apr 30, 2015

Created internal issue to get rid of evals throughout the code.
Closing this pull request, because it won't be relevant.

@vrann vrann closed this Apr 30, 2015
okorshenko pushed a commit that referenced this pull request Jul 5, 2017
[TSG] Backporting for 2.1 (pr16) (2.1.8)
magento-engcom-team pushed a commit that referenced this pull request Feb 1, 2018
…w an error if order doesn't exist. #1162

 - Merge Pull Request magento-engcom/magento2ce#1162 from RomaKis/magento2:7760
 - Merged commits:
   1. 1b3304a
   2. a732ff6
magento-engcom-team pushed a commit that referenced this pull request Feb 1, 2018
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

5 participants