Change Webapi extension to use Inject Pool logic

[nuzil]

Description

We extended Magento/Webapi extension. This allowed us to use Request Processors Pool and avoid of using if/else statements in \Magento\Webapi\Controller\Rest.php

So now any new processor can be easily added via webapi_rest/di.xml file in any extension.

This modification will add much more flexibility in Magento customisations.

Manual testing scenarios

Here is no new feature developed, so Manual test will be in testing default Magento API calls

[vrann]

please use same header as elsewhere in Magento. Just configured Travis for the project, so your next commit should trigger static tests.

[vrann]

The exception text should indicate that the request cannot be processed, does not match any route is too coupled with REST

[naydav]

Need to follow Backward Compatibility
ObjectManager::getInstance()->create(...::class)

[naydav]

We need to operate with RequestProcessorInterface
Pool it will be one of implementation of RequestProcessorInterface

Underhood of RequestProcessorInterface we can resolve proper processor by request and call process method

[naydav]

We need to remove this method from interface
This is part of internal implementation but not part of API

[naydav]

Pls use in all places private access for properties and methods
It should not be part of class contract

[naydav]

So, it will be

$this->requestProcessor->process($request); // $this->requestProcessor is RequestProcessorInterface

[naydav]

Method process could return same state, and pool processor will decide next iteration based on this return status
Statuses can be constants in this RequestProcessorInterface

foreach ($this->requestProcessors as $requestProcessor) {
            if (self::PROCESS_STATUS_SUCCESSFUL === $requestProcessor->process($request)) {
                return;
            }
        }

[naydav]

Or more applicable way

if (!isset($this->processors[$request->getPathInfo()])) {
    throw new \Magento\Framework\Exception\NotFoundException(__('Specified request does not match any route.'));
}
$processor = $this->objectmanager->get($this->processors[$request->getPathInfo()]);
$processor->process();

di.xml

// use string but not object for loading of processors by request
<item key="scheme" type="string">SchemeProcessor
<item key="V1" type="string">SyncProcessor
<item key="async" type="string">AsyncProcessor

And my vote is second way

[naydav]

In second case we can avoid sorting

[naydav]

Also implement RequestProcessorInterface
It is only one private case of implementation

[naydav]

Or more applicable way

if (!isset($this->processors[$request->getPathInfo()])) {
    throw new \Magento\Framework\Exception\NotFoundException(__('Specified request does not match any route.'));
}
$processor = $this->objectmanager->get($this->processors[$request->getPathInfo()]);
$processor->process();

di.xml

// use string but not object for loading of processors by request
<item key="scheme" type="string">SchemeProcessor
<item key="V1" type="string">SyncProcessor
<item key="async" type="string">AsyncProcessor

And my vote is second way

[naydav]

In second case we can avoid sorting

[naydav]

Move to di

[naydav]

Move to di

[naydav]

We can remove sorting if we will have mapping between pathInfo and processor class

[vrann]

should this be called V1SynchronousRequestProcessor

[vrann]

can we hide "V1" from the configuration file and have just "sync"

[nuzil]

question about your second comment:
with current implementation we are detecting parts of URL and routing requests automatically.
If we have to add "sync" part here, then we again have to add constants to the Controller to have correct mapping between V1 and Sync processor.

[paliarush]

Current implementation is confusing, because it is not obvious that the processor name will be used as a pattern to match request URLs.

Better to implement canProcess()/match() method on the RequestProcessor interface and in the pool have implementation like:

        foreach ($this->requestProcessors as $requestProcessor) {
            if ($requestProcessor->canProcess($request)) {
                $requestProcessor->process($request);
            }
        }

Then we can use meaningful names for the request processors in di.xml, like schemaRequestProcessor and synchronousRequestProcessor

I know that @naydav requested current implementation. So feel free to keep it as is.

[paliarush]

Good progress overall, thank you for working on this feature.

[paliarush]

Please add meaningful descriptions to all new classes and methods in the CR.

[paliarush]

There must be no @todo merged to the mainline. Please create a ticket instead.

[paliarush]

Such padding is valid for annotations, however we should not use it for the code. The reason is when you add new field, which is longer than all existing ones, padding for all other fields will have to be adjusted. I would recommend not using it for annotations as well, for the same reason.

Please adjust all similar occurrences in the PR.

[paliarush]

Invalid padding, please adjust across current PR.

[paliarush]

Extra asterisk should be removed.

[paliarush]

Please explain the purpose of this change.

[paliarush]

We cannot merge any temporary test skips.

[paliarush]

Cannot merge any TODOs, please create ticket instead.

[paliarush]

Please make all these properties private and remove underscore from the names.

[paliarush]

Please consider removing general.

[nuzil]

Just a small comment to last PR update: we remover Swagger part from it, cause its not ready and needed additional implementation, we will integrate it in another PR

[vrann]

As we discussed in the EE pull request, let's avoid making changed to communication.xsd. Instead we can get rid of the OperationInterface wrapper from the \Magento\WebapiAsync\Model\AsyncBulkPublisher::scheduleAsyncBulk and publish messages with the schema which corresponds to the topic configuration.