Skip to content

keep literal string for simple str_replace#8619

Merged
orklah merged 2 commits intovimeo:masterfrom
kkmuffme:keep-literal-string-for-simple-str_replace
Oct 27, 2022
Merged

keep literal string for simple str_replace#8619
orklah merged 2 commits intovimeo:masterfrom
kkmuffme:keep-literal-string-for-simple-str_replace

Conversation

@kkmuffme
Copy link
Copy Markdown
Contributor

@kkmuffme kkmuffme commented Oct 26, 2022

Just for the simplest case, as that is helpful to avoid tons of false positives, especially with callbacks that are created dynamically from paths

As discussed in #8611 (comment)

@kkmuffme
Copy link
Copy Markdown
Contributor Author

kkmuffme commented Oct 26, 2022

@orklah the failing tests seem completely unrelated to the changes? Already not sure why DirnameTest.php showed up in CI , I didn't even change that (but I fixed it now anyways). But now more and more unrelated stuff shows up. Are the tests in master branch working?

@kkmuffme kkmuffme marked this pull request as ready for review October 26, 2022 10:46
@kkmuffme kkmuffme force-pushed the keep-literal-string-for-simple-str_replace branch 2 times, most recently from d287c88 to 3bc8c09 Compare October 26, 2022 11:10
This was referenced Oct 26, 2022
Merged
Merged
@mcaskill
Copy link
Copy Markdown
Contributor

mcaskill commented Oct 26, 2022

Thanks for the CS fix of addslashes on DirnameTest.
As for str_replace and str_ireplace, that's a nice and simple solution.

@orklah
Copy link
Copy Markdown
Collaborator

orklah commented Oct 26, 2022 •
edited
Loading

yeah, circleCI is sometimes broken on PR, I'm not sure exactly why, but it happens without fault by the author so I'm merging anyway when I don't see obvious issues. It let some CS issues on master.

About that, you have one of your own right now :)

FILE: ...nal/Provider/ReturnTypeProvider/StrReplaceReturnTypeProvider.php
----------------------------------------------------------------------
FOUND 0 ERRORS AND 1 WARNING AFFECTING 1 LINE
----------------------------------------------------------------------
 59 | WARNING | Line exceeds 120 characters; contains 127 characters
----------------------------------------------------------------------

LGTM after that!

@kkmuffme kkmuffme force-pushed the keep-literal-string-for-simple-str_replace branch from 3bc8c09 to e064a0c Compare October 27, 2022 07:24
@kkmuffme
Copy link
Copy Markdown
Contributor Author

kkmuffme commented Oct 27, 2022

Fixed, ready to merge (missed that, bc I pushed a minor change that created that one)

@orklah
Copy link
Copy Markdown
Collaborator

orklah commented Oct 27, 2022

Thanks!

@orklah orklah added the release:fix The PR will be included in 'Fixes' section of the release notes label Oct 27, 2022
@orklah orklah merged commit 7c83878 into vimeo:master Oct 27, 2022
@kkmuffme kkmuffme deleted the keep-literal-string-for-simple-str_replace branch December 1, 2022 18:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

release:fix The PR will be included in 'Fixes' section of the release notes

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@kkmuffme @mcaskill @orklah