That change has been made with PR #33414 . @PhilETaylor Any thoughts?

All my thoughts are in the original PR #33414 and the reasons behind it, which were open to peer review, tested by two humans, and merged by a release leader - now the president of Open Source Matters...

If an item doesnt exist, then returning an empty object is simply wrong anyway IMHO.
You cannot checkout a non-existing item.
You cannot load a non existing item into a form for edit. Its simply wrong.

All my thoughts are in the original PR #33414 and the reasons behind it, which were open to peer review, tested by two humans, and merged by a release leader - now the president of Open Source Matters...

If an item doesnt exist, then returning an empty object is simply wrong anyway IMHO. You cannot checkout a non-existing item. You cannot load a non existing item into a form for edit. Its simply wrong.

Makes all sense to me.

Also shows how crap the unit tests are if it never picked up such a "huge backwards compatibility change" LMAO.

The first problem here is the backwards compatibility change.

This:
if ($item = parent::getItem($pk)) {

returns TRUE in J3, J4 Beta

but FALSE in J4 RC, J4 Stable

when there is no item, which is the complete opposite

Something like change

FROM:
if ($i > 0)
TO:
if ($i < 0)

The second problem here is to differentiate between error and empty result, which are completely different outcomes.

If you are trying to get an item - and the item doesnt exist, then returning an empty item IS SIMPLY WRONG! In a perfect world that would raise an exception, but in the imperfect Joomla this is now returning false.

if ($item = parent::getItem(6)) {

if item with pk 6 doesnt exist then $item should NOT be an empty object or true - that is just plain WRONG.

If this is a Backward compatibility issue it could be added to https://docs.joomla.org/Potential_backward_compatibility_issues_in_Joomla_4

I have googled and failed to find an official Backward Compatibility Statement from the Joomla project. However with this change now in Joomla 4.0.1/23 versions I do not see it changing back now.

Plus is conceptually correct.

I have not fired up my IDE to check, but like I said, the PR only changed the errors captured, not the return from the methods, that was untouched.

This post is not trying to say what is right or wrong, but that between Beta and RC we can't change the answer of the method to the complete opposite.

The reason for the distinction between an error and an empty result in earlier versions was probably because the empty result defined an object with table columns to work on.

The false has been there for TWELVE YEARS!

In previous version, the FALSE was returned only when there was an error, not when there were no items:

// Check for a table object error.
if ($return === false && $table->getError())
{
    $this->setError($table->getError());

    return false;
}

These are completely two different FALSE statements

J4 BETA

// Check for a table object error.
if ($return === false && $table->getError())
{
    $this->setError($table->getError());

    return false;
}

• FALSE is returned only when there is error.

vs:

J4 RC

// Check for a table object error.
if ($return === false)
{
    // If there was no underlying error, then the false means there simply was not a row in the db for this $pk.
    if (!$table->getError())
    {
        $this->setError(Text::_('JLIB_APPLICATION_ERROR_NOT_EXIST'));
    }
    else
    {
        $this->setError($table->getError());
    }

    return false;
}

FALSE is returned always - when there is error or when there are no items.

In previous version, the FALSE was returned only when there was an error, not when there were no items:

IMHO If you are calling a function called getItem and that function cannot get the item you are asking for from the database then THAT IS AN ERROR, because you were expecting that item to be in the database and its not!

FALSE is returned always - when there is error or when there are no items.

That is exactly what I would expect. I asked for an item, and the item doesnt exist so its an error.

Again, I am not saying here what is right or not.

Regardless of what’s right and not: If we change it back we again have a b/c break, but this time in stable phase.

That is exactly what I would expect. I asked for an item, and the item does not exist so its an error.

This is completely wrong. Empty result is not an error. If you ask for items but system tells you, there are no items, it means, it says, there are 0 items but this is not an error.

Example: How many items are there?

• 10 ... Success (No error)
• 0 ... Success (No error)
• Error (some error happened during asking the database e.g.) ... Error

This is completely wrong. Empty result is not an error. If you ask for items but system tells you, there are no items, it means, it says, there are 0 items but this is not an error.

Then we disagree - WHEN TRYING TO LOAD A SINGLE ITEM using getItem

it means, it says, there are 0 items but this is not an error.

If you have asked for an item with a $pk of an integer, and the db doesnt contain that row with that primary key - that should never return an empty object in my opinion.

How many items are there?

This is not about items plural - the method changed in my PR is getItem (singular) and checkout. Other methods should be unchanged by my PR. Therefore I have no clue what on earth you are on about with asking items (plural) as this method is for loading a SINGLE ITEM from the db checking out A SINGLE ITEM.

If you have issues with " ask for items but system tells you, there are no items," then that is unrelated to the PR quoted.

Please explain how on earth you expect the methods changed in my PR to tell you if there are 10 or 0 items?!??! Im so confused but you seem adamant that this broke, when the methods changed have nothing to do with plural items.

Please explain how on earth you expect the methods changed in my PR to tell you if there are 10 or 0 items?!??! Im so confused but you seem adamant that this broke, when the methods changed have nothing to do with plural items.

This was an example to explain that returning information about not items is not an error. General example to explain what is difference between error and returning empty results.

Im done. This is nothing to do with my PR.

If this has nothing to do with your PR, the question is who changed

FROM:

// Check for a table object error.
if ($return === false && $table->getError())
{
    $this->setError($table->getError());

    return false;
}

TO:

// Check for a table object error.
if ($return === false)
{
    // If there was no underlying error, then the false means there simply was not a row in the db for this $pk.
    if (!$table->getError())
    {
        $this->setError(Text::_('JLIB_APPLICATION_ERROR_NOT_EXIST'));
    }
    else
    {
        $this->setError($table->getError());
    }

    return false;
}

which is huge backwards compatibility change, because it changes the behavior of the function completely.

For those who are not familiar with PHP:

• in first code, the FALSE is only returned when there is an error (empty results returns TRUE)
• in second code, the FALSE is returned even there is no error (empty results returns FALSE)

This is nothing to do with my PR.

All your talk about items (plural) and counting of zero or 10 items is nothing to do with the PR or the changes made - its commentary at best..

**IF AN ITEM CANNOT BE LOADED FROM THE DATABASE WHEN YOU ARE ASKING FOR IT SPECIFICALLY TO BE LOADED FROM THE DATABASE - THAT SHOULD BE AN ERROR/EXCEPTION - THAT SHOULD NEVER RETURN AN EMPTY OBJECT !!!!! THERE IS NO SUCH THING AS AN EMPTY RESULT IF YOU ARE SPECIFICALLY ASKING A METHOD TO RETURN A ROW, AND THAT ROW DOESNT EXIST - that is an ERROR!! **

Revert "[4] When item doesn't exist, show specific message" #35813

If you ask for items but system tells you, there are no items, it means, it says, there are 0 items but this is not an error.

Correct. But we are not talking about ITEMS (PLURAL) here. We are talking about a method called getItem() a method that is used to load an EXISTING ROW from the database.

What you are saying is that if you TRY to load an EXISTING row from the database, and that row doesnt exist, you want it to return an empty object and not error. THAT IS SIMPLY LOGICALLY WRONG regardless if thats what Joomla used to do.

Phil, I'm not here to tell you what's right or wrong. My example with items has nothing to do with your PR, you are right, I just wanted to show that an error and no result are two different things.

Also, I'm not a core developer where this feature has worked for many years, so it's hard to explain why it split between an error and a empty result.

I'm just saying that it's very bad that in Beta this function returned TRUE with empty result and in RC it started to return FALSE under the same conditions.

I'm just saying that it's very bad that in Beta this function returned TRUE with empty result and in RC it started to return FALSE under the same conditions.

In the absence of any official Joomla backward compatibility statement, relying on SEMVER alone, there is nothing wrong with a change of this (accidental) "magnitude" between a beta and a RC - its not ideal. but its not against the b/c rules.

At best it should be documented here https://docs.joomla.org/Potential_backward_compatibility_issues_in_Joomla_4

At worse it can be reverted - the PR is now waiting testing. #35813

BTW I don't know why it worked that way, maybe because if no result was found, then there was an empty object that got filled with data, etc.

In the absence of any official Joomla backward compatibility statement, relying on SEMVER alone, there is nothing wrong with a change of this (accidental) "magnitude" between a beta and a RC - its not ideal. but its not against the b/c rules.

It is not only change between Beta and RC but between many many years used in J3 till J4 Beta and J4 RC.

It is not only change between Beta and RC but between many many years used in J3 till J4 Beta and J4 Stable.

Then the change was (accidentally) made at the correct time, that is, before Joomla 4.0.0 was released just like all the other Potential backward compatibility issues in Joomla 4 listed in https://docs.joomla.org/Potential_backward_compatibility_issues_in_Joomla_4

/**
* Method to get a single record.
*
* @param   integer  $pk  The id of the primary key.
*
* @return  CMSObject|boolean  Object on success, false on failure.
*
* @since   1.6
*/

@return CMSObject|boolean Object on success, false on failure.

Then we are left with the philosophical question, what is success and what is error?

In my opinion, success is also the fact that the function returns an empty result - an empty object. It's still not an error.

To be clear

Calling:

$item = parent::getItem() will still return an empty object in Joomla 4 - This is done like $this->getItem() in the ArticleModel as an example. And all the API codes.

$item = parent::getItem(null) will still return an empty object in Joomla 4

$item = parent::getItem("") will still return an empty object in Joomla 4

$item = parent::getItem(0) will still return an empty object in Joomla 4

The only time Joomla 4 will return FALSE differently to Joomla 3 is if you try to load a row, specifying its primary key id number - and that row doesnt exist in the database table Eg:

$item = parent::getItem(999) where 999 is the primary key of a row, which no longer exists will return FALSE in Joomla 4

and only if calling the base method, and not any overridden version of getItem as there are many of those too

In my opinion, success is also the fact that the function returns an empty result - an empty object. It's still not an error.

I'll say this one last time. You are calling getItem method - the method cannot get you the item as the item doesnt exist - YOU expected it existed as you provided an id, but it doesnt exists - therefore that is an Exception (NOT FOUND) but Joomla provides an error/false.

This is the correct way it should be.

Returning an empty object when you were EXPECTING your object to be in the database is a dangerous thing to rely on.

I agree with @PhilETaylor here. And reverting that change would be a b/c break now in stable phase. So I think this issue and PR #35813 should be closed. But I am only a small maintainer and I might be wrong so I don’t think I am the right one to decide anything.

When getItem is called like this (Workflowcontroller)

$record = $this->getModel()->getItem($recordId);

		if (empty($record->id))
		{
			return false;
		}

then I can see that there is a lot of clean up that I could have done over and above what my PR originally cleaned up. But equally this code still works it seems.

Furthermore if you look at the PluginModel.php and others you will see that the method getItem is actually replaced with their own local copy, and in this instance it returns false if it was unable to load the record from the table - instead of returning an empty object... so Joomla is inconsistent at best....

I agree this is b.c. break compared to Joomla 3, and should be documented https://docs.joomla.org/Potential_backward_compatibility_issues_in_Joomla_4

Also I agree that changes were done is valid. Returning an object for empty result is confusing. No need to revert in my opinion.
Our fault that it was not documented.

However I see another issue: now it not easy to distinct "Model had error", and "Item not found".
Because "Item not found" now is an error

However I see another issue: now it not easy to distinct "Model had error", and "Item not found".

This is in fact, what I asked in first post:

The second question is: How to distinguish between empty result and error now?
#35811 (comment)

That's basically what I've been saying all along. If we take it as an error that there is no record, how do we differentiate that from a real error.

We have in fact three statuses:

1. item will be loaded ... no error
2. item does not exist, will be not loaded ... still no error - we get an error-free response ... no error
3. item will be not loaded because of db error, etc. ... this is the true error.

If we join 2) and 3) then we cannot differentiate the 2) from real error.

So in Joomla 4 Beta or Joomla 3, the purpose of this function was:

• separate only the real error and maybe
• build empty object if there is no data (this is very important for linked tables - for them, we need empty object when creating new item)

You distinguish by reading the error message that is set! Simple!

You distinguish by reading the error message that is set! Simple!

I'm not sure it's any kind of fun? If not, how do I achieve it?

if ($item = parent::getItem($pk)) {
 ...
} else {
    // False returned
    // How to get human readable and translated error messages from message queue and transform them to condition?
    // Something like parsing $app->getMessageQueue(); and what next?
}

You distinguish by reading the error message that is set! Simple!

nope,
Do you mean doing something like this?:

$model->getError() === Text::_('JLIB_APPLICATION_ERROR_NOT_EXIST')

Sorry but it a 💩

If we remove this part

Then it can be like this:

$item = $model->getItem($pk);

if ($model->getError())
{ 
  //... there an error
}
else if (!$item)
{
  // .. item not found
}

And In doing that you then break the whole reason that PR was created and re introduce the initial bug the PR sought to fix.

hm, how is that?
I do not suggest to revert it, but change how "Not found Error" is set.

In my opinion "Not found" message should be set within a View instead of forced in Model.
Because Developer may want to show something specific like "Foobar not found", instead of generic one.

Kind of this:

if ($return === false)
{
	if ($table->getError())
	{
		$this->setError($table->getError());
	}

	return false;
}

instead of

But only problem, then also need check every core View to add "Not found" message 😄

Well, I have no idea.

I don't know why we're talking about overengineering here. What exactly was wrong with the original version:

• when an item exists, an object with the item is returned
• if the item does not exist, an empty object is returned
• if an error occurs, false is returned (possibly initializing an error message)

Making a condition and splitting false or empty object can't be a problem or?

In the same or similar way as it is used in the core, example:
administrator/components/com_categories/src/Model/CategoryModel.php

if ($result = parent::getItem($pk))
		{

...

if (empty($result->id))
			{

or

if ($result->id != null)
			{

etc.

Because point two is stupid and should never happen - if you are asking for an item and that item doesn’t exist then that’s an error - you should never return an empty object, it’s just plain wrong to do so. Period!

See the original issue the PR was designed to solve and many others where an empty object previously is taken as a success when actually it’s a failure

This is just plainly wrong, no other database layer would ever return an empty object if you asked it for a non existent row. Name me any DBAL that would!? I’ll wait….

Sent from my iPhone

On 13 Oct 2021, at 10:53, Jan @.***> wrote:


I don't know why we're talking about overengineering here. What exactly was wrong with the original version:

when an item exists, an object with the item is returned
if the item does not exist, an empty object is returned
if an error occurs, false is returned (possibly initializing an error message)
Making a condition and splitting false or empty object can't be a problem or?

—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub, or unsubscribe.

I don't know why Joomla developers invented and used it for several years. It probably fulfilled some other function that I wrote about. There is no item, so create an empty object that will be used to initialize the new item (usually needed to join with other tables). The crucial thing is that with this design we can easily solve problems. We know how to easily separate the actual error and the empty result. And once again, the empty result is the success that is used to initialize a new entry for example.

We need to remember that this is a universal function that can serve in multiple places. For example, a place where we create a new item and ask if the item already exists. When an empty object is returned, we know that the item does not exist and we use that object to initialize the new item (in the form)

Well I've given you the revert PR and reopened the original issue that it solved so someone else can have a go at fixing the issue using a less architecturally correct solution.

It's now up to you to test the PR and get it passed two human tests and to pass a maintainers decision.

I can do no more.

It is stupid decisions like this that will hold Joomla back from ever being a great CMS again that non-hobby PHP developers enjoy working with.

Ironically Joomla has several places where it will load a row and assume that it loaded a real row and carry on without even checking whether the object is empty or not - so your argument is mute really, because even Joomla implements it wrong and doesn't check everywhere what is being returned from getItem !!!!!

Once again, I'm representing my opinion here and trying to point out that the original solution made some sense. I'm not saying I'm 100% right or that I don't understand why you modified the feature to work the way it does now. I'm just pointing out that it originally worked completely the opposite way and that maybe there was a reason for that. This isn't a fight about removing a PR, I just want to spark a discussion about whether we should modify it. The philosophical question of whether or not the empty result is a error or not should be answered by the core developers.

There is no such thing as "core developers"

You say you don't understand why I made the change - I have clearly explained the reason behind the change in my original Pr and showed why it was a problem and proposed a fix that was tested by two people - including the now president of open source matters and was merged by a maintainer.

Nothing you have said justifies why, when asking for a specific existing row by providing its primary key id, that a method that retrieves that, should return an empty row if it doesn't exists - apart from the fact "it used to do this". That is not a good enough argument.

If you want an empty object you CAN STILL get one by passing a null or empty pk - just like Joomla does in many places!

Hey, stop fighting each other 😄
The issue is clear, we just need a solution (ideally without reverting)

If only parts of my statements are read, then it is difficult to advise further. If all of my argument is summed up in the phrase "it used to do this", then I don't know how more to explain it.

If you want an empty object you CAN STILL get one by passing a null or empty pk - just like Joomla does in many places!

If I want something, I do it easily with writing my own getItem method in my model, it is not about what I want.

I just noticed that Empty object used everywhere,
ArticleModel

It more deep as I expected 😄
Sound like a reverting is a good idea. Or we need a bigger changes in all core Models which use Empty objects.

As I wrote, it looks like returning an empty object is the intention. The whole CMS counts on it. I don't know what exactly was intended, anyway, for the example I gave, it's appropriate: we ask for the existence of the item, if it doesn't exist yet, create an empty object so we can create a new instance right away.

I don't see any downside to that. We have clearly separated the states "error" and "empty object" and in case we need to create a new instance, we have a ready object with all the columns of the database table (ideal for loading into the Form object), etc.

The whole CMS counts on it.

Factually untrue. Scaremongering Rubbish. If that were the case Joomla 4.0.0 4.0.1, 4.0.2 and 4.0.3 would be insanely broken and the releases would have been a disaster. Its taken until now for a single person, you, to report an issue.

it's appropriate: we ask for the existence of the item, if it doesn't exist yet, create an empty object so we can create a new instance right away.

Absolutely pathetic implementation of a database later. Simply wrong. the method is called "getItem" - if you provide it an id it should provide that item or an exception. Simple.

I don't see any downside to that.

Except there are. Architecturally, and the root problem that the PR resolved shows there are several downsides to it. It is simply wrong for a method to be called getItem($pk) when the a $pk doesnt exist, to just return an empty object instead of throwing an exception (or returning false for legacy reasons)

and in case we need to create a new instance, we have a ready object with all the columns of the database table (ideal for loading into the Form object), etc.

This is plain stupid and wrong. If you call getItem with the id of an item, YOU ARE EXPECTING THAT ITEM TO EXIST !!! (else why are you providing an id if you are expecting to get a blank object?!) and what you are saying here is you dont care if it really exists, because you would be happy with an empty object THAT MAKES NO SENSE AT ALL.

If you WANT a blank object then call getItem with no $pk integer - simple, but if you provide an ID and that row doesnt exist in the database then returning a blank object is conceptually, architecturally plain stupid

Its clear you dont understand that. and for this reason Im unsubscribing, because whatever step you force the Joomla contributors to make, is going to be the wrong one.

@Fedik Yes, like I said, my PR could have been better implemented, I agree that.

This issue has a PR to test #35813 and therefore should be closed.

@PhilETaylor I don't understand why you're blaming me for something I didn't create. Something that the core Joomla developers created and used for a few years until the J4 RC version?

If you WANT a blank object then call getItem with no $pk integer - simple, but if you provide an ID and that row doesn't exist in the database then returning a blank object is conceptually, architecturally plain stupid

As written many times, it is about join tables with the current ID, not auto increment ID, if you will read my previous posts properly, I have written it there.

Anyway, if we forget everything this, I still didn't get answer how to differentiate between "error" and "empty data" when false is returned?

Its clear you dont understand that. and for this reason Im unsubscribing, because whatever step you force the Joomla contributors to make, is going to be the wrong one.

I don't force anyone into anything, my tone is friendly, non-confrontational, and I try to avoid escalating the discussion. So I don't understand your comments.

@PhilETaylor can you please revert only AdminModel part and adjust User HtmlView, the rest seems okay, or maybe I missed something?

hm, @PhocaCz found another fun thing, in old code if you do $model->getItem(1111); (with non existing id), it return empty table with this id assigned (I mean $object->id will be 1111), and it still impossible to distinguish between empty result and error.

As example when you try open /administrator/index.php?option=com_users&task=user.edit&id=11111 to edit, you will get a form to edit a user with ID 11111, instead of "Not found" error.

@wilsonge can you please have a look, maybe you have some idea? It looks like a big mess 😄

@Fedik

For the user table it seems to be a different story, the ID is supplied somewhere else.

libraries/src/Table/User.php line 65 (Joomla 3)

// Get the id to load.
if ($userId !== null)
{
	$this->id = $userId;
}
else
{
	$userId = $this->id;
}

The object basically gets each ID and keeps it ($this->id)

You also get a warning, but from a completely different place

libraries/src/User/User.php line 885(Joomla 3)

\JLog::add(\JText::sprintf('JLIB_USER_ERROR_UNABLE_TO_LOAD_USER', $id), \JLog::WARNING, 'jerror');

So it looks like the user table is a slightly different matter. But overall we need to look at the getItem method in the model globally, not just the method that affects the content (article) model, but the model of all parts of the system. And there you need to count on a solution that is as universal as possible (because each part can override its own solution), so that it fits all kinds of needs.

As I've written many times in this thread, I'm not here to argue that I'm right, but rather to point out the problem and somehow contribute to a consensus regarding the handling of errors and empty results.

it still impossible to distinguish between empty result and error

So it seems, in case of user table, you are right, which doesn't make the situation any easier, but rather adds another problem to the decision 😄

hm, I see, so there is another bug :)

will we get a discount when having found 10 bugs, "buy 10 and pay 8 only"?

BTW - maybe new issue should be done for the user table. It is very interesting.

• You can access the edit
• You can save it

• It tells you, that the user is successfully stored but all data will be emptied and there is no record in database

The bad news is that it's behaving very strangely.
The good news is that it hopefully won't allow you to save a record and thus might be meaningless, since the possibility of this happening to a normal user in normal use is slim.

Is there anything to be done here?

I agree with @brianteeman . Joomla 4 was released more than one year ago, so it's too late for us to discuss about backward incompatible changes now. Therefore, I'm closing this issue. Let's move forward !