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

Forbidden usage of settype function #46

Merged
merged 1 commit into from
Mar 27, 2018
Merged

Forbidden usage of settype function #46

merged 1 commit into from
Mar 27, 2018

Conversation

carusogabriel
Copy link
Contributor

I'd like to propose the forbidden usage of the settype function. cast should be used instead:

<?php
$foo = '1';

-settype($foo, 'int');
+$foo = (int) $foo;

var_dump($foo); // int(1)

@carusogabriel carusogabriel self-assigned this Mar 26, 2018
@carusogabriel carusogabriel added this to the 5.0.0 milestone Mar 26, 2018
Majkl578
Majkl578 previously approved these changes Mar 26, 2018
@Majkl578
Copy link
Contributor

Can we also ban gettype()? 🙏

alcaeus
alcaeus previously approved these changes Mar 26, 2018
@carusogabriel
Copy link
Contributor Author

@Majkl578 Isn't gettype helpful in switch statements?

@Majkl578
Copy link
Contributor

No, is_*() should be always preferred.

@carusogabriel
Copy link
Contributor Author

Isn't a switch prefered instead of elseif?

<?php
$foo = (unknow value);

switch(gettype($foo)) {
    case 'string': /* do someting */;
    case 'int': /* do someting */;
    case 'array': /* do someting */;
}

// vs
if (is_string($foo)) {
  // do something
} elseif (is_int($foo)) {
    // do something
} elseif (is_array($foo)) {
    //do something
}

@Majkl578
Copy link
Contributor

switch (true) {
    case is_string($foo) :
        // do something
    case is_int($foo) :
        // do something
    case is_array($foo) :
        //do something
}

@carusogabriel
Copy link
Contributor Author

I don't like switch(true) IMHO :(

So feel free to propose it :)

malarzm
malarzm previously approved these changes Mar 26, 2018
Copy link
Member

@malarzm malarzm left a comment

Choose a reason for hiding this comment

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

TIL settype is a thing o.O

Copy link
Member

@lcobucci lcobucci left a comment

Choose a reason for hiding this comment

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

Can we please add tests to verify this behaviour?

@carusogabriel
Copy link
Contributor Author

@lcobucci Added 👍

Gosh, I amend the commit, all reviews were gone 🤦‍♂️

@deeky666
Copy link
Member

I didn't even know settype() exists. Away with it! gettype() on the other hand is sometimes useful for exception messages though...

@carusogabriel
Copy link
Contributor Author

carusogabriel commented Mar 26, 2018

@deeky666 Feel free to vote with a review Approving changes 😄

@Majkl578 Majkl578 merged commit 8c7f415 into master Mar 27, 2018
@Majkl578 Majkl578 deleted the settype branch March 27, 2018 00:29
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

7 participants