Fix #21692 #21752 - logic in constructor of address validator and Locale Resolver check #21693

Bartlomiejsz · 2019-03-11T15:25:03Z

Description (*)

This fixes issue #21692 - error is described there

Fixed Issues (if relevant)

Incorrect constructor of Magento\Sales\Model\Order\Address\Validator #21692: Incorrect constructor of Magento\Sales\Model\Order\Address\Validator
Error while installing Magento from scratch if Locale Resolver is injected in cli command #21752: Error while installing Magento from scratch if Locale Resolver is injected in cli command

Manual testing scenarios (*)

Add custom module with Cli Command and inject Magento\Sales\Model\Order\Address\Validator into its constructor
Run Magento installation from Cli
Verify if installation is finished with success

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)

… to validate method

magento-engcom-team · 2019-03-11T15:25:09Z

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-engcom-team give me test instance - deploy test instance based on PR changes
@magento-engcom-team give me 2.3-develop instance - deploy vanilla Magento instance

For more details, please, review the Magento Contributor Assistant documentation

magento-engcom-team · 2019-03-12T13:16:37Z

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

magento-engcom-team · 2019-03-12T13:16:39Z

@Bartlomiejsz thank you for contributing. Please accept Community Contributors team invitation here to gain extended permissions for this repository.

soleksii · 2019-03-14T10:44:56Z

Hi @Bartlomiejsz !

I've checked PR and it looks like changes does not solve the problem.

Manual testing scenario:

Add custom module with Cli Command and inject Magento\Sales\Model\Order\Address\Validator into its constructor;
Try to install Magento from scratch using Cli;
See following error:

Bartlomiejsz · 2019-03-14T10:59:25Z

Hi @stoleksiy, this is another error. I wasn't aware that it also will appear for this class. I will add another issue and prepare PR later. Seems that those two are just combined and the other one have to be merged first.

soleksii · 2019-03-14T11:59:32Z

@Bartlomiejsz But, how can i check solution to the issue #21692 ? You need to solve the problem in scope of this PR.

Bartlomiejsz · 2019-03-14T12:18:17Z

@stoleksiy ok then, I will add fixing changes to this PR

Bartlomiejsz · 2019-03-14T16:30:35Z

@stoleksiy I've added fix for second issue

ihor-sviziev · 2019-03-15T06:03:37Z

lib/internal/Magento/Framework/Locale/Resolver.php

+    private function getDeploymentConfig()
+    {
+        if (!$this->deploymentConfig) {
+            $this->deploymentConfig = ObjectManager::getInstance()->get(DeploymentConfig::class);


Could you move deploymentConfig as last argument in constructor? It's not a good idea to get it through ObjectManager.

See "Adding a constructor parameter" section in https://devdocs.magento.com/guides/v2.3/contributor-guide/backward-compatible-development/

Also look at failing unit tests. Please fix them

@ihor-sviziev please check again now

This reverts commit b86f981.

magento-engcom-team · 2019-03-19T07:01:39Z

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

soleksii · 2019-03-19T08:39:46Z

✔️ QA Passed

Result:

…21692_incorrect_constructor

ghost · 2019-03-25T13:55:31Z

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.

…21693

Move Magento\Sales\Model\Order\Address\Validator logic from construct…

28b047e

… to validate method

magento-engcom-team added Component: Sales Release Line: 2.3 labels Mar 11, 2019

ihor-sviziev self-assigned this Mar 12, 2019

ihor-sviziev approved these changes Mar 12, 2019

View reviewed changes

ghost added the Progress: ready for testing label Mar 12, 2019

Bartlomiejsz mentioned this pull request Mar 12, 2019

[Backport] Fix #21692 #21752 - logic in constructor of address validator and Locale Resolver check #21719

Merged

4 tasks

soleksii self-assigned this Mar 14, 2019

ghost added Progress: testing in progress and removed Progress: ready for testing labels Mar 14, 2019

ghost added Progress: needs update and removed Progress: testing in progress labels Mar 14, 2019

Don't check default locale if Magento is not installed

b92ffa2

Bartlomiejsz mentioned this pull request Mar 14, 2019

Error while installing Magento from scratch if Locale Resolver is injected in cli command #21752

Closed

Bartlomiejsz changed the title ~~Fix #21692 - logic in constructor of address validator~~ Fix #21692 #21752 - logic in constructor of address validator and Locale Resolver check Mar 14, 2019

ihor-sviziev requested changes Mar 15, 2019

View reviewed changes

Bartlomiejsz added 3 commits March 18, 2019 09:21

Move deploymentConfig to constructor in Locale Resolver

fda4377

fix NumberFormatter initialization when no currency is set

b2004c3

fix de_CH group symbol in unit test

b86f981

ihor-sviziev self-requested a review March 18, 2019 09:27

Revert "fix de_CH group symbol in unit test"

73d88aa

This reverts commit b86f981.

Bartlomiejsz removed the Progress: needs update label Mar 18, 2019

ghost added the Progress: needs update label Mar 18, 2019

magento deleted a comment Mar 18, 2019

ihor-sviziev approved these changes Mar 19, 2019

View reviewed changes

ghost added Progress: ready for testing and removed Progress: needs update labels Mar 19, 2019

magento-engcom-team added the Component: Locale label Mar 19, 2019

ghost added Progress: testing in progress and removed Progress: ready for testing labels Mar 19, 2019

ghost added Progress: extended testing and removed Progress: testing in progress labels Mar 19, 2019

nmalevanec self-assigned this Mar 19, 2019

nmalevanec added 2 commits March 19, 2019 14:15

Merge remote-tracking branch 'mainline/2.3-develop' into feature/fix_…

9c7fbfd

…21692_incorrect_constructor

Fix static tests.

e09429f

ghost added Progress: accept and removed Progress: extended testing labels Mar 20, 2019

magento-engcom-team added the Award: bug fix label Mar 20, 2019

magento-engcom-team mentioned this pull request Mar 25, 2019

Incorrect constructor of Magento\Sales\Model\Order\Address\Validator #21692

Closed

magento-engcom-team merged commit e09429f into magento:2.3-develop Mar 25, 2019

magento-engcom-team pushed a commit that referenced this pull request Mar 25, 2019

ENGCOM-4482: Fix #21692 - logic in constructor of address validator #…

c10bf9c

…21693

magento-engcom-team added this to the Release: 2.3.2 milestone Mar 25, 2019

Bartlomiejsz mentioned this pull request Apr 4, 2019

Console command can break setup #16173

Closed

Bartlomiejsz deleted the feature/fix_21692_incorrect_constructor branch April 8, 2019 11:10

marcusirgens mentioned this pull request May 7, 2019

Unable to install Magento 2 if a console command depends on ProductRepositoryInterface #22777

Closed

Bartlomiejsz mentioned this pull request Jun 26, 2019

Fix PR owner magento/devdocs#4841

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix #21692 #21752 - logic in constructor of address validator and Locale Resolver check #21693

Fix #21692 #21752 - logic in constructor of address validator and Locale Resolver check #21693

Bartlomiejsz commented Mar 11, 2019 •

edited

Loading

magento-engcom-team commented Mar 11, 2019

magento-engcom-team commented Mar 12, 2019

magento-engcom-team commented Mar 12, 2019

soleksii commented Mar 14, 2019

Bartlomiejsz commented Mar 14, 2019

soleksii commented Mar 14, 2019 •

edited

Loading

Bartlomiejsz commented Mar 14, 2019

Bartlomiejsz commented Mar 14, 2019

ihor-sviziev Mar 15, 2019

ihor-sviziev Mar 15, 2019

Bartlomiejsz Mar 18, 2019

magento-engcom-team commented Mar 19, 2019

soleksii commented Mar 19, 2019

ghost commented Mar 25, 2019

Fix #21692 #21752 - logic in constructor of address validator and Locale Resolver check #21693

Fix #21692 #21752 - logic in constructor of address validator and Locale Resolver check #21693

Conversation

Bartlomiejsz commented Mar 11, 2019 • edited Loading

Description (*)

Fixed Issues (if relevant)

Manual testing scenarios (*)

Contribution checklist (*)

magento-engcom-team commented Mar 11, 2019

magento-engcom-team commented Mar 12, 2019

magento-engcom-team commented Mar 12, 2019

soleksii commented Mar 14, 2019

Bartlomiejsz commented Mar 14, 2019

soleksii commented Mar 14, 2019 • edited Loading

Bartlomiejsz commented Mar 14, 2019

Bartlomiejsz commented Mar 14, 2019

ihor-sviziev Mar 15, 2019

Choose a reason for hiding this comment

ihor-sviziev Mar 15, 2019

Choose a reason for hiding this comment

Bartlomiejsz Mar 18, 2019

Choose a reason for hiding this comment

magento-engcom-team commented Mar 19, 2019

soleksii commented Mar 19, 2019

ghost commented Mar 25, 2019

Bartlomiejsz commented Mar 11, 2019 •

edited

Loading

soleksii commented Mar 14, 2019 •

edited

Loading