restricted Contact page displays error stack instead of 404 or 403 error page (Fix #7053)

[jlainezs]

This is a fix for the issue #7053. It is solved as suggested on the report.

---

Steps to reproduce the issue

Create a contact, public & published, save, memorise id (1).
Create another contact, public & unpublished, save, memorise id (2).
Create another contact, registered & published, save, memorise id (3).
Create a menu item "list contacts in category", call it "contacts".
Navigate to http://mysite.com/contacts to see the list of contacts as public, you'll see just one contact (1).

Navigate to this contact (http://mysite.com/contacts/1-somename), all good.

Navigate to unpublished contact (http://mysite.com/contacts/2-somename), see 404 "Contact not found" error page. All good.

Navigate to registered contact (http://mysite.com/contacts/3-somename), see long error message with stack trace.

Expected result

404 or 403 Error page (error template based) saying "contact not found" or "not authorised"

Actual result

Normal page (not error template based) with just an error message:

Error

exception 'Exception' with message 'Contact not found' in C:\home\bgpa\j31\components\com_contact\models\contact.php:328 Stack trace: #0 C:\home\bgpa\j31\components\com_contact\models\contact.php(252): ContactModelContact->getContactQuery(28) #1 C:\home\bgpa\j31\libraries\legacy\view\legacy.php(401): ContactModelContact->getItem() #2 C:\home\bgpa\j31\components\com_contact\views\contact\view.html.php(66): JViewLegacy->get('Item') #3 C:\home\bgpa\j31\libraries\legacy\controller\legacy.php(690): ContactViewContact->display() #4 C:\home\bgpa\j31\components\com_contact\controller.php(42): JControllerLegacy->display(true, Array) #5 C:\home\bgpa\j31\libraries\legacy\controller\legacy.php(728): ContactController->display() #6 C:\home\bgpa\j31\components\com_contact\contact.php(15): JControllerLegacy->execute(NULL) #7 C:\home\bgpa\j31\libraries\cms\component\helper.php(391): require_once('C:\home\bgpa\j3...') #8 C:\home\bgpa\j31\libraries\cms\component\helper.php(371): JComponentHelper::executeComponent('C:\home\bgpa\j3...') #9 C:\home\bgpa\j31\libraries\cms\application\site.php(191): JComponentHelper::renderComponent('com_contact') #10 C:\home\bgpa\j31\libraries\cms\application\site.php(230): JApplicationSite->dispatch() #11 C:\home\bgpa\j31\libraries\cms\application\cms.php(252): JApplicationSite->doExecute() #12 C:\home\bgpa\j31\index.php(40): JApplicationCms->execute() #13 {main}

System information (as much as possible)

J3.4.1, SEF on, error reporting off (!!!)

Additional comments

Probably a clash of old and new ways of raising errors (JError vs Exception) in contact model.

adding this line in components/com_contact/models/contact.php @ line 257 fixes the situation:

else JError::raiseError(404, JText::_('COM_CONTACT_ERROR_CONTACT_NOT_FOUND'));

[Bakual]

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.

[jlainezs]

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

---

_{This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/7867.}

[jlainezs]

Done.

[infograf768]

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;
            }

[jlainezs]

I'll prepare those corrections.

[infograf768]

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

[Bakual]

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.

[infograf768]

@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?

[Bakual]

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).

[jlainezs]

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.

[joomdonation]

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.

[infograf768]

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

[jlainezs]

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

[infograf768]

I will let @joomdonation discuss this with you :)

[joomdonation]

@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

[jlainezs]

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

[infograf768]

OK here. Thanks.

---

_{This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/7867.}

[zero-24]

@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.

[joomdonation]

@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?

[jlainezs]

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

[zero-24]

Thanks. Moving to RTC than!

---

_{This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/7867.}

[alikon]

why add another function buildContactExtendedData
when still exists getContactQuery
needs more review imho

[joomdonation]

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.