Add type declaration to setter methods of generated ExtensionAttribute classes

[erikhansen]

The code that generates the classes that implement the \Magento\Framework\Api\ExtensionAttributesInterface interface were doing so in a way that did not add type declarations to the setter methods. This pull request adds functionality to add type declarations, but only for types that are valid type declarations. It is important to have type declarations added to the setter methods of ExtensionAttribute classes as it ensures that the getter methods will always return objects of the proper interface/class.

Summary of changes:

• Modified \Magento\Framework\Api\Code\Generator\ExtensionAttributesGenerator::_getClassMethods method to add type declaration to method, as long as type is a valid (e.g. not a primitive type or an array of classes/interfaces).
• Added isValidTypeDeclaration method to \Magento\Framework\Reflection\TypeProcessor to centralize the logic for determining whether a string is valid as a type declaration. When and if Magento 2 supports PHP 7, the logic of this method should be changed to support primitive types. I referenced the PHP Type Declaration section when writing the \Magento\Framework\Reflection\Test\Unit\TypeProcessorTest::testIsValidTypeDeclaration test. I wrote what I believe to be a very comprehensive set of true and false assertions.

Here is an example of how this auto-generated file changes: var/generation/Magento/Catalog/Api/Data/ProductAttributeMediaGalleryEntryExtension.php.

Before pull request

<?php
namespace Magento\Catalog\Api\Data;

/**
 * Extension class for @see
 * \Magento\Catalog\Api\Data\ProductAttributeMediaGalleryEntryInterface
 */
class ProductAttributeMediaGalleryEntryExtension extends \Magento\Framework\Api\AbstractSimpleObject implements \Magento\Catalog\Api\Data\ProductAttributeMediaGalleryEntryExtensionInterface
{
    /**
     * @return \Magento\Framework\Api\Data\VideoContentInterface|null
     */
    public function getVideoContent()
    {
        return $this->_get('video_content');
    }

    /**
     * @param \Magento\Framework\Api\Data\VideoContentInterface $videoContent
     * @return $this
     */
    public function setVideoContent($videoContent)
    {
        $this->setData('video_content', $videoContent);
        return $this;
    }
}

After pull request

You'll notice that the setVideoContent method has a \Magento\Framework\Api\Data\VideoContentInterface type declaration.

<?php
namespace Magento\Catalog\Api\Data;

/**
 * Extension class for @see
 * \Magento\Catalog\Api\Data\ProductAttributeMediaGalleryEntryInterface
 */
class ProductAttributeMediaGalleryEntryExtension extends \Magento\Framework\Api\AbstractSimpleObject implements \Magento\Catalog\Api\Data\ProductAttributeMediaGalleryEntryExtensionInterface
{
    /**
     * @return \Magento\Framework\Api\Data\VideoContentInterface|null
     */
    public function getVideoContent()
    {
        return $this->_get('video_content');
    }

    /**
     * @param \Magento\Framework\Api\Data\VideoContentInterface $videoContent
     * @return $this
     */
    public function setVideoContent(\Magento\Framework\Api\Data\VideoContentInterface $videoContent)
    {
        $this->setData('video_content', $videoContent);
        return $this;
    }
}

[Vinai]

👍 Hopefully authentication for travis builds will be fixed soon, so the PR can be processed further.

[vancoz]

Please merge latest from develop and rerun builds.

[erikhansen]

@vancoz It looks like the tests are failing because there is core code that is setting null values on methods that are now expecting objects of a certain type: PHP Catchable fatal error: Argument 1 passed to Magento\Sales\Api\Data\OrderPaymentExtension::setVaultPaymentToken() must implement interface Magento\Vault\Api\Data\PaymentTokenInterface, null given, called in /home/travis/build/magento/magento2/app/code/Magento/Vault/Plugin/PaymentVaultAttributesLoad.php on line 62 and defined in /home/travis/build/magento/magento2/dev/tests/integration/tmp/sandbox-0-f56255a0efbdc640bbc76576a01f0349/var/generation/Magento/Sales/Api/Data/OrderPaymentExtension.php on line 21

(From https://travis-ci.org/magento/magento2/jobs/102137070#L587)

Can you have one of your architects review this pull request and confirm that they would like to accept this pull request, assuming the build is green? If so, then I can spend the time to refactor any core Magento code that tries to use an extension attribute method to set a value that isn't an instanceof the proper interface. But I only want to spend this time if this PR is going to be merged.

My refactoring will look for any places where a null value could be passed to an extension attribute method that expects an object that implements the defined interface, and if so, it won't try to set a value.

[okorshenko]

Change makes sense, thanks for contribution! Could you please fix Travis builds so that we will be able accept this PR.

[erikhansen]

@okorshenko Are you confirming that you would accept this PR if I make the changes I mentioned on January 14th?

[okorshenko]

@erikhansen , yes, I do

[okorshenko]

PR is Accepted, but tests should be fixed before merge

[okorshenko]

Hi @erikhansen. Should we wait for tests fixes for this Pull Request or this PR can be closed?

[erikhansen]

@okorshenko I just fixed the issue with the test that failed back in January. I'll see what additional tests fail and resolve those issue, assuming the fixes are not too intensive.

[erikhansen]

@okorshenko Travis tests are now green, so this PR should be ready to merge into develop.

[erikhansen]

@okorshenko Bump.

[okorshenko]

Hi @erikhansen . We are running internal builds against your branch.

[okorshenko]

@erikhansen we successfully executed all builds against your branch. There are couple issues that we need to fix before merging this PR.

[erikhansen]

@okorshenko Ok, good. Do I need to fix the issues or are you going to take care of them? If the former, can you let me know what the issues are?

[okorshenko]

param annotation should looks like:

• @param \Magento\Bundle\Api\Data\BundleOptionInterface $complexObjectAttributeWithTypeDeclaration

but this is not issue of this PR

[okorshenko]

@erikhansen I added some comments to show you what issue we found in this PR. We will fix this and and will run all tests again. Thank you for your contribution.

[adragus-inviqa]

@okorshenko - Interesting approach to keeping BC, I must say. But it works.

Slightly offtopic here, but do M2 devs agree with the whole Interface2, Interface3 approach in case of interface changes (the above snippet being for concrete classes only)?

[okorshenko]

@erikhansen your pull request merged to develop branch. Thank you for your contribution.