Skip to content

Conversation

@hws47a
Copy link
Contributor

@hws47a hws47a commented Jul 6, 2019

Description (*)

  • Improve Magento\AdobeStockAssetApi\Api\Data\AssetInterface:
    • Fix incorrect PhpDoc: set methods return void instead of $this
    • Add void return type to methods with appropriate phpdoc
    • change setId and setExtensionAttributes method signatures to return void instead of self as it should be by design.
  • Change Magento\AdobeStockAsset\Model\Asset to fit it's interface

@magento-cicd2
Copy link

magento-cicd2 commented Jul 6, 2019

CLA assistant check
All committers have signed the CLA.

@sivaschenko
Copy link
Member

Hi @hws47a , thanks for the pull request!

The proposed changes do make sense in the context of strict CQRS principle following. However, could you please share what is this approach based on? Are there any particular standards or guidelines that you've used?

@hws47a
Copy link
Contributor Author

hws47a commented Jul 7, 2019

@sivaschenko

For explicit return types:
Magento technical guidelines:

1.2. Explicit return types MUST BE declared on functions.

For changed $this to void in setId and setExtensionAttributes:
I cannot find where exactly I've seen that it's a recommended way.

As example I can show two newest Magento core classes:
Inventory: https://github.com/magento/inventory/blob/2.3-develop/app/code/Magento/InventoryApi/Api/Data/SourceInterface.php
GraphQlCache: https://github.com/magento/graphql-ce/blob/2.3-develop/app/code/Magento/GraphQlCache/Model/CacheableQuery.php

Anyway, all of other methods returns void and I believe class should follow same design for all methods.

@sivaschenko sivaschenko requested a review from sidolov July 8, 2019 12:01
@sivaschenko
Copy link
Member

Great, thank you! I am ok with convention that all setters in adobe-stock-integration API will return void. @sidolov any objections?

@sidolov sidolov merged commit 672b3d2 into magento:develop Jul 8, 2019
magento-devops-reposync-svc pushed a commit that referenced this pull request Oct 14, 2025
…-09052025

Cia pre release develop sync 09052025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants