[4.0][Workflow] Remove com_content defaults from workflow code by HLeithner · Pull Request #21641 · joomla/joomla-cms

HLeithner · 2018-08-16T12:42:15Z

This is a cleanup PR to remove com_content references

Summary of Changes

Remove all 'com_content' constants from workflow code.

Testing Instructions

Create, Delete, Edit

for com_content

articles

for com_workflow

stages
transitions
workflows

Expected result

All works fine

…avior)

…ntent

HLeithner · 2018-08-16T13:10:42Z

This PR is based on #21585

brianteeman · 2018-08-16T13:27:22Z

I thought it looks familiar - so can you close that one please

brianteeman · 2018-08-16T13:28:24Z

administrator/components/com_content/Model/FeaturedModel.php


 defined('_JEXEC') or die;

+use Joomla\Component\Content\Administrator\Extension\ContentComponent;


Please alphas sort

Change in Update in #21585

brianteeman · 2018-08-16T13:35:48Z

libraries/src/Form/Field/WorkflowconditionField.php

+/**
+ * @package     Joomla.Administrator
+ * @subpackage  com_workflow
+ *


please update the @Package @subpackage

Do we sill have this header? because all other Fields doen't have it. Anyway would be part of the other PR.

If you're adding a new file (as the GitHub diff seems to imply here) then the header really should be correct from the beginning.

Change in Update in #21585

brianteeman · 2018-08-16T13:37:02Z

libraries/src/Form/Field/WorkflowconditionField.php

+	protected $type = 'Workflowcondition';
+
+	/**
+	 * The extension where we're


please correct to we are

Change in Update in #21585

laoneo · 2018-08-17T06:26:27Z

administrator/components/com_content/Extension/ContentComponent.php

@@ -163,22 +204,125 @@ public function filterTransitions($transitions, $pk): array
 	 */
 	public function countItems(array $items, string $section)


Will this code be the same for every extension which want's to implement workflow? If yes, please move it either to the CategoriesServiceTrait or the WorkflowServiceTrait. Otherwise every component needs to copy the same code.

This function exists already in several components in joomla it is/was in the helper class (com_fields-> FieldsHelper, com_contact ->ContactHelper, com_newsfeeds -> NewsfeedsHelper) also it was in the com_content helper and has been moved here by the parent PR #21585

All countItems functions are different, countItems() is already implemented in CategoriesServiceTrait, but can't handle workflow conditions at the moment.

If its ok for you I would create a new PR to migrate all existing function to the component container and try to remove redundancy.

Actually just look on com_content (or banners) as the others have not being migrated because of some open pull requests which would lead to conflicts. But they will use the CategoriesTrait as the countItems code is the same on all of them, except the table name. No need to copy things over and over again.

All countItems functions are different

What is the difference? I mean all workflow related extensions will have the same code, except column and row names, or not?

I only skimmed the functions and the looked a bit different that's the reason why I would like to do this a separate PR for all components. I will do it after #21585 is merged.

In the end hopefully all component uses the traits function

When #20693 gets merged then I will do com_contact and com_newsfeeds. com_banners is already done and com_fields doesn't use categories at all.

com_workflow also have countItems but this does something different then the other components.

If com_fields doesn't use why we have the function in
administrator/components/com_fields/helpers/fields.php:560

It is dead code, should be removed as we have now groups.

See #21663.

…ntent # Conflicts: # administrator/components/com_content/Model/FeaturedModel.php # libraries/src/Form/Field/WorkflowconditionField.php # libraries/src/Workflow/WorkflowServiceTrait.php

HLeithner · 2018-08-17T12:41:33Z

So #21585 is merged now this PR much smaller maybe I we get some testers here?

The countItems function rewrite @laoneo mentioned will get it's own PR.

ghost · 2018-08-17T13:14:38Z

I have tested this item 🔴 unsuccessfully on 76535dc

Delete an Stage got Error (see Screenshot), but Stage is trashed.

_{This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/21641.}

HLeithner · 2018-08-17T13:29:00Z

Delete an Stage got Error (see Screenshot), but Stage is trashed.

Not sure if this is related, does this not happen with the dev-4.0 branch?

ghost · 2018-08-17T13:31:31Z

it happen with applied Pull Request.

HLeithner · 2018-08-17T13:51:07Z

This is unrelated seams to be a problem with state->stage renaming please open a new issue.

wilsonge · 2018-08-17T15:52:48Z

libraries/src/Form/Field/TransitionField.php

 	 */
 	protected $extension;

+	protected $workflowStage;


doc block for this please

bembelimen · 2018-08-18T00:16:57Z

Not sure about this PR, the idea was to use com_content as fallback like in categories. If we remove this fallback (which is OK with me), we have to add checks and throw an error if extension is missing.

HLeithner · 2018-08-18T07:51:32Z

There is ni reason to let com content be the fallback, this only makes it more error prone.

I found 3 places where a 3rd party component would fail to use the workflow because com_content was hardcoded or the default fallback.

In my opinion the same should be done for categories.

wilsonge · 2018-08-18T12:33:37Z

I agree with this PR - we should remove com_content as the default but @bembelimen is correct we should probably hard bail if we can't find an extension.

HLeithner · 2018-08-18T14:45:24Z

Sure it should have correct amd useful error messages I will adopt this PR

wilsonge · 2018-08-23T08:50:11Z

administrator/components/com_workflow/Controller/StagesController.php

+	 * @throws  \InvalidArgumentException when no extension or workflow id is set
 	 */
-	public function __construct(array $config = array(), MVCFactoryInterface $factory = null, $app = null, $input = null)
+	public function __construct($config = array(), MVCFactoryInterface $factory = null, $app = null, $input = null)


why remove the array typehint?

phpstorm 1 me 0

wilsonge · 2018-08-23T08:51:05Z

administrator/components/com_workflow/Controller/StagesController.php

 				'index.php?option=' . $this->option . '&view=' . $this->view_list
-				. $extensionURL . '&workflow_id=' . $workflow_id, false
+				. '&extension=' . $this->extension
+				. $extensionURL . '&workflow_id=' . $this->workflowId, false


You just deleted $extensionURL

$extensionURL will be removed

…ntent # Conflicts: # administrator/components/com_workflow/Model/WorkflowModel.php

wilsonge · 2018-09-08T09:58:20Z

LGTM!

HLeithner · 2018-09-08T10:41:43Z

that was a bit too early... I create a new PR fixing 2 things

…a#21641) * Add component specific workflow conditions. * Archived articles should not be visible when not filtered (=> 3.x behavior) * CS * Fixed missing namespace. * CS * Doc fixes and removed archive state from featured published filter. * Make drone happy * Drone... * cs * Postgresql update, add extension parameter to getCondictions() * Fix condition filter * Remove com_content references in workflow * cs * Use quoteName * Fix workflow edit link * Throw an exception if no extension is set in workflow controller. * Fixed type hint and leftover variable.

HLeithner and others added 18 commits August 14, 2018 23:42

Add component specific workflow conditions.

29cddae

Archived articles should not be visible when not filtered (=> 3.x beh…

2eeef28

…avior)

CS

31a2fb2

Fixed missing namespace.

789aef1

CS

353dd28

Doc fixes and removed archive state from featured published filter.

0cb03a6

Make drone happy

ccc8dfd

Merge remote-tracking branch 'upstream/4.0-dev' into workflow-conditions

b4864ec

Drone...

3bcf122

cs

7e82fa4

Merge remote-tracking branch 'upstream/4.0-dev' into workflow-conditions

e4e0117

Postgresql update, add extension parameter to getCondictions()

fc380fc

Fix condition filter

338676b

Remove com_content references in workflow

049f74a

cs

1145e90

Use quoteName

1fb2915

Fix workflow edit link

6ceae04

Merge remote-tracking branch 'upstream/4.0-dev' into workflow-nocomco…

06a5bc4

…ntent

joomla-cms-bot added the PR-4.0-dev label Aug 16, 2018

HLeithner added 2 commits August 16, 2018 15:13

Merge remote-tracking branch 'upstream/4.0-dev' into workflow-conditions

7dd9534

Merge branch 'workflow-conditions' into workflow-nocomcontent

83fea09

brianteeman reviewed Aug 16, 2018

View reviewed changes

laoneo requested changes Aug 17, 2018

View reviewed changes

brianteeman added the New Feature Workflow label Aug 17, 2018

Merge remote-tracking branch 'upstream/4.0-dev' into workflow-nocomco…

76535dc

…ntent # Conflicts: # administrator/components/com_content/Model/FeaturedModel.php # libraries/src/Form/Field/WorkflowconditionField.php # libraries/src/Workflow/WorkflowServiceTrait.php

HLeithner mentioned this pull request Aug 17, 2018

[4.0] [RFC] Optimize countItems and introduce component name #21667

Closed

ghost mentioned this pull request Aug 17, 2018

[4.0] com_workflow: Undefined Method PlgContentJoomla #21668

Closed

wilsonge reviewed Aug 17, 2018

View reviewed changes

wilsonge and others added 2 commits August 22, 2018 16:56

Merge branch '4.0-dev' into workflow-nocomcontent

67985bd

Throw an exception if no extension is set in workflow controller.

19334d7

wilsonge reviewed Aug 23, 2018

View reviewed changes

HLeithner added 2 commits September 8, 2018 10:55

Merge remote-tracking branch 'upstream/4.0-dev' into workflow-nocomco…

00fb941

…ntent # Conflicts: # administrator/components/com_workflow/Model/WorkflowModel.php

Fixed type hint and leftover variable.

4b44582

wilsonge merged commit df09dc0 into joomla:4.0-dev Sep 8, 2018

wilsonge added this to the Joomla 4.0 milestone Sep 8, 2018

HLeithner mentioned this pull request Sep 8, 2018

[4.0] Fix transitions controller missing namespace #22062

Merged

HLeithner deleted the workflow-nocomcontent branch September 8, 2018 13:12


		defined('_JEXEC') or die;

		use Joomla\Component\Content\Administrator\Extension\ContentComponent;

		@@ -163,22 +204,125 @@ public function filterTransitions($transitions, $pk): array
		*/
		public function countItems(array $items, string $section)

Uh oh!

Conversation

HLeithner commented Aug 16, 2018

Summary of Changes

Testing Instructions

Expected result

Uh oh!

HLeithner commented Aug 16, 2018

Uh oh!

brianteeman commented Aug 16, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

HLeithner commented Aug 17, 2018

Uh oh!

ghost commented Aug 17, 2018 • edited by ghost Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

HLeithner commented Aug 17, 2018

Uh oh!

ghost commented Aug 17, 2018

Uh oh!

HLeithner commented Aug 17, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bembelimen commented Aug 18, 2018

Uh oh!

HLeithner commented Aug 18, 2018

Uh oh!

wilsonge commented Aug 18, 2018

Uh oh!

HLeithner commented Aug 18, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wilsonge commented Sep 8, 2018

Uh oh!

HLeithner commented Sep 8, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

ghost commented Aug 17, 2018 •

edited by ghost

Loading