J4 Issue ?
avatar bembelimen
bembelimen
1 May 2021

The emptyState feature is really nice, but the database query has two issues:

Steps to reproduce the issue

First issue:

Go to a model and add the following code:

	public function getTest()
	{
		echo $this->_getListQuery()->dump();

		$this->getIsEmptyState();

		echo $this->_getListQuery()->dump();
	}

Now call in your view:
$this->get('Test');

2nd issue:

have a query with 2 from or joins, grouping, having, etc.

Expected result

First issue

twice the same query

Second issue

The method is generic enough to work all the time (Imho clearing the query is per se wrong, it should build up the query...but that's my opinion)

Actual result

First issue

different queries

Second issue

It fails on so many instances (Joins, Grouping, several froms, ...)

Additional comments

The emptyState check changes the original query instead of making a clone (which is still broken) or do a normal new query with the correct syntax. => also check _getListCount(...)

avatar bembelimen bembelimen - open - 1 May 2021
avatar joomla-cms-bot joomla-cms-bot - change - 1 May 2021
Labels Added: ?
avatar joomla-cms-bot joomla-cms-bot - labeled - 1 May 2021
avatar richard67 richard67 - change - 1 May 2021
Labels Added: ?
avatar richard67 richard67 - labeled - 1 May 2021
avatar chmst chmst - change - 1 May 2021
Labels Added: J4 Issue
avatar chmst chmst - labeled - 1 May 2021
avatar richard67
richard67 - comment - 1 May 2021

We could fix issue #33386 with use of the empty state feature, too, if this issue here is solved.

avatar PhilETaylor
PhilETaylor - comment - 1 May 2021

Not really a release blocker - as you can simply add "return false" at the top of the isEmptyState function and be done with it and release Joomla 4 without this feature. The feature is only a smoke screen, all previous behaviour is still there.

Everyone seems to have their opinion on what should be in that function, but I have not seen any real concrete examples from people yet.

getIsEmptyState() method is written to check the empty state of the last SQL run. (Ie removing any limits from the SQL and counting the number of rows). It is not written to check if a model is an empty state, because Joomla abuses models with huge queries with multiple joins. As written, the query in getIsEmptyState() method does exactly what it was designed to do, that is, to take the last query run, clean it up, and to run it again to return a count of rows.

Used like that, the method is working well.

Used any other way, with false assumptions, like the one you are reporting in this issue, will result in different behaviour.

Issue 1 - if you are going to misuse functions then its up to you to understand the knock on effect. _getListQuery is a protected method for a reason and getIsEmptyState() method was written to directly manipulate the LAST query before it was run.

Issue 2 - It does not "fail on so many instances". All reported "instances" have already been fixed within hours of being reported.

_getListCount literally does something similar to the code in getIsEmptyState.

avatar richard67
richard67 - comment - 1 May 2021

See also #33453 (comment) .

avatar PhilETaylor
PhilETaylor - comment - 1 May 2021

However, since it is a Public method, we could not assure that. For a public method like that, developer could call it anytime they want and not depend on the calling of other methods.

Which again comes down to the DEVELOPER not understanding that getIsEmptyState is working on the LAST QUERY RUN.

There simply is no quick an easy way to say "Hey Im on the Articles page, are there no articles in the db?" in a GENERIC AND REUSEABLE way.

I can see us now having to pull up the getIsEmptyState and have to write a different method for each and every component just to satisfy the "new" requirements developers now have of this feature. (Same as we have done for Categories and tags where getIsEmptyState is in the Model for the component.... hardly a nicer solution.

avatar joomdonation
joomdonation - comment - 1 May 2021

Which again comes down to the DEVELOPER not understanding that getIsEmptyState is working on the LAST QUERY RUN.

That mean we don't understand how to write proper public API method. As mentioned, developers should be allowed to call a public method anytime they want. A public method call should not depend on other method call.

We will have to find a way to solve this issue properly (I mean we, not you work on it alone)

avatar PhilETaylor
PhilETaylor - comment - 1 May 2021

So you want to disconnect getIsEmptyState from the previous SQL run, that's cool - just means we now need to hand craft a SQL for each and every component..

avatar bembelimen
bembelimen - comment - 1 May 2021

You know, we're all in favour of the emptyState feature, we just want to get it stabilized. So I'm not sure why you're against looking for a nice solution which makes everyone happy but slashing out against that "stupid developer" (yeah, my words, not yours).

_getListCount works on a copy of the original query, so it's not the same, but can be fixed very easy.
This would also fix the issue @joomdonation mentioned.
The clear-approach is, in my opinion, the wrong one because there are too many things to consider. So perhaps by discussing we find a better solution....if not then not...

So we're all karma here....

avatar joomdonation
joomdonation - comment - 1 May 2021

So you want to disconnect getIsEmptyState from the previous SQL run, that's cool - just means we now need to hand craft a SQL for each and every component..

I did not say that. At least we should borrow some ideas from getTotal method and continue from there

avatar PhilETaylor
PhilETaylor - comment - 1 May 2021

Im not against anything, but equally I have not seen any concrete examples from anyone that take every scenario I have already had to play through over several weeks to get this where it is today - which is merged and working.

avatar PhilETaylor
PhilETaylor - comment - 1 May 2021

At least we should borrow some ideas from getTotal method and continue from there

Dont you think I tried that. It doesn't work because again it relies on the list query - and therefore if you select a Tag and you get no results you get empty state, when the table is not in an empty state.

avatar joomdonation
joomdonation - comment - 1 May 2021

Dont you think I tried that. It doesn't work because again it relies on the list query - and therefore if you select a Tag and you get no results you get empty state, when the table is not in an empty state.

We must find a way. If not today, then tomorrow. We still have time.

avatar PhilETaylor
PhilETaylor - comment - 1 May 2021

Well Im working on it. And coffee. Coffee is more important.

avatar bembelimen
bembelimen - comment - 1 May 2021

Just an untested idea:

public function getIsEmptyState()
{
	// Get a storage key.
	$store = $this->getStoreId('getIsEmptyState');

	// Try to load the data from internal storage.
	if (isset($this->cache[$store]))
	{
		return $this->cache[$store];
	}

	$query = $this->_getListQuery();

	$query = clone $query;

	if ($query instanceof DatabaseQuery)
	{
		// We allow 3rd party developer to set a specific table to avoid conflicts on multiple "FROM"
		if (isset($this->emptyStateTable))
		{
			$query->clear('from')
				->from($query->quoteName($this->emptyStateTable));
		}

		$query->clear('where')
			->clear('join')
			->clear('having')
			->clear('bounded');
	}

	$this->cache[$store] = $this->_getListCount($query) === 0;

	return $this->cache[$store];
}

What's missing: additional "where" like in the categories view when the extension parameter is needed. Probably we could add an helper method like:

/**
* Can be used by the child class to add getIsEmptyState conditions
* */
protected function getTableStateCondition()
{
	return false;
}

And the code above can be extended in something like:

	$conditions = $this->getTableStateCondition();

	if (is_array($conditions))
	{
		foreach ($conditions as $condition)
		{
			$query->where($condition);
		}
	}

Or having a prepare method which can be extended by the child class...

avatar joomdonation
joomdonation - comment - 1 May 2021

I'm thinking abit different:

  1. Add a new method getEmptyStateQuery method to allow building basic query for empty state
       protected function getEmptyStateQuery()
	{
		$query = clone $this->_getListQuery();

		if ($query instanceof DatabaseQuery)
		{
			$query->clear('where')
				->clear('bounded')
                               ->clear('join')
                              ->clear('having');
		}

		return $query;
	}
  1. Then the getIsEmptyState could be simple like this
public function getIsEmptyState()
{
	return (int) $this->_getListCount($this->getEmptyStateQuery()) === 0;
}

The idea of having getEmptyStateQuery method is that it allows child model class to extend it to add more conditions like this if needed https://github.com/joomla/joomla-cms/blob/4.0-dev/administrator/components/com_categories/src/Model/CategoriesModel.php#L512

       protected function getEmptyStateQuery()
	{
		$query = parent::getEmptyStateQuery();

               $query->where($this->_db->quoteName('extension') . ' = :extension')
			->bind(':extension', $extension);

                return $query;
	}

I'm unsure if that would work. Will have to try and see (I still don't know what ->clear('bounded') is doing :(.

avatar richard67
richard67 - comment - 1 May 2021

(I still don't know what ->clear('bounded') is doing :(.

@joomdonation I assume it clears the "bind" part of the query, like ->clear('join') clears the "join" part and so on.

avatar PhilETaylor
PhilETaylor - comment - 1 May 2021

well all these suggestions are not far off what we already have, with just slight changes.

avatar richard67
richard67 - comment - 1 May 2021

@joomdonation P.S.: If you clear the where clause you also have to clear bind, having and join used in that where clause.

avatar bembelimen
bembelimen - comment - 1 May 2021

Go for @joomdonation solution (great idea Tuan) + the caching then all issues should be solved.

avatar joomdonation
joomdonation - comment - 1 May 2021

@PhilETaylor These could help:

  • Allow calling the method in any order
  • Don't change $this->query directly, clone it.
  • Abit easier to extend/change the query to meet the need of child model class.

And if you look at _getListCount method, you will see that it also deal with other cases. We cannot always use SELECT COUNT(*), there are cases which we have to query and count total number of returned records https://github.com/joomla/joomla-cms/blob/4.0-dev/libraries/src/MVC/Model/BaseDatabaseModel.php#L182-L191

avatar PhilETaylor
PhilETaylor - comment - 1 May 2021
  • we have to find a way to clear the filter.* from the state for @richard67 ... would would have through that was so difficult but Joomla has lots of get/set but no clear methods...
avatar joomdonation
joomdonation - comment - 1 May 2021

@bembelimen I'm unsure if we have to cache it. The emptyState does not depend on model states, right? So I don't think we have to cache it like getItems and getTotal.

avatar bembelimen
bembelimen - comment - 1 May 2021

@PhilETaylor I don't think you have to solve this. With the above improvement there we then can go the next step (somewhere in the model):

if (!$this->getEmptyStateQuery())
{
    // Apply filter
}

@joomdonation true, but still it should be cached as calling it twice gives the same result (like in the state improvement)

avatar joomdonation
joomdonation - comment - 1 May 2021

@richard67 Thanks. I haven't used prepared statement, so I don't know about ->clear('bounded') yet. Will have to look at it when have time (there are many changes in Joomla 4 which I still looked at yet)

avatar joomdonation
joomdonation - comment - 1 May 2021

@bembelimen I don't think we even call that method twice, so cache it is not really needed. Even if we cache it, use a property of the class should be enough (not like cache getItems and getTotal).

avatar bembelimen
bembelimen - comment - 1 May 2021

We will, at latest when this state reset is implemented. (Or a third party developer does something...)

avatar PhilETaylor
PhilETaylor - comment - 1 May 2021

please test [4] redo the logic behind Empty State Queries. #33471

avatar bembelimen bembelimen - change - 1 May 2021
Status New Closed
Closed_Date 0000-00-00 00:00:00 2021-05-01 19:34:58
Closed_By bembelimen
Labels Added: ?
Removed: ?
avatar bembelimen bembelimen - close - 1 May 2021
avatar bembelimen
bembelimen - comment - 1 May 2021

Closing as having a PR

avatar brianteeman
brianteeman - comment - 11 May 2021

As this is closed please remove the release blocker tag

avatar richard67 richard67 - change - 11 May 2021
Labels Removed: ?
avatar richard67 richard67 - unlabeled - 11 May 2021

Add a Comment

Login with GitHub to post a comment