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

Fix strpos with non-string needle #1213

Merged
merged 2 commits into from
Dec 28, 2020
Merged

Fix strpos with non-string needle #1213

merged 2 commits into from
Dec 28, 2020

Conversation

luigifab
Copy link
Contributor

@luigifab luigifab commented Sep 17, 2020

Description

This fix: Deprecated functionality: strpos(): Non-string needles will be interpreted as strings in the future. Use an explicit chr() call to preserve the current behavior in .../app/code/core/Mage/Eav/Model/Entity/Increment/Numeric.php on line 48

OpenMage 20.0.3 / PHP 7.4.9

Manual testing scenarios

  1. Create a new website with a new store view in backend.
  2. Go to frontend on this new store view.
  3. Add a product to cart, go to checkout, and try to make the order.

Contribution checklist

  • Pull request has a meaningful description of its purpose
  • All commits are accompanied by meaningful commit messages
  • All automated tests passed successfully (all builds are green)

@Flyingmana Flyingmana added the PHP 7.x Related to PHP 7.x label Sep 17, 2020
@github-actions github-actions bot added the Component: Eav Relates to Mage_Eav label Sep 17, 2020
@kiatng
Copy link
Contributor

kiatng commented Oct 23, 2020

Question: The needle in this case is $this->getPrefix(), in the official doc:

If needle is not a string, it is converted to an integer and applied as the ordinal value of a character. This behavior is deprecated as of PHP 7.3.0, and relying on it is highly discouraged. Depending on the intended behavior, the needle should either be explicitly cast to string, or an explicit call to chr() should be performed.

So, shouldn't the fix be strpos($last, (string) $this->getPrefix())?

@luigifab
Copy link
Contributor Author

@kiatng hum, when you are creating the first order on a new store view, $this->getPrefix() is empty (null?).

@kiatng
Copy link
Contributor

kiatng commented Oct 24, 2020

@luigifab I was thinking in a more general case: if (strpos($last, (string) $this->getPrefix()) === 0) { will work for not just when $last is null but also when $this->getPrefix() returns a non-string.

@luigifab
Copy link
Contributor Author

oh, yes, I can change with (string)

@Flyingmana
Copy link
Contributor

I added a testcase for this, and validated this PR will fix the Issue.

Maybe someone can extend this with a few more tests with real world variables, especially for the case of a not set prefix.

https://github.com/OpenMage/Testfield/blob/main/tests/Unit/Regression/Lts/Ticket1213Test.php

@Flyingmana Flyingmana merged commit ea44188 into OpenMage:1.9.4.x Dec 28, 2020
@github-actions
Copy link
Contributor

Unit Test Results

1 files  1 suites   0s ⏱️
0 tests 0 ✔️ 0 💤 0 ❌
2 runs  2 ✔️ 0 💤 0 ❌

Results for commit ea44188.

@sreichel sreichel added this to the Release 19.4.9 / 20.0.5 milestone Dec 28, 2020
@luigifab luigifab deleted the fix-increment-notice branch February 2, 2021 09:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Component: Eav Relates to Mage_Eav PHP 7.x Related to PHP 7.x
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants