joomla / joomla-cms

Home of the Joomla! Content Management System
https://www.joomla.org
GNU General Public License v2.0
4.77k stars 3.65k forks source link

The Workflow can't be used from Console Commands #40653

Closed anibalsanchez closed 5 days ago

anibalsanchez commented 1 year ago

Steps to reproduce the issue

When a Console Command tries to insert an article, the Workflow throws an error.

TypeError: Joomla\CMS\Workflow\Workflow::__construct(): Argument #2 ($app) must be of type ?Joomla\CMS\Application\CMSApplication, Joomla\CMS\Application\ConsoleApplication given, called in /app/www/libraries/src/MVC/Model/WorkflowBehaviorTrait.php on line 90

The Joomla\CMS\Workflow \Workflow assumes it will receive a CMSApplication (child of WebApplication). When called from a Console Command, the application is a Joomla\CMS\Application\ConsoleApplication. So, the error is thrown.

Expected result

Console Commands must be able to use the Joomla Workflow

Actual result

TypeError: Joomla\CMS\Workflow\Workflow::__construct(): Argument #2 ($app) must be of type ?Joomla\CMS\Application\CMSApplication, Joomla\CMS\Application\ConsoleApplication given, called in /app/www/libraries/src/MVC/Model/WorkflowBehaviorTrait.php on line 90

System information (as much as possible)

Joomla 4.3.1

Additional comments

alikon commented 1 year ago

can yuo please post your console command script for testing ?

anibalsanchez commented 1 year ago

The console command is part of a system; I can't publish it here.

You can reproduce the issue in any console command by instantiating an article model:

        $app = \Joomla\CMS\Factory::getApplication();
        $mvcFactory = $app->bootComponent('com_content')->getMVCFactory();
        $articleModel = $mvcFactory->createModel('Article', 'Administrator', ['ignore_request' => true]);

When it creates the ArticleModel, it executes the $this->setUpWorkflow('com_content.article'); and $this->workflow = new Workflow($extension, Factory::getApplication(), $db); (libraries/src/MVC/Model/WorkflowBehaviorTrait.php, line 90)

alejoasotelo commented 1 year ago

I have the same problem with my console plugin to bulk import articles to joomla. Any workaround to create articles from console plugin?

anibalsanchez commented 1 year ago

The restrictive argument type in the Workflow constructor causes the error when it receives the whole App. The only possible workaround is patching the Workflow constructor to change the argument type.

Septdir commented 10 months ago

Same problem. I think that, in addition to correcting the argument type, we also need to remove class initialization if Workflow is disabled.

There is no need to connect something that will not be used.

Septdir commented 10 months ago
if (!Factory::getApplication()->isClient('cli')) {
            $this->setUpWorkflow('com_content.article');
        }
brianteeman commented 10 months ago

The way that workflow is coded is such that even when it is disabled records are created in the workflow tables for the article status. This is so that you can enable workflows at a later date if you wish.

Septdir commented 10 months ago

The way that workflow is coded is such that even when it is disabled records are created in the workflow tables for the article status. This is so that you can enable workflows at a later date if you wish.

I saw this and it is also very bad. The more Joins, the more unstable the indexes.

As a result, disabling Workflow does not actually disable it.

brianteeman commented 10 months ago

not saying its good or bad - just saying what it is

HermanPeeren commented 10 months ago

My observations:

  1. Workflow-parameters like CMSApplication and DatabaseDriver should be typed against an interface.
  2. general idea was that different Joomla applications (CLI, frontend, backend, API or other) all implement Joomla\Application\CMSApplicationInterface, so for instance an ArticleModel could then also be used in a CLIApplication. However, in the current implementation, some models use specific methods of the application not part of CMSApplicationInterface, thereby violating Liskov Substitution (for example $app->getParams(), $app->getUserState() or $app->getUserStateFromRequest()) . The workflow is not the only spot where using a frontend or backend Model fails in a CLIApplication.
  3. the workflow is a dependency of the extension that uses it. Dependencies should be injected via the Dependency Injection Container and specified in a service provider. This was probably the plan for the Workflow-service too, but not yet realised.

My preliminary conclusions:

  1. Workflow parameters should be typed as \Joomla\Application\CMSApplicationInterface and \Joomla\Database\DatabaseInterface
  2. we should go through the code of the CMS to check if we keep to our contracts, that is: when we type against an interface, then methods of more specific ("covariant") types must not be executed without a check. Especially in our case: all application-methods which are not part of CMSApplicationInterface must only be called if they belong to the specific type of the current application.
  3. the Workflow-service should be injected with dependency injection.

Please correct me if I'm wrong or if you have other obeservations or conclusions.

anibalsanchez commented 10 months ago

@HermanPeeren To fix the issue, your proposal is the right approach.

Looking into a long-term solution, the problem is that the hexagonal CMS architecture needs to be more widely adopted in the core development practice.

At any time, a developer can create a feature or a bug fix that does not follow the separation between the layers. Similar issues will appear repeatedly if the practice is not enforced (automatically or manually).

leeroy1821 commented 6 days ago

@HermanPeeren is right because \Joomla\CMS\Workflow\Workflow only use $app for bootComponent. But bootComponent is method define in CMSApplicationInterface. You not need CMSApplication.

I checkt with this change. It works.

Why leave Joomla broken 1+ year if we know fix? I do not understand why wait.

Quy commented 6 days ago

@leeroy1821 Please submit a pull request. Thanks.

leeroy1821 commented 5 days ago

@Quy I made PR: https://github.com/joomla/joomla-cms/pull/44397 Thank you!

richard67 commented 5 days ago

Closing as having a pull request. Please test #44397 . Thanks in advance.