Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Frontend: Changing JError to enqueueMessage when access is not permitted (Solves partly https://github.com/joomla/joomla-cms/issues/7812 #7824

Merged
merged 2 commits into from
Sep 14, 2015

Conversation

infograf768
Copy link
Member

See test instructions in #7812

When access is not permitted, in the cases taken care off in this PR, we will get an Error message instead of loading the error.php page.

@mbabker
Copy link
Contributor

mbabker commented Sep 5, 2015

There's a small concern here I didn't think about before (not trying to block this in case someone gets that impression). In our bootstrap, we define the error handler for JError::raiseError() to ultimately call JError::customErrorPage() which renders the page as a JDocumentError instance (what we're trying to avoid here). One of the things JDocumentError::render() does is set the response headers with the correct 403 HTTP code. That needs to be accounted for IMO if we are going to move away from rendering an error document, otherwise you're going to have error pages go from returning a 403 code to a 200 code which is incorrect for that context.

@infograf768
Copy link
Member Author

@mbabker
If you think this is wrong, I could use instead:
JError::raiseWarning(403, JText::_('JERROR_ALERTNOAUTHOR'));

What do you think?

@mbabker
Copy link
Contributor

mbabker commented Sep 5, 2015

raiseWarning doesn't change the response headers either. So there's really two options:

  1. Leave raiseError calls as is, like I mentioned before JDocumentError just sucks when it comes to adding extra bells and whistles.
  2. Set the response header ($app->setHeader()) the same way JDocumentError::render() does in the controller. You avoid the error page and still set the right HTTP code.

@infograf768
Copy link
Member Author

  1. Set the response header ($app->setHeader()) the same way JDocumentError::render() does in the controller. You avoid the error page and still set the right HTTP code.

That's over my head I am afraid. Better close this and let it to someone more qualified.

Could you look also at #7825 where the matter is slightly different?

@infograf768 infograf768 closed this Sep 5, 2015
@infograf768 infograf768 reopened this Sep 5, 2015
@infograf768
Copy link
Member Author

Added setHeader as suggested by @mbabker

@Bakual
Copy link
Contributor

Bakual commented Sep 5, 2015

Looks fine from review.
Thanks for doing this. This will likely serve as an example for other cases :)

@gwsdesk
Copy link

gwsdesk commented Sep 5, 2015

Applied patch and looks fine to me as well. Thanks JM

@infograf768
Copy link
Member Author

2 testers OK. RTC


This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/7824.

@joomla-cms-bot joomla-cms-bot added the RTC This Pull Request is Ready To Commit label Sep 6, 2015
@Kubik-Rubik Kubik-Rubik added this to the Joomla! 3.4.5 milestone Sep 11, 2015
@Kubik-Rubik
Copy link
Member

Thank you @infograf768!

Kubik-Rubik added a commit that referenced this pull request Sep 14, 2015
Frontend: Changing JError to enqueueMessage when access is not permitted (Solves partly #7812
@Kubik-Rubik Kubik-Rubik merged commit 379ab86 into joomla:staging Sep 14, 2015
@joomla-cms-bot joomla-cms-bot removed the RTC This Pull Request is Ready To Commit label Sep 14, 2015
@zero-24 zero-24 modified the milestones: Joomla! 3.4.6, Joomla! 3.5.0 Oct 28, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants