Skip to content

Module separation #145

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

Merged
merged 10 commits into from
Apr 19, 2019
Merged

Module separation #145

merged 10 commits into from
Apr 19, 2019

Conversation

melnikovi
Copy link
Member

@melnikovi melnikovi commented Apr 12, 2019

Problem

Proposal describes different options for module names and namespaces when splitting modules.

Solution

Requested Reviewers

@magento-cicd2
Copy link

CLA assistant check
Thank you for your submission, we really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.

@melnikovi melnikovi added the services isolation Design documents in scope of Services Isolation work label Apr 12, 2019
@nuzil
Copy link
Contributor

nuzil commented Apr 13, 2019

I guess rewriting all old modules in a new manner its a too big change in terms of backward incompatibility. Developers should not just align their modules, they have mostly rewrite all of them.
Also making magento update for all merchant will cost huge amount of time and money.
Maybe a solution here will be to leave old Namespaces and from release to release make vere small movements in a direction of new approach. I know that will take much more time, but will produce less pain.


Common code (business logic used by multiple areas, models, resource modelss)
* CatalogCommon
* CatalogShared
Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure about Common and Shared, it assumes that the code under these namespaces knows about its usage in the different areas. I propose to combine these namespaces into something like Domain.

Copy link
Member Author

Choose a reason for hiding this comment

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

Added to the list.

Cons
* Backwards incompatible, all of the extensions will be affected

3. Keep old namespaces. Classes in CatalogAdminUi, CatalogStorefrontUi, CatalogCron, etc will have Catalog namespace
Copy link
Contributor

Choose a reason for hiding this comment

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

New classes probably will have new namespaces as we don't need to preserve backward compatibility for a new code.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes. Added this for clarity.

* No overhead from preserving keeping backwards compatibility

Cons
* We going to have incorrect namespaces
Copy link

Choose a reason for hiding this comment

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

The namespaces are not incorrect. They simply don't follow the same pattern. The composer autoloader thinks they are perfectly valid.

Copy link
Member Author

Choose a reason for hiding this comment

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

You are right, I will rephrase.

* No overhead from preserving keeping backwards compatibility

Cons
* Backwards incompatible, all of the extensions will be affected

Choose a reason for hiding this comment

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

This should not even be an option. You're talking about completely breaking every extension in the ecosystem overnight; causing every single extn developer to go fork their extn into two versions, one for older magento and one for this; and causing every develop to have to learn a new paradigm. You know the costs of all that across the ecosystem is easily hundreds of thousands of dollars and right?

* CatalogCron _recommended_

CLI (models and resources potentially)
* CatalogCli _recommended_
Copy link
Member Author

Choose a reason for hiding this comment

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

Need to clarify how the CLI will be used.

* CatalogCli _recommended_

WebApi (models and resources potentially)
* CatalogWebApi _recommended_
Copy link
Member Author

Choose a reason for hiding this comment

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

Should we split on SOAP, REST and GraphQL?

Copy link
Contributor

Choose a reason for hiding this comment

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

REST and SOAP have unified implementation, but represent different areas.
GraphQL is completely separate implementation and area.

Copy link
Contributor

Choose a reason for hiding this comment

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

We can't support some things for REST and SOAP at the same time, like associative arrays (SOAP doesn't allow to use them). Probably it makes sense to separate REST and SOAP.

* CatalogWebApi _recommended_

Common code (business logic used by multiple areas, models, resource modelss)
* CatalogCommon
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 having this module named Catalog and have metapackage named CatalogMetapackage (or something similar).

The issue is that third-party modules will not be able to get seamless upgrade if they depend on current Catalog. We need to evaluate how realistic is that we will not bump major version of the Catalog package with this module split.

Copy link
Contributor

Choose a reason for hiding this comment

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

The idea was to don't introduce a new name and 3rd party developers will have a possibility to specify multiple versions in composer (option 3 from https://github.com/magento/architecture/blob/master/design-documents/service-isolation/split-extensions.md#new-module---old-composer)

The following is proposed naming for modules:
Admin Ui and admin related business logic (blocks, controllers, static resources, models and resources potentially)
* CatalogAdmin
* CatalogAdminUi _recommended_
Copy link
Member Author

Choose a reason for hiding this comment

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

Should we drop Ui postfix? We can have a case when resource model is only relevant only in storefront, as it's not UI it can be confusing.

Copy link
Member Author

Choose a reason for hiding this comment

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

Decided to leave Ui postfix.

@melnikovi melnikovi changed the title Module split Module separation Apr 19, 2019
@buskamuza buskamuza merged commit 28e78d8 into magento:master Apr 19, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
services isolation Design documents in scope of Services Isolation work
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants