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

Convert intval to typecast int #491

Closed
PirataNervo opened this Issue May 7, 2014 · 29 comments

Comments

Projects
None yet
10 participants
@PirataNervo
Contributor

PirataNervo commented May 7, 2014

Speed Tests Reference:
http://hakre.wordpress.com/2010/05/13/php-casting-vs-intval/

The speed increase is incredible and since we have a lot of intval's, I decided to write a script to help me out.

PHP Script:
http://pastebin.com/Qjfk1WEh

Usage: intval.php?dir=[PATH]

@PirataNervo PirataNervo added this to the 1.8 Beta 1 milestone May 7, 2014

@PirataNervo PirataNervo self-assigned this May 7, 2014

@fabiomaia

This comment has been minimized.

Show comment
Hide comment
@fabiomaia

fabiomaia May 9, 2014

Does the speed increase apply to strval($foo) vs (string)$foo as well?

strval() occurs in captcha.php. Even if it's not faster, typecasting should still be preferred for some consistency.

Actually, is strval() even needed there?

fabiomaia commented May 9, 2014

Does the speed increase apply to strval($foo) vs (string)$foo as well?

strval() occurs in captcha.php. Even if it's not faster, typecasting should still be preferred for some consistency.

Actually, is strval() even needed there?

@fabiomaia

This comment has been minimized.

Show comment
Hide comment
@fabiomaia

fabiomaia May 9, 2014

There is also a bunch of occurrences for floatval().

fabiomaia commented May 9, 2014

There is also a bunch of occurrences for floatval().

@PirataNervo

This comment has been minimized.

Show comment
Hide comment
@PirataNervo

PirataNervo May 9, 2014

Contributor

We need to do the replacement for floatval too but the amount of occurrences is much lower. As for the captcha, shouldn't be needed to replace by anything.

Contributor

PirataNervo commented May 9, 2014

We need to do the replacement for floatval too but the amount of occurrences is much lower. As for the captcha, shouldn't be needed to replace by anything.

@fabiomaia

This comment has been minimized.

Show comment
Hide comment
@fabiomaia

fabiomaia May 9, 2014

There is a strval() in captcha.php. Why not typecast there instead? It's a tiny detail but... :P

fabiomaia commented May 9, 2014

There is a strval() in captcha.php. Why not typecast there instead? It's a tiny detail but... :P

@JordanMussi

This comment has been minimized.

Show comment
Hide comment
@JordanMussi

JordanMussi May 9, 2014

Contributor

For consistency, yes it should be changed to a typecast. 👅

Contributor

JordanMussi commented May 9, 2014

For consistency, yes it should be changed to a typecast. 👅

@PirataNervo

This comment has been minimized.

Show comment
Hide comment
@PirataNervo

PirataNervo May 9, 2014

Contributor

I just think that we should focus on important things rather than tiny details right now, but I guess that's just me.

Contributor

PirataNervo commented May 9, 2014

I just think that we should focus on important things rather than tiny details right now, but I guess that's just me.

@JN-Jones

This comment has been minimized.

Show comment
Hide comment
@JN-Jones

JN-Jones May 9, 2014

Contributor

Not only ;) we can change this in a few seconds before we release beta 1, 2 or even before the final release - we should finish the jQuery conversion and release beta 1 ASAP

Contributor

JN-Jones commented May 9, 2014

Not only ;) we can change this in a few seconds before we release beta 1, 2 or even before the final release - we should finish the jQuery conversion and release beta 1 ASAP

@fabiomaia

This comment has been minimized.

Show comment
Hide comment
@fabiomaia

fabiomaia May 9, 2014

I was just pointing it out. I didn't see this mentioned before.

On Fri, May 9, 2014 at 8:16 PM, Jones notifications@github.com wrote:

Not only ;) we can change this in a few seconds before we release beta 1, 2 or even before the final release - we should finish the jQuery conversion and release beta 1 ASAP

Reply to this email directly or view it on GitHub:
#491 (comment)

fabiomaia commented May 9, 2014

I was just pointing it out. I didn't see this mentioned before.

On Fri, May 9, 2014 at 8:16 PM, Jones notifications@github.com wrote:

Not only ;) we can change this in a few seconds before we release beta 1, 2 or even before the final release - we should finish the jQuery conversion and release beta 1 ASAP

Reply to this email directly or view it on GitHub:
#491 (comment)

@Sama34

This comment has been minimized.

Show comment
Hide comment
@Sama34

Sama34 May 11, 2014

Contributor

We can push this on Beta 1 and leave the rest of possible type-casting for beta 2. Who is the one to apply this? IIRC there were no issues on the tests done.

Contributor

Sama34 commented May 11, 2014

We can push this on Beta 1 and leave the rest of possible type-casting for beta 2. Who is the one to apply this? IIRC there were no issues on the tests done.

@PirataNervo

This comment has been minimized.

Show comment
Hide comment
@PirataNervo

PirataNervo May 12, 2014

Contributor

I can do this, not a problem. But I'll only be able to look at this Thursday - though I believe there's no rush. We still have jQuery to complete.

Contributor

PirataNervo commented May 12, 2014

I can do this, not a problem. But I'll only be able to look at this Thursday - though I believe there's no rush. We still have jQuery to complete.

@Stefan-ST

This comment has been minimized.

Show comment
Hide comment
@Stefan-ST

Stefan-ST May 12, 2014

Contributor

Do it as late as possible, it may break pull requests and other stuff.

Contributor

Stefan-ST commented May 12, 2014

Do it as late as possible, it may break pull requests and other stuff.

@JN-Jones JN-Jones added 1.8 labels May 17, 2014

@PirataNervo

This comment has been minimized.

Show comment
Hide comment
@PirataNervo

PirataNervo May 18, 2014

Contributor

Stefan is right, we should do this only when we're about to release Beta 1.

Contributor

PirataNervo commented May 18, 2014

Stefan is right, we should do this only when we're about to release Beta 1.

@Sama34 Sama34 assigned PaulBender and unassigned PirataNervo May 19, 2014

@PirataNervo PirataNervo modified the milestones: 1.8 Beta 2, 1.8 Beta 1 May 27, 2014

@PenguinPaul

This comment has been minimized.

Show comment
Hide comment
@PenguinPaul

PenguinPaul Jun 3, 2014

Contributor

Looks like Beta 2 then?

Contributor

PenguinPaul commented Jun 3, 2014

Looks like Beta 2 then?

@JordanMussi

This comment has been minimized.

Show comment
Hide comment
@JordanMussi

JordanMussi Jun 4, 2014

Contributor

Yeah 👅 looks like we forgot this one...

Contributor

JordanMussi commented Jun 4, 2014

Yeah 👅 looks like we forgot this one...

@PirataNervo

This comment has been minimized.

Show comment
Hide comment
@PirataNervo

PirataNervo Jun 4, 2014

Contributor

I didn't forget it - we can't risk losing PR compatibility until a few days before Beta 2 is ready. I postponed it to Beta 2.

Contributor

PirataNervo commented Jun 4, 2014

I didn't forget it - we can't risk losing PR compatibility until a few days before Beta 2 is ready. I postponed it to Beta 2.

@WildcardSearch

This comment has been minimized.

Show comment
Hide comment
@WildcardSearch

WildcardSearch Jun 4, 2014

Member

I am of the opinion that this should be one of the last changes made before final testing. As mentioned it will effect almost every active PR so it would be best to finish all open issues (are as many as are feasible) before making this change.

Member

WildcardSearch commented Jun 4, 2014

I am of the opinion that this should be one of the last changes made before final testing. As mentioned it will effect almost every active PR so it would be best to finish all open issues (are as many as are feasible) before making this change.

@PirataNervo PirataNervo modified the milestones: 1.8 Beta 3, 1.8 Beta 2 Jun 24, 2014

@PirataNervo

This comment has been minimized.

Show comment
Hide comment
@PirataNervo

PirataNervo Jun 24, 2014

Contributor

Postponing to Beta 3.

Contributor

PirataNervo commented Jun 24, 2014

Postponing to Beta 3.

@Sama34

This comment has been minimized.

Show comment
Hide comment
@Sama34

Sama34 Jul 17, 2014

Contributor

@PirataNervo remember this is to be pushed at the last moment, we should get all enhancements and PRs done by then.

Contributor

Sama34 commented Jul 17, 2014

@PirataNervo remember this is to be pushed at the last moment, we should get all enhancements and PRs done by then.

@PirataNervo

This comment has been minimized.

Show comment
Hide comment
@PirataNervo

PirataNervo Jul 17, 2014

Contributor

@Sama34 yeap I remember :)

Contributor

PirataNervo commented Jul 17, 2014

@Sama34 yeap I remember :)

@PirataNervo

This comment has been minimized.

Show comment
Hide comment
@PirataNervo

PirataNervo Jul 21, 2014

Contributor

This is finished.

Contributor

PirataNervo commented Jul 21, 2014

This is finished.

@PirataNervo PirataNervo added the fixed label Jul 21, 2014

@JordanMussi

This comment has been minimized.

Show comment
Hide comment
@JordanMussi

JordanMussi Jul 21, 2014

Contributor

Before: $pms = implode(",", array_map("intval", $pms));
After: $pms = implode(",", array_map("(int), $pms); - missing " and you can't array_map using a cast either...
Should be (how it started): $pms = implode(",", array_map("intval", $pms);
https://github.com/mybb/mybb/blob/feature/moderation.php#L2791

We need to find more occurrences of this... (if there are any)

Contributor

JordanMussi commented Jul 21, 2014

Before: $pms = implode(",", array_map("intval", $pms));
After: $pms = implode(",", array_map("(int), $pms); - missing " and you can't array_map using a cast either...
Should be (how it started): $pms = implode(",", array_map("intval", $pms);
https://github.com/mybb/mybb/blob/feature/moderation.php#L2791

We need to find more occurrences of this... (if there are any)

@JordanMussi JordanMussi reopened this Jul 21, 2014

@euantorano

This comment has been minimized.

Show comment
Hide comment
@euantorano

euantorano Jul 21, 2014

Member

Nice catch Jordan!

Member

euantorano commented Jul 21, 2014

Nice catch Jordan!

@JordanMussi

This comment has been minimized.

Show comment
Hide comment
@JordanMussi

JordanMussi Jul 21, 2014

Contributor

Right, there are more locations.

Here's the list of all of them...


Some array_map('intval')s are unaffected. It must have been an odd issue in the converter...

Contributor

JordanMussi commented Jul 21, 2014

Right, there are more locations.

Here's the list of all of them...


Some array_map('intval')s are unaffected. It must have been an odd issue in the converter...

@PirataNervo

This comment has been minimized.

Show comment
Hide comment
@PirataNervo

PirataNervo Jul 21, 2014

Contributor

Yes the converted skipped array_maps. But I'll fix those manually now. Thanks Jordan.
EDIT: Ah the converted looked for ' and not " in array_map. It should have looked for both.

Contributor

PirataNervo commented Jul 21, 2014

Yes the converted skipped array_maps. But I'll fix those manually now. Thanks Jordan.
EDIT: Ah the converted looked for ' and not " in array_map. It should have looked for both.

@PirataNervo

This comment has been minimized.

Show comment
Hide comment
@PirataNervo

PirataNervo Jul 21, 2014

Contributor

@JordanMussi I think some of those locations are not right. I guess you probably just searched array_map rather than array_map("(int)

I've fixed 16 occurrences, going to commit now.

Contributor

PirataNervo commented Jul 21, 2014

@JordanMussi I think some of those locations are not right. I guess you probably just searched array_map rather than array_map("(int)

I've fixed 16 occurrences, going to commit now.

@PirataNervo

This comment has been minimized.

Show comment
Hide comment
@PirataNervo

PirataNervo Jul 21, 2014

Contributor

I think this can be closed now.

Contributor

PirataNervo commented Jul 21, 2014

I think this can be closed now.

@PaulBender

This comment has been minimized.

Show comment
Hide comment
@PaulBender

PaulBender Jul 21, 2014

Member

There are still some PHP parse errors. The array_map functions in 12096ed are all missing a closing parentheses ( ) ).

Member

PaulBender commented Jul 21, 2014

There are still some PHP parse errors. The array_map functions in 12096ed are all missing a closing parentheses ( ) ).

@PirataNervo

This comment has been minimized.

Show comment
Hide comment
@PirataNervo

PirataNervo Jul 21, 2014

Contributor

Oops, forgot the converter did that too. Fixed that and the (int)(int) found in one file.

Contributor

PirataNervo commented Jul 21, 2014

Oops, forgot the converter did that too. Fixed that and the (int)(int) found in one file.

@PirataNervo

This comment has been minimized.

Show comment
Hide comment
@PirataNervo

PirataNervo Jul 21, 2014

Contributor

I've gone through many many pages and didn't find any parse errors related to this, so I'm closing this issue.

Contributor

PirataNervo commented Jul 21, 2014

I've gone through many many pages and didn't find any parse errors related to this, so I'm closing this issue.

@PirataNervo PirataNervo removed their assignment Jan 12, 2015

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