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

Cast types instead of function #735

Closed
wants to merge 6 commits into from
Closed

Cast types instead of function #735

wants to merge 6 commits into from

Conversation

sreichel
Copy link
Contributor

No description provided.

@sreichel sreichel added the Cleanup: Code style Related to simple CS fixes. label Jun 16, 2019
@colinmollenhour
Copy link
Member

What is the motivation/benefit of making this change?

@sreichel
Copy link
Contributor Author

sreichel commented Jun 17, 2019

These functions are "forbidden" in EQP standard, so they should not be used in core too. Just codestyle fixes.

Copy link
Collaborator

@midlan midlan left a comment

Choose a reason for hiding this comment

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

I am voting against this PR. I would change to cast types when theese PHP functions is at least deprecated.

@Sekiphp
Copy link
Collaborator

Sekiphp commented Jun 18, 2019

@sreichel You talking about this repository https://github.com/magento/marketplace-eqp?

I think, that these changes are pointless. There is no bug fixes, no performance improvments....but too many changes.

@sreichel
Copy link
Contributor Author

@midlan the functions will not deprecate, because of additional features (at least intval() & strval()).

@Sekiphp to many changes for what? I do not believe thery will be big code changes anyymore, that could cause merge conflicts.

I dont think its pointless. What's wrong with consistent code? Magento 2 replaced all PHP function with typecasting. Btw ... typecasting is much more faster (i know benchmarks vs real usage)

@Flyingmana
Copy link
Member

a few things.

  • Having a common standard with Magento may be beneficial.
  • every change has the possibility to introduce bugs, even if it may seem impossible
  • the performance difference is becoming less with every php version.

benchmark case for PHP 7.1: https://stackoverflow.com/questions/1912599/is-there-any-particular-difference-between-intval-and-casting-to-int-int-x
result: performance gain of ~0,00000014ms per call, thats an improvement of <1%

Copy link
Member

@Flyingmana Flyingmana left a comment

Choose a reason for hiding this comment

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

fix TravisCI error (one opening Parenthesis was not removed)

@colinmollenhour
Copy link
Member

I scanned through and didn't see any errors, but what method did you use to make this change, was it manual or automated? Did you account for cases like this?

intval($a * $b) // original
(int)$a * $b // incorrect
(int)($a * $b) // correct

@sreichel
Copy link
Contributor Author

Thanks @colinmollenhour . Good point. I manually checked all edit, but also missed that opening parenthesis)

Update tomorrow.

@tmotyl
Copy link
Contributor

tmotyl commented Apr 24, 2020

@sreichel can you bring the patch back to life?

@sreichel
Copy link
Contributor Author

IMHO it looks good. I'd prefer the other PRs get merge and let this open (for now).

@tmotyl
Copy link
Contributor

tmotyl commented Apr 28, 2020

ok

@sreichel sreichel added the hold label May 9, 2020
tmotyl
tmotyl previously approved these changes May 30, 2020
@sreichel sreichel removed the hold label May 31, 2020
@sreichel
Copy link
Contributor Author

Something went wrong .... i'll create a new one.

@sreichel sreichel closed this Jun 13, 2020
@sreichel sreichel deleted the hotfix/cast-types branch June 13, 2020 00:41
fballiano pushed a commit that referenced this pull request Dec 31, 2022
fballiano pushed a commit to fballiano/openmage that referenced this pull request Jan 2, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Cleanup: Code style Related to simple CS fixes. invalid
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants