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

Support for non-FQN classes on web-API interfaces. #17424

Conversation

phoenix128
Copy link
Contributor

@phoenix128 phoenix128 commented Aug 7, 2018

This commit provides a support for "use" statement in web-API interfaces and the usage of non-FQN class names in doctypes. It also provides a fix for MSI-1524 and MSI-1537.

Description

Before this commit you were not able to import class names in interfaces used for web-API.

For example, the following interface was not working with the non-FQN name MyClass imported through the use alias definition.

namespace ...;

use My\Module\MyClass;

interface SomeInterface
{
...
/**
 * @param MyClass $param
 * ...
 */
 public function getMyMethod()
 ...
...
}

Now, through a new method \Magento\Framework\Reflection\TypeProcessor::resolveFullyQualifiedClassName, the FQN is automatically resolved if an alias or a short name is provided.

Fixed Issues (if relevant)

  1. Provide robust fix for nested object instantiation via Web API inventory#1537: Provide robust fix for nested object instantiation via Web API
  2. POST V1/inventory/source-selection-algorithm-result throws exception inventory#1524: POST V1/inventory/source-selection-algorithm-result throws exception

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)

This commit provides a support for "use" statement in web-API interfaces and the usage of non-FQN class names in doctypes.
It also provides a fix for MSI-1524 and MSI-1537.
@magento-engcom-team magento-engcom-team added Partner: MageSpecialist partners-contribution Pull Request is created by Magento Partner labels Aug 7, 2018
@magento-engcom-team
Copy link
Contributor

Hi @phoenix128. 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 {$VERSION} instance - deploy vanilla Magento instance

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

@magento-engcom-team magento-engcom-team added Component: Framework/Webapi USE ONLY for FRAMEWORK RELATED BUG! E.g If bug related to Catalog WEB API use just Catalog Component: Framework/Reflection Release Line: 2.3 labels Aug 7, 2018
@orlangur orlangur self-assigned this Aug 8, 2018
@maghamed
Copy link
Contributor

maghamed commented Sep 9, 2018

@phoenix128 looks like the Integration build is broken on your latest PR

@ishakhsuvarov ishakhsuvarov added this to the Release: 2.3.1 milestone Sep 18, 2018
@phoenix128
Copy link
Contributor Author

@maghamed , trying to understand why it breaks the integration tests.

@maghamed maghamed mentioned this pull request Oct 7, 2018
4 tasks
@phoenix128
Copy link
Contributor Author

@maghamed , it should be fixed now.

@phoenix128
Copy link
Contributor Author

@ishakhsuvarov , I have some failing tests in the code quality of test classes I use as fixture, should I ignore it?

@phoenix128
Copy link
Contributor Author

@ishakhsuvarov , any news here? Thanks.

@ishakhsuvarov
Copy link
Contributor

@phoenix128 Sorry for the delay. I don't have any major comments, hopefully it's getting merge soon enough.

* @param ClassReflection $sourceClass
* @return array
*/
public function getAliasMapping(ClassReflection $sourceClass): array
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please, consider making methods private, looks like they used only in the private implementation

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @sidolov ,
it is used in \Magento\Framework\Reflection\Test\Unit\TypeProcessorTest. We have two options: remove tests or eject this method to a dedicated service.

*/
public function isSimpleType(string $typeName): bool
{
return strtolower($typeName) === $typeName;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can't help but feel like this should be a check against a dictionary of reserved words...

Wouldn't something like use \Magento\Framework\App\ObjectManager as om; cause weird problems and error messages that wouldn't make sense?

While it's not the way one should be writing code (subjective), I think we can still prevent the issue if its tackled here?

@okorshenko okorshenko removed this from the Release: 2.3.1 milestone Jan 28, 2019
@magento-engcom-team
Copy link
Contributor

Hi @sidolov, thank you for the review.
ENGCOM-4046 has been created to process this Pull Request

@ghost
Copy link

ghost commented Feb 10, 2019

Hi @phoenix128, 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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Award: bug fix Award: complex Component: Framework/Reflection Component: Framework/Webapi USE ONLY for FRAMEWORK RELATED BUG! E.g If bug related to Catalog WEB API use just Catalog Partner: MageSpecialist partners-contribution Pull Request is created by Magento Partner Progress: accept Release Line: 2.3
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants