magemojo / m2-ce-cron

Magento 2 cron project to fix bugs and common cron issues.
https://magemojo.com
MIT License
165 stars 45 forks source link

Cannot "setup:install" new Magento app with module installed #79

Closed pavelshulga closed 4 years ago

pavelshulga commented 5 years ago

As per the initial report, there is a blocking issue with the Magento installation if magemojo cron module is a part of the project. It happens due to the XML configuration snippet which adds cronExecute item into command list interface' commands list.

The issue is caused by the way the constructor injection chain is handled by Magento. Object Manager is the key component in this flow and is responsible for creating parameters for each object required by the functionality. It resolves all the assets down the dependency chain.

In case of CLI commands, when Magento installation command is triggered, object manager is used to resolve arguments of CommandListInterface which happens to have commands array argument. When resolving all the arguments it tried to instantiate all the items with type object (refer to XML snippet below) `

MageMojo\Cron\Console\Command\CronExecuteCommand
</type>`

As seen above, in cron module we have CronExecute command defined which has Schedule class as contruct parameter. The Schedule class has Magento\Cron\Model\Config class as its dependency. As naming semanticity suggests, the later is a model for configuration entity and implicitly relies on core_config_data table.

The simplest solution in such cases would be to rely on the proxy concept which is a part of out of the box toolset of Magento (both OpenSource and Commerce versions).

Proposed change includes

Reference

pavelshulga commented 5 years ago

While working on proposed changeset I noticed some discrepancies in naming convention of variables across multiple files. As the suggestion we can update those to be aligned with Magento go-to naming approach (ref. Should be in camelCase with lowercase first letter.)

pavelshulga commented 4 years ago

@ericvhileman could you please review the proposed solution? Happy to discuss any of technical decisions

gnuzealot commented 4 years ago

This was fixed in another pull request