I wouldn't change the exception throwing back to the deprecated JError. That is the wrong approach.
the problem is a few lines below in the catch block (https://github.com/joomla/joomla-cms/blob/staging/components/com_contact/models/contact.php#L335). There the full exception object is passed to $this->setError(). Change that from $this->setError($e); to $this->setError($e->getMessage()); and you get a fine message instead.

Ok. It sounds strange to change the throw by the JError... I'll check that.

Done.

Shall we not display a non authorised message in that case as it seems that even after merging #7824 , we do not get the correct message for a contact.

Something like:

            $access = $this->getState('filter.access');

            try
            {
                $db->setQuery($query);
                $result = $db->loadObject();

                if (empty($result))
                {
                    if ($access)
                    {
                        throw new Exception(JText::_('COM_CONTACT_ERROR_CONTACT_NOT_FOUND'), 404);
                    }
                    else
                    {
                        throw new Exception(JText::_('JERROR_ALERTNOAUTHOR'), 403);
                    }
                }
            }
            catch (Exception $e)
            {
                $this->setError($e->getMessage());

                return false;
            }

I'll prepare those corrections.

@Bakual
Is the solution OK? com_contacts model is really different from the other components.

To me it looks like bad design if you have to check the source of the issue at this time.
Ideally, the correct exception message should be thrown in the model itself.

@Bakual
Not sure I understand.
Do you mean all components should throw the exception/error in the model (what is done in this PR) and not the way it is done now (com_contents, com_newsfeeds).
Shortly said: is the PR here OK or not?

Sorry, my fault. I remembered this PR wrong and for some reason thought the code was in the view.

Imho, it is wrong to distinguish the error message based on a filter state. You don't know why the result is false. It may be because the contact doesn't exist or becauise the permissions aren't sufficient.

But I don't understand that method to begin with. It is named getContactQuery but returns an object and not the query. I'm not even sure what it is supposed to do since we also have a getItem method (which calls the getContactQuery one).

From the code it seems that getContactQuery should mean something like getContactDetails, as its result is being used to load the contact profile and the contact articles.
getItem uses a protected array (_item[]) to store retrived before contacts (line 142), but once those contacts are in the array it always ask for its details (line 252), so it doesn't load the contact twice but it loads the details every time is called.
For me is not clear to throw an error when the contact details are loaded instead of being raised when the contact is loaded. Maybe using a throwin lines 198 or 204 and moving the call to getContactQuery inside the same if which loads the contact will correct this.

I think this PR is still wrong. Also, the code in ContactModelContact is wrong / bad, too.

1. The method getContactQuery method should not query the database again to get contact data as it is already loaded. As @jlainezs mentioned, it should be called getContactDetails. To be safe, I think instead of modifying that method, we should write a method called getContactExtendedData (or getContactExtendedData) to get the details information of the contact.

2. Then on this line https://github.com/joomla/joomla-cms/blob/staging/components/com_contact/models/contact.php#L254, we can change the code to:

if ($extendedData = $this->getContactExtendedData($this->_item[$pk]))
{
      $this->_item[$pk]->articles = $extendedData->articles;
      $this->_item[$pk]->profile = $extendedData->profile;
}

Happy to make a new PR to replace this one if needed.

Tested @joomdonation modifications in a file he sent me and it works great. We now get the Error message from viewthml display() OK

@infograf768
so those changes, are going to be pushed in a different PR?

I will let @joomdonation discuss this with you :)

@jlainezs Feel free to make the change to this PR. I think we both have same thinking about this issue and solution.

For your information, here is my code with the direction mentioned on my comment

https://gist.github.com/joomdonation/d57c9b14dbb9ac1a5798

Nice! I'll take it tomorrow afternoon and will do a push.

OK here. Thanks.

@joomdonation can you please test if it works as expected with the changes? If yes we can move it together with @infograf768 's test to RTC. Thanks.

@zero-24 As part of the code is mine, I already tested it. However, I am not sure I am allowed to give the test result?

I've tested it before pushing and, for me, its OK.

Thanks. Moving to RTC than!

why add another function buildContactExtendedData

when still exists getContactQuery
needs more review imho

getContactQuery is not good because it has an extra query to get the contact data while we have the data available already

I decided to add buildContactExtendedData instead of improve getContactQuery because I am worry that it might be used somewhere and cause something broken.