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

Define dependency of Magento_CatalogUrlRewrite to Magento_ImportExport as soft dependency #21125

Open
wants to merge 1 commit into
base: 2.3-develop
from

Conversation

Projects
None yet
3 participants
@avstudnitz
Copy link
Contributor

avstudnitz commented Feb 11, 2019

Description (*)

When trying to remove all *ImportExport modules, the only unexpected dependencies were those from the Magento_CatalogUrlRewrite to Magento_CatalogImportExport and Magento_ImportExport:

{
    "name": "magento/module-catalog-url-rewrite",
    "description": "N/A",
    "config": {
        "sort-packages": true
    },
    "require": {
        "php": "~7.1.3||~7.2.0",
        "magento/framework": "*",
        "magento/module-backend": "*",
        "magento/module-catalog": "*",
        "magento/module-catalog-import-export": "*",
        "magento/module-eav": "*",
        "magento/module-import-export": "*",
        "magento/module-store": "*",
        "magento/module-ui": "*",
        "magento/module-url-rewrite": "*"
    },
    [...]
}

The Magento_CatalogUrlRewrite module contains two observers (\Magento\CatalogUrlRewrite\Observer\AfterImportDataObserver, \Magento\CatalogUrlRewrite\Observer\ClearProductUrlsObserver) which observe events from the Magento_CatalogImportExport module (catalog_product_import_bunch_save_after, catalog_product_import_bunch_delete_after) and contain references to ImportExport modules (constants and return types only, no classes instantiated via DI).
If Magento_CatalogImportExport is disabled or removed, these two observers aren't instantiated or used otherwise. This means, Magento_CatalogUrlRewrite will work perfectly if the ImportExport modules are not available. In my opinion it's a soft dependency ("A module with a soft dependency on another module can function properly without the other module, even if they have a dependency upon it.", see https://devdocs.magento.com/guides/v2.3/architecture/archi_perspectives/components/modules/mod_depend_types.html). Thus, I suggest to not require the classes in question but just suggest them so we can use Magento_CatalogUrlRewrite without Magento_CatalogImportExport and Magento_ImportExport.

Manual testing scenarios (*)

  1. Remove the ImportExport modules, i.e. by adding the following section to your composer.json:
    "replace": {
        "magento/module-advanced-pricing-import-export": "*",
        "magento/module-bundle-import-export": "*",
        "magento/module-catalog-import-export": "*",
        "magento/module-configurable-import-export": "*",
        "magento/module-customer-import-export": "*",
        "magento/module-downloadable-import-export": "*",
        "magento/module-grouped-import-export": "*",
        "magento/module-import-export": "*",
        "magento/module-tax-import-export": "*"
    },
  1. Check that the CatalogUrlWrite still works by creating a product in the Magento Admin and checking that it is available in the frontend with a "speaking" URL. Do the same with a category.

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)
@magento-engcom-team

This comment has been minimized.

Copy link
Contributor

magento-engcom-team commented Feb 11, 2019

Hi @avstudnitz. 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 2.3-develop instance - deploy vanilla Magento instance

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

@avstudnitz

This comment has been minimized.

Copy link
Contributor Author

avstudnitz commented Feb 13, 2019

I see there is a test failing which checks for dependency declarations.
Should we have a new module Magento_CatalogImportExportUrlRewrite which has dependencies to magento/module-catalog-url-rewrite, magento/module-catalog-import-export and magento/module-import-export and contains the two observers instead? I think that would be the clearest way to do it if the above suggested way to just undeclare the hard dependencies doesn't work out.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment