Fix #30296 - Wrong ip value in sendfriend_log table

[Bartlomiejsz]

Description (*)

This PR fixes incorrect behavior of Magento\Framework\HTTP\PhpEnvironment\RemoteAddress::getRemoteAddress method when it's called twice in a row with true passed, and removes "hacks" added to sendfriend module because of this issue.

Related Pull Requests

Fixed Issues (if relevant)

1. Fixes Wrong ip value in send friend_log table #30296

Manual testing scenarios (*)

Steps to reproduce in issue fixed are not correct.

Preconditions:

• Create a product
• Email to Friend set to "Enabled" in configurations
• Limit Sending By set to "IP address"
• Allow for Guests set to "Yes" (not nessesary but if you don't you have create customer before.)

Steps To Reproduce:

• Go to storefront to the product page
• Click on send email to friend icon
• Fill all required fields and click "Send" button
• Check sendfriend_log table

Expected result
Ip saved in the database should be valid int value.

Questions or comments

Contribution checklist (*)

• Pull request has a meaningful description of its purpose
• All commits are accompanied by meaningful commit messages
• All new or changed code is covered with unit/integration tests (if applicable)
• All automated tests passed successfully (all builds are green)

[m2-assistant]

Hi @Bartlomiejsz. Thank you for your contribution
Here is some useful tips how you can test your changes using Magento test environment.
Add the comment under your pull request to deploy test or vanilla Magento instance:

• @magento give me test instance - deploy test instance based on PR changes
• @magento give me 2.4-develop instance - deploy vanilla Magento instance

❗ Automated tests can be triggered manually with an appropriate comment:

• @magento run all tests - run or re-run all required tests against the PR changes
• @magento run <test-build(s)> - run or re-run specific test build(s)
  For example: @magento run Unit Tests

<test-build(s)> is a comma-separated list of build names. Allowed build names are:

1. Database Compare
2. Functional Tests CE
3. Functional Tests EE,
4. Functional Tests B2B
5. Integration Tests
6. Magento Health Index
7. Sample Data Tests CE
8. Sample Data Tests EE
9. Sample Data Tests B2B
10. Static Tests
11. Unit Tests
12. WebAPI Tests

You can find more information about the builds here

ℹ️ Please run only needed test builds instead of all when developing. Please run all test builds before sending your PR for review.

For more details, please, review the Magento Contributor Guide documentation.

⚠️ According to the Magento Contribution requirements, all Pull Requests must go through the Community Contributions Triage process. Community Contributions Triage is a public meeting.

🕙 You can find the schedule on the Magento Community Calendar page.

📞 The triage of Pull Requests happens in the queue order. If you want to speed up the delivery of your contribution, please join the Community Contributions Triage session to discuss the appropriate ticket.

🎥 You can find the recording of the previous Community Contributions Triage on the Magento Youtube Channel

✏️ Feel free to post questions/proposals/feedback related to the Community Contributions Triage process to the corresponding Slack Channel

[Bartlomiejsz]

@magento run all tests

[rogyar]

Hi @Bartlomiejsz. Thanks for your work, good job. Could I ask you, to check the failing tests, please, and also make sure that this case is covered by an integration test. Extending the existing integration testisExceedLimitByIp should be sufficient.

Thank you!

[Bartlomiejsz]

@magento run all tests

[Bartlomiejsz]

@magento run Functional Tests CE

[Bartlomiejsz]

@magento run Functional Tests EE

[Bartlomiejsz]

Hi @rogyar, failing test is not related to my changes, so this is ready for review now

[magento-engcom-team]

Hi @rogyar, thank you for the review.
ENGCOM-8326 has been created to process this Pull Request

[engcom-Bravo]

Hello @Bartlomiejsz

Thank you for your Pull request

I can not reproduce the original issue with the steps provided in the issue:
Proconditions:

• Create a product
• Email to Friend set to "Enabled" in configurations
• Limit Sending By set to "IP address"
• Allow for Guests set to "Yes" (not nessesary but if you don't you have create customer before.)

Steps To Reproduce:

• Go to storfront to the product page
• Click on send email to friend icon
• Fill all required fields and click "Send" button
• Check sendfriend_log table

(/) Expected Result:
Ip value should be converted to biginteger value

(x) Actual Result:
Ip value is not converted properly and it cuted. F.e. If your ip value 127.0.0.1 it will be cutted to 1270.

I'm getting the same converted IP sendfriend_log table
Before applying changes provided in this PR

and After switching to this PR

Can you please correct my steps if I'm processing incorrectly?
Thank you in advance

[Bartlomiejsz]

Hello @engcom-Bravo, funny thing here is that whole issue and steps to reproduce were added here by Magento team from your internal system :)
I believe steps are not exactly valid, issue that was fixed here was incorrect behavior of Magento\Framework\HTTP\PhpEnvironment\RemoteAddress::getRemoteAddress method. Before fix, when this method was called twice time in a row with passed true as parameter, it was returning incorrect result second time - first was integer value (as it should when true is passed), but second was string with ip address.

It's not possible to reproduce issue from steps on Magento instance, because there was some dirty fix added in send friend module - instead of fixing original issue, value returned from getRemoteAddress method was again passed to ip2long method in getSendCount and addSendItem methods.

So I believe there are two things that should be verified here:

• Magento\Framework\HTTP\PhpEnvironment\RemoteAddress::getRemoteAddress method called twice in a row with true passed should return int result (it is covered with a test btw)
• Ip saved to db from send friend module should still work correctly after all the changes

[engcom-Bravo]

@Bartlomiejsz
Thank you for the quick response.
I've tried to check the issue as described in the comment above Magento\Framework\HTTP\PhpEnvironment\RemoteAddress::getRemoteAddress method called twice in a row with true passed should return int result

In my case, the method returns false for ipToLong, and a string "127.0.0.1". And it is called several times, not twice.

In order to proceed correctly, can you please update the PR Description, and provide exact steps?

Thank you in advance

[Bartlomiejsz]

Hi @engcom-Bravo, I updated steps to reproduce.
As I mentioned, you can't expect any change in how this ip is saved into database, because there were hacks added - ip2long method was called inside sendfriend module.
So you need to verify if after all the changes sendfriend module still works correctly without those hacks.

[engcom-Bravo]

✔️ QA Passed

Manual testing scenario

1. From Admin go to Stores - Configuration - Catalog -Email to a Friend and set the following settings
   Enabled - Yes
   Allow for Guests - Yes
   Limit Sending By - IP Address
2. Save Config and clear Magento cache
3. From Storefront, open any product page and click Email button
   
4. Fill in all the required fields and click Send Email
   
5. Assert that "The link to a friend was sent." message appears
   
6. Open database table sendfriend_logand assert that the IP value is saved as a valid INT value

Actual result

IP is saved as a valid INT value

[m2-assistant]

Hi @Bartlomiejsz, thank you for your contribution!
Please, complete Contribution Survey, it will take less than a minute.
Your feedback will help us to improve contribution process.