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

Remove isPositiveInteger() check for third argument to strpos() #5257

Merged
merged 6 commits into from Dec 3, 2023

Conversation

ikari7789
Copy link
Contributor

Fixes: rectorphp/rector#8303

Removes check that third argument is only a positive integer. Should fix cases where the offset is set dynamically through a variable or expression.

@samsonasik
Copy link
Member

More on looking at it, I think dynamic offset should be skipped, to avoid offset changed, eg: on loop which cause invalid changed

@ikari7789
Copy link
Contributor Author

More on looking at it, I think dynamic offset should be skipped, to avoid offset changed, eg: on loop which cause invalid changed

After I started looking at the code, I think this isn't the right approach to take. The value of the variable here doesn't matter. The previous code was only adding in the offset if the second argument was explicitly an integer. The code now longer cares what the offset is (unless explicitly a zero integer) and sets that directly into place. Even in places where the offset changes, I don't think it should have any impact, unless I'm wildly misunderstanding how this gets processed.

@samsonasik
Copy link
Member

I will let @TomasVotruba decide on this.

imo, language features is ok as far as it has safety belt, as old backport to old language will always work.

On dynamic offset usage, it probably make harder to debug when there is a bug caused by its transformation.

@TomasVotruba
Copy link
Member

Looks good to me, let's give it a try 👍

@TomasVotruba TomasVotruba merged commit 18617f9 into rectorphp:main Dec 3, 2023
39 checks passed
@TomasVotruba
Copy link
Member

Thank you @ikari7789

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants