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.

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

#7213 prevent pointless session start in webapi scope #26032

Merged
merged 20 commits into from
Aug 1, 2020
Merged

#7213 prevent pointless session start in webapi scope #26032

merged 20 commits into from
Aug 1, 2020

Conversation

maqlec
Copy link
Contributor

@maqlec maqlec commented Dec 13, 2019 •
edited by ihor-sviziev
Loading

Description (*)

Session is started even if is not needed. This issue was fixed in version 2.2 but there is no fix in 2.3 and the newest 2.4.
Formkey is readed from cookies also in Webapi scope what's starting session but there is no need to check it in webapi and start session for that

Fixed Issues (if relevant)

  1. Fixes WEBAPI: PHP session is always started 2.1.2 #7213 WEBAPI: PHP session is always started 2.1.2

Manual testing scenarios (*)

  1. curl an stateless endpoint in Magento rest api
  2. You will get a Set-Cookie header with PHPSESSID

Questions or comments

Nope

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
Copy link

m2-assistant bot commented Dec 13, 2019

Hi @maqlec. 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

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

@maqlec
Copy link
Contributor Author

maqlec commented Jan 23, 2020

@magento give me test instance

@magento-engcom-team
Copy link
Contributor

Hi @maqlec. Thank you for your request. I'm working on Magento instance for you

@magento-engcom-team
Copy link
Contributor

Hi @maqlec, here is your new Magento instance.
Admin access: https://pr-26032.instances.magento-community.engineering/admin_d936
Login: ae202496 Password: 36f23638280d
Instance will be terminated in up to 3 hours.

@ihor-sviziev
Copy link
Contributor

@magento run all tests

@ihor-sviziev ihor-sviziev self-assigned this Feb 6, 2020
ihor-sviziev
ihor-sviziev requested changes Feb 7, 2020
View reviewed changes
Copy link
Contributor

@ihor-sviziev ihor-sviziev left a comment

Choose a reason for hiding this comment

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

Hi @maqlec,
Looks like your changed introduces some regression. Could you review failing tests?

Also would be great to cover your changes with some tests, could you do that?

@maqlec
Copy link
Contributor Author

maqlec commented Feb 7, 2020

Of course, I'll do that.

@ihor-sviziev ihor-sviziev added the Auto-Tests: Not Covered Changes in Pull Request requires coverage by auto-tests label Feb 12, 2020
@opepin opepin mentioned this pull request Feb 24, 2020
Merged
@ihor-sviziev
Copy link
Contributor

@magento run Functional Tests B2B, Integration Tests, Unit Tests, WebAPI Tests

@ihor-sviziev
Copy link
Contributor

@magento run WebAPI Tests

Not sure how it could fail functiality, but currently following webapi tests failing, restarting...
image
image

ihor-sviziev
ihor-sviziev requested changes Mar 3, 2020
View reviewed changes
Copy link
Contributor

@ihor-sviziev ihor-sviziev left a comment

Choose a reason for hiding this comment

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

Hi @maqlec,
I tried to re-run few times failing tests - looks like this PR caused some issue in web api, or maybe test should be updated. Please fix that.
Additionally could you cover your changes with some kind of test, probably web api test?

app/code/Magento/PageCache/etc/frontend/di.xml Outdated Show resolved Hide resolved
@ihor-sviziev
Copy link
Contributor

Hi @maqlec,
Will you be able to update your PR?

@ihor-sviziev
Copy link
Contributor

ihor-sviziev commented Mar 25, 2020 •
edited
Loading

@engcom-Charlie @engcom-Golf could you help with failing WebAPI test?
This seems to me really important fix

@engcom-Kilo engcom-Kilo self-assigned this Mar 26, 2020
@ihor-sviziev
Copy link
Contributor

ihor-sviziev commented Mar 27, 2020 via email

@engcom-Kilo
Copy link
Contributor

engcom-Kilo commented Mar 27, 2020 •
edited
Loading

Are you sure? It was failing few times in row and didn’t failed on other PRs
…
On Thu, 26 Mar 2020 at 16:17, Alexander Steshuk @.***> wrote: @ihor-sviziev https://github.com/ihor-sviziev Failed WebAPI tests not related to the changes in this PR. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#26032 (comment)>, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAOJOUNBNR4D2BPHSQHFWCDRJNPYPANCNFSM4J2SONZA .

I am recheck again.

@ihor-sviziev
Copy link
Contributor

@magento run Database Compare

@ihor-sviziev
Copy link
Contributor

As for me changes looks good, but DB Compare test is fails, while no issues in report.

@lenaorobei @slavvka could you give us any info why DB Compare test fails?

@lenaorobei
Copy link
Contributor

@magento run Database Compare

@lenaorobei
Copy link
Contributor

@ihor-sviziev branch was outdated.

@ihor-sviziev
Copy link
Contributor

ihor-sviziev commented Apr 3, 2020 •
edited
Loading

Hi @engcom-Kilo,
seems like changes you’ve did in api tests are incorrect. For instance if you’ll move exception for non existing store to the start - seems like others checks will not happen, as there will be exception thrown earlier.

@maqlec It seems to me like change created regression in this part.

Could you re-check it? Maybe @engcom-Golf could help with it?

@engcom-Kilo
Copy link
Contributor

@ihor-sviziev
Updated WebAPI tests
Seems now are correctly

@engcom-Kilo
Copy link
Contributor

@ihor-sviziev
Should to cover with additional Test ?

@engcom-Kilo
Copy link
Contributor

Please to approve changes in this Pull Request.
Because these changes in Pull Request, will change logic of PHP Session. And can cause to incorrect work with functional, or with security logic.

@engcom-Kilo
Copy link
Contributor

@magento run all tests

@engcom-Kilo
Copy link
Contributor

@magento run all tests

@magento-engcom-team
Copy link
Contributor

Hi @ihor-sviziev, thank you for the review.
ENGCOM-7611 has been created to process this Pull Request

magento-engcom-team pushed a commit that referenced this pull request Aug 1, 2020
@slavvka
ENGCOM-7611: #7213 prevent pointless session start in webapi scope #2…
06f406e 
…6032
@magento-engcom-team magento-engcom-team merged commit d1124d9 into magento:2.4-develop Aug 1, 2020
@m2-assistant
Copy link

m2-assistant bot commented Aug 1, 2020

Hi @maqlec, 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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@engcom-Kilo engcom-Kilo engcom-Kilo left review comments

@slavvka slavvka slavvka requested changes

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

@kokoc kokoc Awaiting requested review from kokoc

@paliarush paliarush Awaiting requested review from paliarush

Assignees

@ihor-sviziev ihor-sviziev

@slavvka slavvka

@engcom-Alfa engcom-Alfa

@engcom-Delta engcom-Delta

@engcom-Kilo engcom-Kilo

Labels
Auto-Tests: Covered All changes in Pull Request is covered by auto-tests Award: bug fix Component: Customer Component: PageCache Component: User Priority: P2 A defect with this priority could have functionality issues which are not to expectations. Progress: accept QA: Added to Regression Scope Scenario was analysed and added to Regression Testing Scope Release Line: 2.4 Severity: S3 Affects non-critical data or functionality and does not force users to employ a workaround.
Projects
Pull Requests Dashboard
Archived in project
Milestone
2.4.1
Development

Successfully merging this pull request may close these issues.

WEBAPI: PHP session is always started 2.1.2
8 participants
@maqlec @magento-engcom-team @ihor-sviziev @engcom-Kilo @lenaorobei @engcom-Delta @slavvka @engcom-Alfa