Skip to content
This repository was archived by the owner on Nov 19, 2024. It is now read-only.

Technical guidelines: Clarify what to do when service contracts already exist in a module not suffixed with "Api" #3524

Merged
merged 8 commits into from
Apr 10, 2019
Merged

Technical guidelines: Clarify what to do when service contracts already exist in a module not suffixed with "Api" #3524

merged 8 commits into from
Apr 10, 2019

Conversation

nathanjosiah
Copy link
Contributor

@nathanjosiah nathanjosiah commented Jan 7, 2019

This PR is a:

  • New topic
  • Content update
  • Content fix or rewrite
  • Bug fix or improvement

Summary

Re: https://devdocs.magento.com/guides/v2.3/coding-standards/technical-guidelines.html#64-service-contracts-application-layer

For the new service contract doc requirements, what should be done when new interfaces need to be added to a module that already has the contracts in the main module. For example, I need to add contracts to support MyModule but MyModule/Api already exists. Should they be added to MyModule/Api? or should the contract locations be fragmented and create the new module MyModuleApi/Api/ with the new contracts and leave the MyModule/Api contracts as-is?

More information regarding suggested change

My suggestion is to keep the contracts in the module rather than making a new module and deprecating the old contracts. This will prevent 3rd party devs from deprecating code and it also will simplify the development process for module that already have contracts. These modules can be migrated to the new form at a later time with specific direction when it makes sense in the project lifecycles (i.e. a major or minor release).

whatsnew
Added clarification about the location of Service Contracts in section 6.4.1.1 of the Technical Guidelines

@magento-cicd2
Copy link
Contributor

magento-cicd2 commented Jan 7, 2019

CLA assistant check
All committers have signed the CLA.

@magento-engcom-team
Copy link

@nathanjosiah thank you for contributing. Please accept Community Contributors team invitation here to gain extended permissions for this repository.

@magento-cicd2
Copy link
Contributor

An admin must run tests on this PR before it can be merged.

@keharper keharper self-assigned this Jan 7, 2019
@keharper keharper added 2.x Internal Dev Differentiates work between community and Magento staff Technical Updates to the code or processes that alter the technical content of the doc labels Jan 7, 2019
@navarr
Copy link
Member

navarr commented Jan 7, 2019

Doesn't this burden eventually moving existing contracts into an Api module?

Wouldn't it be better to @deprecate the old ones, place them in a new service contract module, have the new interfaces extend the old interfaces, and give plenty of time for implementations to switch?

@keharper
Copy link
Contributor

@buskamuza Is any additional architect review required?

@keharper
Copy link
Contributor

@buskamuza Can this be merged?

@buskamuza
Copy link
Contributor

Please don't merge yet.

@keharper
Copy link
Contributor

@buskamuza You closed magento/architecture#75 last week. Can this PR be merged?

@@ -536,7 +536,7 @@ class View extends Template

6.4.1.1. Service contract interfaces SHOULD be placed in separate API modules. The other modules will depend on the API module, and implementations could be easily swapped via `di.xml`. API module namess must end with the `Api` suffix. For example, if a module is named `MyModule`, its APIs SHOULD be declared in a module named `MyModuleApi`.

6.4.1.2. Service interfaces that should be exposed as web APIs MUST be placed under the `MyModuleApi/Api` directory. Service data interfaces MUST be placed under `MyModuleApi/Api/Data`. Directories under `MyModuleApi/Api` SHOULD NOT be nested.
6.4.1.2. Service interfaces that should be exposed as web APIs MUST be placed under the `MyModuleApi/Api` directory. Service data interfaces MUST be placed under `MyModuleApi/Api/Data`. Directories under `MyModuleApi/Api` SHOULD NOT be nested. If contracts need to be added to support a module that already contains classes under `Api/` (e.g. `MyModule/Api`), the contracts SHOULD be added to the existing module.
Copy link
Contributor

Choose a reason for hiding this comment

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

I propose to add a note to 6.4.1.1. instead as that is the item describing the rules on a module level. So I'd rather rephrase that one as "6.4.1.1. Service contract interfaces SHOULD be placed in separate API modules , except when existing module already contains Service Contracts in Api folder. The other modules will depend on the API module, and implementations could be easily swapped via di.xml. API module namess must end with the Api suffix. For example, if a module is named MyModule, its APIs SHOULD be declared in a module named MyModuleApi."

An important point here is that Api folder should contain Service Contracts (probably that's the only expected, but who knows).

@buskamuza
Copy link
Contributor

@keharper , we close tickets just to indicate that it's a past meeting.

@keharper keharper added Major Update Significant original updates to existing content and removed Technical Updates to the code or processes that alter the technical content of the doc labels Apr 10, 2019
@keharper
Copy link
Contributor

running tests

@keharper
Copy link
Contributor

running tests

@keharper keharper merged commit 96e3cc8 into magento:master Apr 10, 2019
@ghost
Copy link

ghost commented Apr 10, 2019

Hi @nathanjosiah, 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 subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
2.x Internal Dev Differentiates work between community and Magento staff Major Update Significant original updates to existing content
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants