The emptyState feature is really nice, but the database query has two issues:
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');
have a query with 2 from or joins, grouping, having, etc.
twice the same query
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)
different queries
It fails on so many instances (Joins, Grouping, several froms, ...)
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(...)
Labels |
Added:
?
|
Labels |
Added:
?
|
Labels |
Added:
J4 Issue
|
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
.
See also #33453 (comment) .
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.
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)
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..
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....
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
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.
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.
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.
Well Im working on it. And coffee. Coffee is more important.
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...
I'm thinking abit different:
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;
}
getIsEmptyState
could be simple like thispublic 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 :(.
(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.
well all these suggestions are not far off what we already have, with just slight changes.
@joomdonation P.S.: If you clear the where clause you also have to clear bind, having and join used in that where clause.
Go for @joomdonation solution (great idea Tuan) + the caching then all issues should be solved.
@PhilETaylor These could help:
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
@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
.
@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)
@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)
@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).
We will, at latest when this state reset is implemented. (Or a third party developer does something...)
Status | New | ⇒ | Closed |
Closed_Date | 0000-00-00 00:00:00 | ⇒ | 2021-05-01 19:34:58 |
Closed_By | ⇒ | bembelimen | |
Labels |
Added:
?
Removed: ? |
Closing as having a PR
As this is closed please remove the release blocker tag
Labels |
Removed:
?
|
We could fix issue #33386 with use of the empty state feature, too, if this issue here is solved.