Fix #10583: Checkout place order exception when using a new address #11556

joni-jones · 2017-10-19T11:20:18Z

Fixed Issues

Checkout place order exception when using a new address #10583: Checkout place order exception when using a new address

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)

orlangur · 2017-10-19T12:17:21Z

lib/internal/Magento/Framework/Reflection/Test/Unit/TypeProcessorTest.php

@@ -25,6 +27,12 @@ class TypeProcessorTest extends \PHPUnit\Framework\TestCase
    protected function setUp()
    {
        $this->_typeProcessor = new \Magento\Framework\Reflection\TypeProcessor();
+        $loader = new ClassLoader();
+        $loader->addPsr4(


Why do we need to hack autoloader instead of placing fixtures accordingly?

To do not use _files word in the class namespace.

You don't need to use _files and do manual include. Just place fixtures in something like lib/internal/Magento/Framework/Reflection/Test/Unit/TypeProcessorTest/Fixture/TSample.php.

If you open integration or unit tests you will see all fixtures located in _files directory. I don't want to change such "standard".

There is no such standard. I remember such cases in integration tests just because there is no class usually.

When we can make our code PSR-4 compliant we should do so. Please remove unnecessary ClassLoader complication, other _files occurrences containing PHP classes should be considered as technical debt.

orlangur · 2017-10-19T12:20:53Z

lib/internal/Magento/Framework/Reflection/Test/Unit/_files/TSample.php

+class TSample implements TSampleInterface
+{
+    /**
+     * {@inheritdoc}


Such syntax only inherits method description, not type hints for parameters and return value.

See https://github.com/php-fig/fig-standards/blob/master/proposed/phpdoc.md#61-making-inheritance-explicit-using-the-inheritdoc-tag

Please change implementation in such a way that we do not allow misuse of syntax with curly braces and only inherit return/param in case of @inheritdoc.

According to Magento internal guidelines, this annotation should look like {@inheritdoc}.

Where do you see it? http://devdocs.magento.com/guides/v2.2/coding-standards/docblock-standard-general.html#inheritdoc is compliant with what I'm saying.

From internal Magento wiki but I agree with @inheritdoc usage, even I think it's better to use it.

Great 👍 Yeah, seems to be some outdated documentation, please change/remove it or delegate this to whoever is in charge of it.

orlangur · 2017-10-19T12:22:25Z

dev/tests/integration/testsuite/Magento/Quote/Model/QuoteTest.php

@@ -393,7 +393,7 @@ protected function _getCustomerDataArray()
            \Magento\Customer\Model\Data\Customer::DOB => '2014-02-03 00:00:00',
            \Magento\Customer\Model\Data\Customer::EMAIL => 'qa@example.com',
            \Magento\Customer\Model\Data\Customer::FIRSTNAME => 'Joe',
-            \Magento\Customer\Model\Data\Customer::GENDER => 'Male',
+            \Magento\Customer\Model\Data\Customer::GENDER => 0,


Why need this change?

Fixed fixture implementation according to interface doc block annotation https://github.com/magento/magento2/blob/2.2-develop/app/code/Magento/Customer/Api/Data/CustomerInterface.php#L269. After replacing get_class by namespace this fixture breaks test.

joni-jones · 2017-10-20T07:03:51Z

The class complexity is 103 but Codacy configured complexity threshold is 50.

orlangur · 2017-10-20T08:57:36Z

@joni-jones I don't think we should any suppressions when testCodeMess is passed but Codacy complains.

joni-jones · 2017-10-20T09:12:39Z

@orlangur, this class has 7 deprecated methods, which increase complexity, I can't them remove because it will be BIC. Magento class complexity configured as 100 (https://github.com/magento/magento2/blob/2.2-develop/dev/tests/static/testsuite/Magento/Test/Php/_files/phpmd/ruleset.xml#L24) but codacy complexity is 50. So without full refactoring, the codacy test for this class will never be green.

orlangur · 2017-10-20T09:32:34Z

@joni-jones do you mean green in Travis or green in Codacy? The latter is pretty odd, like forbidding else usage, contains TooLongVariable rule which was intentionally removed from Magento PHPMD ruleset.

joni-jones · 2017-10-20T09:40:08Z

I mean green Codacy. Removing deprecated methods will decrease complexity, but the class also requires refactoring like the composition usage, unit tests. I think it should be done in some release but not in a patch release and not during this bug fix.

orlangur · 2017-10-20T09:37:52Z

lib/internal/Magento/Framework/Reflection/TypeProcessor.php

+                    $parentClassReflection->getMethod($methodName)
+                );
+
+                break;


Currently logic of this cycle is not so easy to understand. I suggest something like

foreach (...) { if ($parent...->hasMethod()) { $returnAnnotations = ...; break; } }

Don't see it :(

Please, see a list of changed files, I amended commit.

Hm, I was answering to next thread actually O.o

orlangur · 2017-10-20T09:38:30Z

lib/internal/Magento/Framework/Reflection/TypeProcessor.php

+        $methodDocBlock = $methodReflection->getDocBlock();
+        if (!$methodDocBlock) {
+            throw new \InvalidArgumentException(
+                "Each getter must have dock block. "


Typo dock -> doc. "have a doc block"?

"Each getter must have a dock block. "

I meant don't see this changed.

https://github.com/magento/magento2/pull/11556/files#diff-292261de777138aa6bb4342b9757a3a5R744

"dock" is correct spelling? I was not certain regarding "a" but "dock" seems to be not correct.

Yes, sure. Fixed.

orlangur · 2017-10-20T09:40:48Z

lib/internal/Magento/Framework/Reflection/Test/Unit/Fixture/TSample.php

+    }
+
+    /**
+     * @inheritdoc


Would be really nice to add test case with {@inheritdoc} to make it explicit such syntax is not supposed to work. I do not insist though, this PR is already excellent.

I thought about it, but a lot of Magento code has such annotation. The current unit test covers such case for parsing return tag because the behavior just checks if a method has @return annotation.

a lot of Magento code has such annotation

We should fix it anyway. Not saying it must be a scope of this PR, but it could be.

Absolutely agree, even more, I think there should be a static test which won't allow adding this annotation.

orlangur · 2017-10-20T09:43:03Z

@joni-jones ok, thanks. According to my understanding we should ignore invalid Codacy complaints but I'm not sure if we have a requirement to make it green (especially when "making green" is just adding suppression).

Please remove this suppression if it is not mandatory to make Codacy green.

joni-jones · 2017-10-20T09:47:57Z

This suppress needed for Travis and Bamboo builds because the class has complexity 103, after small refactoring, without changing functionality I've got - 101. To reduce it need to remove deprecated methods and do deep refactoring.

orlangur · 2017-10-20T09:54:47Z

Aaah, ok. So green Bamboo actually, thanks. I'll raise a topic regarding Codacy in Slack, we should do something to it.

- Added doc block parsing for interceptors

…a new address #11556

VoidWalker

Missing inheridocs from extended classes

VoidWalker · 2018-01-19T13:35:51Z

lib/internal/Magento/Framework/Reflection/TypeProcessor.php

+        $returnAnnotations = $this->getReturnFromDocBlock($methodReflection);
+        if (empty($returnAnnotations)) {
+            // method can inherit doc block from implemented interface, like for interceptors
+            $implemented = $methodReflection->getDeclaringClass()->getInterfaces();


Sir, you're missing docs from extended class here. $methodReflection->getDeclaringClass()->getParentClass() should be included in this array

Hi @VoidWalker, please report an issue using a template consisting of full steps to reproduce.

ishakhsuvarov self-assigned this Oct 19, 2017

ishakhsuvarov added this to the October 2017 milestone Oct 19, 2017

ishakhsuvarov added Release Line: 2.2 bug report hacktoberfest labels Oct 19, 2017

orlangur requested changes Oct 19, 2017

View reviewed changes

orlangur self-assigned this Oct 19, 2017

joni-jones force-pushed the G#10583 branch from 3428ecc to 09c75be Compare October 19, 2017 16:20

orlangur requested changes Oct 20, 2017

View reviewed changes

joni-jones force-pushed the G#10583 branch from 09c75be to 1ed0bae Compare October 20, 2017 10:08

Checkout place order exception when using a new address

fea77c7

- Added doc block parsing for interceptors

joni-jones force-pushed the G#10583 branch from 1ed0bae to fea77c7 Compare October 20, 2017 12:00

orlangur approved these changes Oct 20, 2017

View reviewed changes

okorshenko modified the milestones: October 2017, November 2017 Nov 1, 2017

okorshenko removed the hacktoberfest label Nov 1, 2017

magento-engcom-team added the bugfix label Nov 7, 2017

okorshenko merged commit fea77c7 into magento:2.2-develop Nov 7, 2017

okorshenko pushed a commit that referenced this pull request Nov 7, 2017

MAGETWO-82314: Fix #10583: Checkout place order exception when using …

5109d8c

…a new address #11556

magento-engcom-team added the Fixed in 2.2.x The issue has been fixed in 2.2 release line label Nov 7, 2017

VoidWalker reviewed Jan 19, 2018

View reviewed changes

joni-jones deleted the G#10583 branch July 2, 2018 07:16

Fix #10583: Checkout place order exception when using a new address #11556

Fix #10583: Checkout place order exception when using a new address #11556

Conversation

joni-jones commented Oct 19, 2017

Fixed Issues

Contribution checklist

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

orlangur Oct 19, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joni-jones Oct 19, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joni-jones commented Oct 20, 2017 • edited

orlangur commented Oct 20, 2017

joni-jones commented Oct 20, 2017 • edited

orlangur commented Oct 20, 2017

joni-jones commented Oct 20, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

orlangur Oct 20, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joni-jones Oct 20, 2017 • edited

Choose a reason for hiding this comment

orlangur commented Oct 20, 2017

joni-jones commented Oct 20, 2017

orlangur commented Oct 20, 2017

VoidWalker left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

orlangur Oct 19, 2017 •

edited

joni-jones Oct 19, 2017 •

edited

joni-jones commented Oct 20, 2017 •

edited

joni-jones commented Oct 20, 2017 •

edited

orlangur Oct 20, 2017 •

edited

joni-jones Oct 20, 2017 •

edited