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

Mutator/mbstring #664

Merged
merged 23 commits into from Apr 1, 2019
Merged

Mutator/mbstring #664

merged 23 commits into from Apr 1, 2019

Conversation

majkel89
Copy link
Contributor

@majkel89 majkel89 commented Mar 10, 2019

This PR:

Relates to issue #654

Converts function calls like mb_chr(74, 'utf-8'), mb_strlen('test', 'utf-8') to chr('test'), strlen('test').

Supported functions:

Pay attention to the function mapping when doing code review.

  • mb_chr -> chr
  • mb_ord -> ord
  • mb_parse_str -> parse_str
  • mb_send_mail -> mail
  • mb_strcut -> substr
  • mb_stripos -> stripos
  • mb_stristr -> stristr
  • mb_strlen -> strlen
  • mb_strpos -> strpos
  • mb_strrchr -> strrchr
  • mb_strripos -> strripos
  • mb_strrpos -> strrpos
  • mb_strstr -> strstr
  • mb_strtolower -> strtolower
  • mb_strtoupper -> strtoupper
  • mb_substr_count -> substr_count
  • mb_substr -> substr
  • mb_convert_case -> strtolower, strtoupper, ucwords depending on mode argument

New profile @extensions created to hold this mutator. It is on by default.

Configuration

    "mutators": {
        "MBString": {
            "settings": {
                "mb_strlen": false,
                "mb_ord": false,
            }
        }
    },

@majkel89
Copy link
Contributor Author

I will have to test it a little bit longer so that this mutator will not produce mutats that will be killed by differences in mb string functions and std string functions.

@majkel89
Copy link
Contributor Author

I have tested the mapping and removed functions that cannot be easily mapped:

  • mb_split - uses regular expressions like ereg functions
  • mb_strrichr - have no equivalent in standard string functions

@majkel89
Copy link
Contributor Author

final fixes added and doc PR created. I think this PR is polished enough to be merged

@maks-rafalko maks-rafalko requested review from BackEndTea and sanmai and removed request for BackEndTea March 22, 2019 21:43
maks-rafalko
maks-rafalko previously approved these changes Mar 22, 2019
Copy link
Member

@maks-rafalko maks-rafalko left a comment

Choose a reason for hiding this comment

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

Looks very nice 👍

@majkel89
Copy link
Contributor Author

could you merge this PR ?

Copy link
Member

@sanmai sanmai left a comment

Choose a reason for hiding this comment

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

The mutator could be simplified if we add a workaround against missing constants to the test.

src/Mutator/Extensions/MBString.php Outdated Show resolved Hide resolved
src/Mutator/Extensions/MBString.php Outdated Show resolved Hide resolved
src/Mutator/Extensions/MBString.php Outdated Show resolved Hide resolved
src/Mutator/Extensions/MBString.php Outdated Show resolved Hide resolved
src/Mutator/Extensions/MBString.php Outdated Show resolved Hide resolved
src/Mutator/Extensions/MBString.php Outdated Show resolved Hide resolved
tests/Mutator/Extensions/MBStringTest.php Show resolved Hide resolved
@sanmai
Copy link
Member

sanmai commented Mar 28, 2019

I'm sorry for going back and forth with this PR, but there isn't a test case for a mixed-case replacement as far as I can see, but please correct me if I overlook.

E.g. a test case displaying that this replacement happens:

- MB_Chr(74);
+ chr(74);

This is customary for other mutators, an example.

Other than that you're doing a beautiful work!

@sanmai sanmai self-requested a review March 28, 2019 07:26
Ocramius
Ocramius previously approved these changes Mar 29, 2019
Copy link
Member

@BackEndTea BackEndTea left a comment

Choose a reason for hiding this comment

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

Great PR,

Only thing i'm missing is that a testcase function call with a variable name is not mutated. (And mostly that it doesn't crash)

e.g.

$a = 'mb_chr'
$a(74)

Should not mutate, and not crash.

@maks-rafalko maks-rafalko added this to the 0.13.0 milestone Apr 1, 2019
@maks-rafalko
Copy link
Member

Thank you @majkel89 👍

@maks-rafalko maks-rafalko merged commit e25b9d6 into infection:master Apr 1, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants