Skip to content
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.

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

Each getter must have description with @return annotation. #981

Conversation

fooman
Copy link
Contributor

@fooman fooman commented Jan 19, 2015

The autogenerated code for Interceptor classes is using the inheritdoc notation. This then does not pass checks on the class for correct docblock description and return type. For details to reproduce and confirmation this change fixes it see https://gist.github.com/mzeis/11a7231cd67a5216ec47

@okorshenko
Copy link
Contributor

Hi @fooman
Thank you for you contribution!
Please, checkout the latest available version (0.42.0-beta4). Your plugin should work on this version. There is no sense to merge this PR yet, since it just hides implementation errors (when someone relies on the API interface implementations instead of API interface)
The main idea is that our API clients should not know about API interface implementation. Our code must rely on the API interfaces only.
Please take a look at https://github.com/magento/magento2/blob/develop/app/code/Magento/Customer/Model/Customer/Mapper.php#L39

@fooman
Copy link
Contributor Author

fooman commented Jan 20, 2015

@okorshenko I can confirm that the error message indeed has gone away with beta4. Could you please explain what you meant with

The main idea is that our API clients should not know about API interface implementation. Our code must rely on the API interfaces only.

The plugin was intercepting

<type name="Magento\Customer\Api\Data\CustomerInterface">

Could you confirm if this is a correct/valid approach or if something else should have been chosen.

Any documentation / link to documentation that you can provide in terms of what to intercept when would be great.

@fooman fooman closed this Jan 20, 2015
@okorshenko
Copy link
Contributor

Could you please explain what you meant with

I meant that if you want to convert object to array using method Magento\Framework\Api\ExtensibleDataObjectConverter::toNestedArray($dataObject, $skipCustomAttributes, $dataObjectType) you can not rely on provided data object class, since this class can implements several different interfaces. You have to specify valid $dataObjectType that is one of the stabel API interface. In this case we relies on the API interface only, but not on one of the implementation of the API interface. Resulting we don't need to know about autogenerated interceptor interfaces (such as method ___callParent).
Your error was caused by this line of code https://gist.github.com/mzeis/11a7231cd67a5216ec47#file-error-L6:

#4 /var/www/magento2/app/code/Magento/Customer/Model/Customer/Mapper.php(38): Magento\Framework\Api\ExtensibleDataObjectConverter->toNestedArray(Object(Magento\Customer\Model\Data\Customer\Interceptor))

But luckily we have already fixed this issue on the our code base. Please, enjoy :)

Could you confirm if this is a correct/valid approach or if something else should have been chosen.

I confirm that this is valid approach.

Any documentation / link to documentation that you can provide in terms of what to intercept when would be great.

I'm not sure that we have some documentation that describes what to intercept and when. This depends on your concrete case. The general approach/best practices are:

  • pluginize stable API interfaces, if it is available (as you did in your code for Magento\Customer\Api\Data\CustomerInterface)
  • if API interface is not implemented, it is highly recommended to intercept the class method that is the most closer to functionality that you want to intercept (for instance, you don't need to intercept Layout model if you want to change behavior of one of the generated Block)
  • it is discouraged to use around interception strategy in the plugin classes without call of proceed closure (since this is a potentially conflicting modification. In this case you may break other 3rd party plugin's logic)

@fooman fooman deleted the fix-missing-method-description-when-using-inheritdoc branch January 27, 2016 03:31
@fooman fooman restored the fix-missing-method-description-when-using-inheritdoc branch January 27, 2016 04:19
magento-team pushed a commit that referenced this pull request Apr 3, 2017
[Falcons] Delivery of deployment improvements and bug fixes
magento-team pushed a commit that referenced this pull request Dec 11, 2017
 - Merge Pull Request magento-engcom/magento2ce#981 from p-bystritsky/magento2:ISSUE-12582
 - Merged commits:
   1. 0db6052
   2. 28cd092
magento-team pushed a commit that referenced this pull request Dec 11, 2017
[EngCom] Public Pull Requests - 2.2-develop
 - MAGETWO-85332: Fix error loading theme configuration on PHP 7.2 #12606
 - MAGETWO-85307: 12468: Sort by Price not working on CatalogSearch Page in Magento 2 #929
 - MAGETWO-85303: #12582: Can't remove item description from wishlist #981
 - MAGETWO-85297: 8410: Custom Checkout Step and Shipping Step are Highlighted and Combined upon Checkout page load #975
 - MAGETWO-85290: 7467: File Put Contents file with empty content. #962
magento-engcom-team added a commit that referenced this pull request Oct 13, 2019
 - Merge Pull Request magento/graphql-ce#983 from TomashKhamlai/graphql-ce:issue/981
 - Merged commits:
   1. 287e906
magento-engcom-team added a commit that referenced this pull request Oct 13, 2019
Accepted Community Pull Requests:
 - magento/graphql-ce#873: graphQl-509: `save_in_address_book` has no impact on Address Book (by @kisroman)
 - magento/graphql-ce#991: graphQl-987: [Test coverage] Cover exceptions in Magento\QuoteGraphQl� (by @kisroman)
 - magento/graphql-ce#995: magento/graphql-ce#985: Remove unnecessary exceptions (by @atwixfirster)
 - magento/graphql-ce#997: magento/graphql-ce#975: [Test coverage] Cover CartAddressTypeResolver (by @atwixfirster)
 - magento/graphql-ce#990: Extend test coverage for CustomerDownloadableGraphQl (by @TomashKhamlai)
 - magento/graphql-ce#983: #981 Extend test coverage for BraintreeGraphQl (by @TomashKhamlai)
 - magento/graphql-ce#973: GraphQl-972: added support of the Global scope in the config fixture (by @VitaliyBoyko)


Fixed GitHub Issues:
 - #975: [Question] Have traits been considered for the Interceptor classes? (reported by @fooman) has been fixed in magento/graphql-ce#997 by @atwixfirster in 2.3-develop branch
   Related commits:
     1. 493d744

 - #989: make build artifacts accessable (reported by @Flyingmana) has been fixed in magento/graphql-ce#990 by @TomashKhamlai in 2.3-develop branch
   Related commits:
     1. bccd287

 - #972: Pub/Static directory is empty (reported by @mrugeshrocks) has been fixed in magento/graphql-ce#973 by @VitaliyBoyko in 2.3-develop branch
   Related commits:
     1. 3489da5
     2. 63bd232
     3. b64a485
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants