Skip to content
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

[4.0] Implement condition query for frontend modules/plugins #21702

Closed

Conversation

bembelimen
Copy link
Contributor

@bembelimen bembelimen commented Aug 19, 2018

Summary of Changes

Change hardcoded state calls to the new condition constants

Testing Instructions

  • Create mod_stats or mod_related_items modules and check the numbers
  • Search content in frontend
  • Check pagenav in article view
  • They should work now like in J! 3 again.

@bembelimen bembelimen changed the title Implement condition query for frontend modules/plugins [4.0] Implement condition query for frontend modules/plugins Aug 19, 2018
…frontend-stages

# Conflicts:
#	modules/mod_stats/Helper/StatsHelper.php
@bembelimen
Copy link
Contributor Author

@brianteeman could you please tag this PR, too? Thx.

@brianteeman
Copy link
Contributor

tagged as requested.

…frontend-stages

# Conflicts:
#	modules/mod_stats/Helper/StatsHelper.php
. ' ON ' . $db->quoteName('ws.id') . ' = ' . $db->quoteName('c.state')
)
->where($db->quoteName('ws.condition') . ' = 1');

Copy link
Contributor

Choose a reason for hiding this comment

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

Remove blank line

bembelimen and others added 4 commits September 5, 2018 09:20
…frontend-stages

# Conflicts:
#	modules/mod_stats/Helper/StatsHelper.php
Fix  error c.id not in field list.
@csthomas
Copy link
Contributor

Adding workflow tables to each sql query will slow down joomla 4 on front end.

As #__content.state is a copy of wf.condition why can not we stay with something like
a.state = ContentComponent::CONDITION_PUBLISHED.

IMO workflow staff should be loaded optionally, I mean workflow tables should not be used in the sql WHERE clause. The database should first find and sort the rows, and finally add the workflow columns to the result.

Example using subquery

SELECT old.*, wa.*, ws.*
( SELECT * FROM #__content INNER JOIN #__categories [...] WHERE [...] ORDER BY [...] LIMIT 30) AS old
LEFT JOIN #__workflow_associations wa [...]
LEFT JOIN #__workflow_stages ws [...]

@chmst
Copy link
Contributor

chmst commented Sep 10, 2018

I have tested this item ✅ successfully on d5aaf5f

Tested successfuly for all modules mentioned in testing instructions.


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/21702.

@bembelimen
Copy link
Contributor Author

Hi @csthomas thanks for your feedback.
Yes I'm aware (ofc) that more JOINs make queries slower. Do you see a big impact although we're using keys?
Could you show some benchmarks how big th eimpact is?

The state field of com_content should be viewed as "deprecated", it's only there because of B/C reasons but is not used by the workflow in any case.

@ggppdk
Copy link
Contributor

ggppdk commented Sep 10, 2018

The state field of com_content should be viewed as "deprecated", it's only there because of B/C reasons but is not used by the workflow in any case.

So the workflow tables need to be used when listing / querying the records of components that use workflow ?

Could we not say that it is not there for B/C,
but that it will stay there for performance / simplicity too ? and because workflow is "optional"

@csthomas
Copy link
Contributor

I manage website with 400k articles, 140k of them are published now.

Now on upgrade to 4.0 I will have to create 400k rows in #__workflow_associations. It will be long process...

@csthomas
Copy link
Contributor

So the workflow tables need to be used when listing / querying the records of components that use workflow ?

Joomla goes in this direction. If you want to read article you have to ask "a workflow" for permission:)

@bembelimen
Copy link
Contributor Author

Joomla goes in this direction. If you want to read article you have to ask "a workflow" for permission:)

Not the workflow, but in the future, the stage/state of the article should matter, yes. At the moment everyone can edit an article all the time with edit rights. In the future only people with permission based on the stage will be able to edit. For example if you're responsible for proofreading, you should be able to edit an article when it's unpublished but not when it's published and so on.

So you need this "mapping".

@ggppdk
Copy link
Contributor

ggppdk commented Sep 10, 2018

For example if you're responsible for proofreading, you should be able to edit an article when it's unpublished but not when it's published and so on.

ok,
when editing the record
and / or changing stage of the record
and / or "modifying" the record in some way
and / or modifying "related" data of the record,
then i understand the argument of needing to check workflow

but is workflow required to be checked when viewing the records too ?
later it will effect viewing too ?

@bembelimen
Copy link
Contributor Author

but is workflow required to be checked when viewing the records too ?
later it will effect viewing too ?

I think that was a joke from @csthomas viewing is based on the Access level (view levels) of the article and if it's published. That makes no sense to solve it over the workflow.

@csthomas
Copy link
Contributor

  1. The workflow has several tables, queries to #__workflow_stages or #__workflow_associations for check published state is IMO too much.

    To view the article(-s) we should use the fastest way.

    I would like to replace:
    ->where('ws.condition = ' . (int) ContentComponent::CONDITION_PUBLISHED)
    by:
    ->where('a.state = ' . (int) ContentComponent::CONDITION_PUBLISHED)

    and only left join to #__workflow_stages and #__workflow_associations.

  2. Why we use #__workflow_associations at all?
    There is a many-to-many sql relation:
    #__content -> #__workflow_associations -> #__workflow_stages but one article can have only one workflow_stage.

    Please consider to add workflow_stage_id column to #__content and remove the table #__workflow_associations.

@ggppdk
Copy link
Contributor

ggppdk commented Sep 11, 2018

Workflow feature is very nice,

but it should stay out of "usage" queries as much as possible
not polluting them without a real necessity to do so

at most we should have an API call to get the viewable conditions

->where('a.state IN (' . implode(',' SOMEAPICALL) . ')'

or this

->where('a.state = ' . (int) ContentComponent::CONDITION_PUBLISHED)

About state being a deprecated B/C thing, i would not agree,

workflow should continue set to a condition into the state column in the future, without having the column removed !!

and the workflow tables should not be required at all

  • when viewing,
  • when doing accounting calculation, etc

they should be used only when finding current workflow information to show in management,
when doing transitions, etc

not when using the content
it feels really wrong

@alikon
Copy link
Contributor

alikon commented Sep 11, 2018

the joke was merging the workflow in the 4.x branch to begin with
...looking for a permanent ban...
personal opinion is not allowed in this project

@bembelimen bembelimen closed this Nov 22, 2018
@bembelimen bembelimen deleted the workflow-frontend-stages branch December 30, 2018 23:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants