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

Joomla 4.0: Removing calls to JError::raiseError() #12272

Merged
merged 6 commits into from Oct 29, 2016

Conversation

Projects
None yet
5 participants
@Hackwar
Member

Hackwar commented Oct 2, 2016

This PR removes almost all calls to JError::raiseError() and replaces them with either a JViewGenericdataexception or a JUserAuthorizationException.

JError::raiseError(500, $model->getError());
return false;
throw new Exception($model->getError(), 500);

This comment has been minimized.

@wilsonge

wilsonge Oct 5, 2016

Contributor

This one in the controller means we can probably do something better than this?

@@ -51,8 +51,7 @@ public function delete()
{
if (!JFactory::getUser()->authorise('core.admin', $this->option))
{
JError::raiseError(500, JText::_('JERROR_ALERTNOAUTHOR'));
jexit();
throw new Exception(JText::_('JERROR_ALERTNOAUTHOR'), 500);

This comment has been minimized.

@wilsonge

wilsonge Oct 5, 2016

Contributor

OK Let's make these 403's whilst we are here

@@ -71,8 +70,7 @@ public function publish()
{
if (!JFactory::getUser()->authorise('core.admin', $this->option))
{
JError::raiseError(500, JText::_('JERROR_ALERTNOAUTHOR'));
jexit();
throw new Exception(JText::_('JERROR_ALERTNOAUTHOR'), 500);

This comment has been minimized.

@wilsonge

wilsonge Oct 5, 2016

Contributor

403

@@ -91,8 +89,7 @@ public function reorder()
{
if (!JFactory::getUser()->authorise('core.admin', $this->option))
{
JError::raiseError(500, JText::_('JERROR_ALERTNOAUTHOR'));
jexit();
throw new Exception(JText::_('JERROR_ALERTNOAUTHOR'), 500);

This comment has been minimized.

@wilsonge

wilsonge Oct 5, 2016

Contributor

403

@@ -111,8 +108,7 @@ public function saveorder()
{
if (!JFactory::getUser()->authorise('core.admin', $this->option))
{
JError::raiseError(500, JText::_('JERROR_ALERTNOAUTHOR'));
jexit();
throw new Exception(JText::_('JERROR_ALERTNOAUTHOR'), 500);

This comment has been minimized.

@wilsonge

wilsonge Oct 5, 2016

Contributor

403

@@ -131,8 +127,7 @@ public function checkin()
{
if (!JFactory::getUser()->authorise('core.admin', $this->option))
{
JError::raiseError(500, JText::_('JERROR_ALERTNOAUTHOR'));
jexit();
throw new Exception(JText::_('JERROR_ALERTNOAUTHOR'), 500);

This comment has been minimized.

@wilsonge

wilsonge Oct 5, 2016

Contributor

403

@@ -57,8 +57,7 @@ public function delete()
if (!JFactory::getUser()->authorise('core.admin', $this->option))
{
JError::raiseError(500, JText::_('JERROR_ALERTNOAUTHOR'));
jexit();
throw new Exception(JText::_('JERROR_ALERTNOAUTHOR'), 500);

This comment has been minimized.

@wilsonge

wilsonge Oct 5, 2016

Contributor

403

JError::raiseError(500, $error);
return false;
throw new Exception(implode("\n", $errors), 500);

This comment has been minimized.

@wilsonge

wilsonge Oct 5, 2016

Contributor

OK So whilst we are here should a module failing stop the entire page from loading??

This comment has been minimized.

@mbabker

mbabker Oct 5, 2016

Member

This one should probably be caught by the module's base file and enqueue a message I'd say. Modules probably shouldn't kill the entire page but it'll have to be a case-by-case to figure out what to do.

@@ -52,7 +52,7 @@ public function click()
}
catch (RuntimeException $e)
{
JError::raiseError(500, $e->getMessage());
throw new Exception($e->getMessage(), 500);

This comment has been minimized.

@wilsonge

wilsonge Oct 5, 2016

Contributor

Can we use setError here for now please and return false and let the view handle the error for consistency

@@ -96,7 +96,7 @@ public function send()
{
if (strpos($_POST[$field], $header) !== false)
{
JError::raiseError(403, '');
throw new Exception('', 403);

This comment has been minimized.

@wilsonge

wilsonge Oct 5, 2016

Contributor

Let's define a message for the love of god

@zero-24 zero-24 added this to the Joomla 4.0 milestone Oct 5, 2016

@Hackwar

This comment has been minimized.

Member

Hackwar commented Oct 6, 2016

I reverted several of the changes to concentrate on the view.html.php files. I also introduced 2 new exceptions, JViewGenericdataexception and JUserAuthorizationException. Those 2 are consistently used throughout the view.html.php files. I'm wondering if it wouldn't be better to create a new branch and new PR with just those view.html.php files...

@mbabker

This comment has been minimized.

Member

mbabker commented Oct 6, 2016

There are also a couple new Exception classes added for either 3.6.3 or 3.7
from Andre. I think his was only a controller exception but I say it
because I think it dealt with authorization.

On Thursday, October 6, 2016, Hannes Papenberg notifications@github.com
wrote:

I reverted several of the changes to concentrate on the view..php files.
I also introduced 2 new exceptions, JViewGenericdataexception and
JUserAuthorizationException. Those 2 are consistently used throughout the
view.
.php files. I'm wondering if it wouldn't be better to create a new
branch and new PR with just those view.*.php files...


You are receiving this because you commented.
Reply to this email directly, view it on GitHub
#12272 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AAWfodYFEqP2cgK9trq2_5jZ7Bpp8aTiks5qxXctgaJpZM4KMJdF
.

@Hackwar Hackwar force-pushed the Hackwar:j4error branch from e49893a to e94f72e Oct 6, 2016

@Hackwar

This comment has been minimized.

Member

Hackwar commented Oct 6, 2016

I've reverted most of the stuff and started over and just concentrated on the view files. Taking babysteps here... I would keep it at this and do further refactoring in other PRs.

JError::raiseError(403, JText::_('COM_MAILTO_LINK_IS_MISSING'));
return false;
throw new Exception(JText::_('COM_MAILTO_LINK_IS_MISSING'), 403);

This comment has been minimized.

@wilsonge

wilsonge Oct 7, 2016

Contributor

I think this is a 400 not a 403?

@wilsonge

This comment has been minimized.

Contributor

wilsonge commented Oct 7, 2016

This is beginning to look significantly better :)

Hackwar added some commits Oct 7, 2016

@Hackwar

This comment has been minimized.

Member

Hackwar commented Oct 11, 2016

Should we merge this for the moment or do you want more changes before accepting this?

@wilsonge wilsonge merged commit ea71f82 into joomla:4.0-dev Oct 29, 2016

2 checks passed

continuous-integration/drone the build was successful
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment