-
Notifications
You must be signed in to change notification settings - Fork 9.3k
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 for Issue #7227: "x_forwarded_for" value is always empty in Order object #21787
Fix for Issue #7227: "x_forwarded_for" value is always empty in Order object #21787
Conversation
Hi @cmuench. Thank you for your contribution
For more details, please, review the Magento Contributor Assistant documentation |
if ($remoteAddress) { | ||
$quote->setRemoteIp($remoteAddress); | ||
$xForwardIp = $this->request->getServer('HTTP_X_FORWARDED_FOR'); | ||
$quote->setXForwardedFor($xForwardIp); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe we should check for emptiness here? Variable name looks strange, why not $forwardedForIp
or something?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The code is the same as in \Magento\Checkout\Model\remoteAddress::getQuote
.
I checked the docblock of \Magento\Framework\HTTP\PhpEnvironment\RemoteAddress::getRemoteAddress
. IMHO the @return
type is wrong. It should be "@return int|bool|string".
What do you mean?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It also possible to inline the code. The variable is not really needed.
$this->requestMock | ||
->expects($this->once()) | ||
->method('getServer') | ||
->willReturn($xForwarderForHeader); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We must assert data was actually set to quote, please consider making a separate tiny test method. Don't use unneeded execution flow checks like $this->once
, we prefer to verify code behavior rather than execution flow.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok, I will remove that, but in the same test case is $this->once
already used several times for similar checks.
I already tried to create an own test method, but it's hard because I have to copy&paste a lot of stuff.
Hi @orlangur, thank you for the review. |
✔️ QA passed |
Hi @cmuench, thank you for your contribution! |
…mpty in Order object #21787
Description
I added the header logic of the quote session to the Quote Management service. This will fill the currently empty x_forwarded_for column in the sales_order table.
Fixed Issues
Contribution checklist