Skip to content

#11825: Generate new FormKey and replace for oldRequestParams Wishlist #12038

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 3 commits into from
Dec 1, 2017
Merged

#11825: Generate new FormKey and replace for oldRequestParams Wishlist #12038

merged 3 commits into from
Dec 1, 2017

Conversation

osrecio
Copy link
Member

@osrecio osrecio commented Nov 5, 2017

Generate new FormKey afterLogin and set to the beforeRequest(Wishlist)

Description

Generate new FormKey afterLogin and set to the beforeRequest(Wishlist)

Fixed Issues (if relevant)

  1. 2.1.9 Item not added to the Wishlist if the user is not logged at the moment he click on the button to add it. #11825: 2.1.9 Item not added to the Wishlist if the user is not logged at the moment he click on the button to add it
  2. Adding to wishlist doesn't work when not logged in #11908: Adding to wishlist doesn't work when not logged in

Manual testing scenarios

  1. Create an user account.
  2. Logout from the user account
  3. Add a product to your Wishlist , you will get redirected to the login page
  4. Login

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 on Travis CI are green)

@osrecio
Copy link
Member Author

osrecio commented Nov 5, 2017

Can you check @vkublytskyi . Thanks!

@vkublytskyi vkublytskyi self-assigned this Nov 5, 2017
@vkublytskyi vkublytskyi added this to the November 2017 milestone Nov 5, 2017
@vkublytskyi vkublytskyi added Release Line: 2.2 2.2.x duplicate Reproduced on 2.1.x The issue has been reproduced on latest 2.1 release Reproduced on 2.2.x The issue has been reproduced on latest 2.2 release Reproduced on 2.3.x The issue has been reproduced on latest 2.3 release labels Nov 5, 2017
ihor-sviziev
ihor-sviziev requested changes Nov 6, 2017
View reviewed changes
app/code/Magento/PageCache/Observer/FlushFormKey.php Outdated
*/
public function __construct(CookieFormKey $cookieFormKey, DataFormKey $dataFormKey)
public function __construct(CookieFormKey $cookieFormKey, DataFormKey $dataFormKey, Session $customerSession)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is not backward compatible change. Could you add $customerSession as optional parameter with fallback to retrieving this object via ObjectManger?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @ihor-sviziev I will work after mm17es in this fix.

app/code/Magento/PageCache/Observer/FlushFormKey.php Outdated
$this->cookieFormKey->delete();
$this->dataFormKey->set(null);

$beforeParams = $this->customerSession->getBeforeRequestParams();
if ($beforeParams['form_key'] == $currentFormKey) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you add check that $beforeParams['form_key'] is exist? $beforeParams might be empty array.
Also would be great to use strict equal === comparison instead of ==.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @ihor-sviziev I will work after mm17es in this fix.

@osrecio
Copy link
Member Author

osrecio commented Nov 6, 2017

@ihor-sviziev Changes Made. Thanks for your comments

@vkublytskyi
Copy link

@ihor-sviziev Observers and plugins do not follow this backward compatibility rule (as they must not be extended or called individually) and constructor may be changed.

@vkublytskyi
Copy link

@osrecio tests failed as this fix introduces dependency for PageCache on Customer module. We should avoid this for bugfixes. Also we should avoid this as Customer module already has a dependency on PageCashe module and we must avoid circular dependencies. As a solution, I may suggest make a plugin on \Magento\PageCache\Observer\FlushFormKey in a customer module

ihor-sviziev
ihor-sviziev requested changes Nov 7, 2017
View reviewed changes
Copy link
Contributor

@ihor-sviziev ihor-sviziev left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please review latest comments form @vkublytskyi

@osrecio osrecio force-pushed the PR#11825_2.2 branch 2 times, most recently from e4caf97 to f6b052f Compare November 26, 2017 12:05
@osrecio
Copy link
Member Author

osrecio commented Nov 26, 2017

Hi @vkublytskyi I added a Plugin in Customer Module to make the same logic.

@ihor-sviziev
Copy link
Contributor

@osrecio static test failed because of not used parameter. Please ignore it in the code.
Also would be great to remove not related changes to keep only needed.
And third thing - could you add unit tests or better integration tests for your new code?

vkublytskyi
vkublytskyi reviewed Nov 27, 2017
View reviewed changes
app/code/Magento/Customer/Model/Plugin/CustomerFlushFormKey.php Outdated
/**
* @param FlushFormKey $subject
* @param callable $proceed
* @param $args
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add @SuppressWarnings(PHPMD.UnusedFormalParameter) to avoid static tests failure

@osrecio osrecio force-pushed the PR#11825_2.2 branch 2 times, most recently from 80a93f2 to 60a15b8 Compare November 27, 2017 20:09
@osrecio
Copy link
Member Author

osrecio commented Nov 27, 2017 •
edited
Loading

@vkublytskyi @ihor-sviziev

  1. Added @SuppressWarnings(PHPMD.UnusedFormalParameter) to avoid static tests failure
  2. Deleted not related changes
  3. Added Unit Tests

@ihor-sviziev ihor-sviziev self-assigned this Nov 28, 2017
vkublytskyi
vkublytskyi suggested changes Nov 28, 2017
View reviewed changes
Copy link

@vkublytskyi vkublytskyi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added unit test contains an error

app/code/Magento/Customer/Test/Unit/Model/Plugin/CustomerFlushFormKeyTest.php Outdated
->method('setBeforeRequestParams')
->with($beforeParams);

$plugin->aroundExecute($observer, $this->closure, $observer);
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This line contains an error. $observer is instance of Magento\PageCache\Observer\FlushFormKey which is child of \Magento\Framework\Event\ObserverInterface. aroundExecute method should mutch signature of Magento\PageCache\Observer\FlushFormKey::execute so last parameter should be instance of Magento\Framework\Event\Observer which is child of Magento\Framework\DataObject and NOT related to Magento\Framework\Event\ObserverInterface.

To catch such kind of an issue I propose you to add logic to closure and invoke FlushFormKey ::execute inside.

@osrecio
Copy link
Member Author

osrecio commented Dec 1, 2017

Thanks @vkublytskyi I was going to work on the changes this weekend. Busy Week

@vkublytskyi
Copy link

@osrecio I will resolve minor fixes to free your time for more interesting things)

@magento-team magento-team merged commit 04a1c77 into magento:2.2-develop Dec 1, 2017
magento-team pushed a commit that referenced this pull request Dec 1, 2017
MAGETWO-83287: #11825: Generate new FormKey and replace for oldReques…
fd23e7c 
…tParams Wishlist #12038
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@vkublytskyi vkublytskyi vkublytskyi approved these changes

@ihor-sviziev ihor-sviziev ihor-sviziev approved these changes

Assignees

@vkublytskyi vkublytskyi

@ihor-sviziev ihor-sviziev

Labels
Award: test coverage duplicate Progress: accept Release Line: 2.2 Reproduced on 2.1.x The issue has been reproduced on latest 2.1 release Reproduced on 2.2.x The issue has been reproduced on latest 2.2 release Reproduced on 2.3.x The issue has been reproduced on latest 2.3 release
Projects
None yet
Milestone
December 2017
Development

Successfully merging this pull request may close these issues.
5 participants
@osrecio @vkublytskyi @ihor-sviziev @magento-team @okorshenko