Skip to content

Prevent leaking of variables to global scope #27622

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 6 commits into from
May 19, 2020
Merged

Prevent leaking of variables to global scope #27622

merged 6 commits into from
May 19, 2020

Conversation

krzksz
Copy link
Contributor

@krzksz krzksz commented Apr 5, 2020 •
edited by ghost
Loading

Description (*)

This PR adjusts minification file resolver for RequreJS that is generated to prevent leaking of variables it uses to global scope.

Manual testing scenarios (*)

  1. JavaScript minification still works properly and correct .min.js files are requested.
  2. There is no ctx, origNameToUrl and baseUrl variables under window leaked.

Resolved issues:

  1. resolves [Issue] Prevent leaking of variables to global scope #28110: Prevent leaking of variables to global scope

@m2-assistant
Copy link

m2-assistant bot commented Apr 5, 2020

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

@ihor-sviziev ihor-sviziev self-assigned this Apr 5, 2020
ihor-sviziev
ihor-sviziev requested changes Apr 6, 2020
View reviewed changes
@ghost ghost assigned krzksz Apr 8, 2020
Mateusz Krzeszowiak added 2 commits April 8, 2020 19:59
@magento-engcom-team
Copy link
Contributor

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

@engcom-Alfa engcom-Alfa self-assigned this Apr 14, 2020
@engcom-Alfa
Copy link
Contributor

✔️ QA Passed

JS Minification still works as previously(expected)

@sdzhepa
Copy link
Contributor

sdzhepa commented May 5, 2020

@magento create issue

@ghost ghost mentioned this pull request May 5, 2020
Closed
@VladimirZaets VladimirZaets added Auto-Tests: Not Covered Changes in Pull Request requires coverage by auto-tests and removed Auto-Tests: Covered All changes in Pull Request is covered by auto-tests labels May 6, 2020
slavvka
slavvka requested changes May 12, 2020
View reviewed changes
Copy link
Member

@slavvka slavvka left a comment

Choose a reason for hiding this comment

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

@krzksz please resolve the conflicts and re-run tests after upgrade to PHP 7.4

@ghost ghost assigned slavvka May 12, 2020
@ghost ghost dismissed ihor-sviziev’s stale review May 12, 2020 18:02

Pull Request state was updated. Re-review required.

@sdzhepa sdzhepa added the QA: Added to Regression Scope Scenario was analysed and added to Regression Testing Scope label May 13, 2020
@VladimirZaets VladimirZaets added Auto-Tests: Not Required Changes in Pull Request does not require coverage by auto-tests and removed Auto-Tests: Not Covered Changes in Pull Request requires coverage by auto-tests labels May 13, 2020
@ihor-sviziev
Copy link
Contributor

@magento run all tests

@ghost ghost added Progress: pending review and removed QA: Added to Regression Scope Scenario was analysed and added to Regression Testing Scope Progress: needs update labels May 18, 2020
@sdzhepa sdzhepa added the QA: Added to Regression Scope Scenario was analysed and added to Regression Testing Scope label May 18, 2020
@ghost ghost assigned VladimirZaets May 18, 2020
@magento-engcom-team
Copy link
Contributor

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

slavvka added a commit that referenced this pull request May 19, 2020
@slavvka
ENGCOM-7286: Prevent leaking of variables to global scope #27622
6ffb0d3
@slavvka slavvka merged commit df38cb6 into magento:2.4-develop May 19, 2020
@m2-assistant
Copy link

m2-assistant bot commented May 19, 2020

Hi @krzksz, 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

@slavvka slavvka slavvka requested changes

@VladimirZaets VladimirZaets VladimirZaets approved these changes

@ihor-sviziev ihor-sviziev ihor-sviziev left review comments

Assignees

@krzksz krzksz

@ihor-sviziev ihor-sviziev

@VladimirZaets VladimirZaets

@slavvka slavvka

@engcom-Alfa engcom-Alfa

@engcom-Echo engcom-Echo

Labels
Auto-Tests: Not Required Changes in Pull Request does not require coverage by auto-tests Component: RequireJs Event: Global-Contribution-Day improvement Partner: creativestyle partners-contribution Pull Request is created by Magento Partner 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: S2 Major restrictions or short-term circumventions are required until a fix is available.
Projects
None yet
Milestone
2.4.1
Development

Successfully merging this pull request may close these issues.

[Issue] Prevent leaking of variables to global scope
8 participants
@krzksz @magento-engcom-team @engcom-Alfa @ihor-sviziev @sdzhepa @VladimirZaets @slavvka @engcom-Echo