Move from helper->error to exception

Discussion forum for Extension Writers regarding Extension Development.
User avatar
KaileyT
Community Team Member
Community Team Member
Posts: 2995
Joined: Mon Sep 01, 2014 1:00 am
Location: sudo rm -rf /
Name: Kailey Truscott

Move from helper->error to exception

Post by KaileyT »

So, it looks like the helper->error method is going away in 3.1.3. Is there any documentation on moving to the exception method? Is it as simple as using throw new \phpbb\exception("LANG_KEY"); and adding LANG_KEY to my language file?
Kailey Truscott - Community Team
User avatar
MattF
Extensions Development Coordinator
Extensions Development Coordinator
Posts: 5376
Joined: Sat Jan 17, 2009 9:37 am
Location: Los Angeles, CA
Name: Matt Friedman
Contact:

Re: Move from helper->error to exception

Post by MattF »

It's not going away in 3.1.3. It will be around for a long time, at least until 3.3.0 which could be years away.

But it would be to throw new \phpbb\exception\http_exception(200, 'OPTIONAL_LANG_VAR', array(optional parameters used in the lang var));
Formerly known as VSEMy ExtensionsPlease do not PM me for support.
nicofuma
3.2 Release Manager
3.2 Release Manager
Posts: 546
Joined: Sun Apr 13, 2014 1:47 am
Location: Grenoble - France

Re: Move from helper->error to exception

Post by nicofuma »

I'll write a wiki page when I will find the time but to be simple :
throw new \phpbb\exception\runtime_exception("LANG_KEY")); if you want to throw a runtime exception (ie: an error like when the server send an error 500), it will send a response with the http status code 500
throw new \phpbb\exception\http_exception(status_code, "LANG_KEY"); if you want to send an error with a custom http status code (404 not found or 403 Forbidden per example)

Note: If it's an ajax request a Json response is sent automatically
Note2: If the language string takes arguments you can pass them in an array \phpbb\exception\runtime_exception("LANG_KEY", array("param1", "param2")) and \phpbb\exception\http_exception(404, "LANG_KEY", array("param1", "param2"))
Member of phpBB Development-Team
No Support via PM
User avatar
KaileyT
Community Team Member
Community Team Member
Posts: 2995
Joined: Mon Sep 01, 2014 1:00 am
Location: sudo rm -rf /
Name: Kailey Truscott

Re: Move from helper->error to exception

Post by KaileyT »

So the controller equivalent of trigger_error() (like when I have a route setup for my extension and want to show an error) would be throw new \phpbb\exception\http_exception(200, 'LANG_VAR')? I get the difference between runtime for server and http for client, just not sure of informational errors. Sorry, this exception stuff is new to me.
Kailey Truscott - Community Team
nicofuma
3.2 Release Manager
3.2 Release Manager
Posts: 546
Joined: Sun Apr 13, 2014 1:47 am
Location: Grenoble - France

Re: Move from helper->error to exception

Post by nicofuma »

200 means that everything is okay and if everything is okay you shouldn't throw an exception or use trigger_error. Instead you should use the template explicitly (by the way we could add a method for that in the helper)

So what do you want to say to your user exactly?
Member of phpBB Development-Team
No Support via PM
User avatar
KaileyT
Community Team Member
Community Team Member
Posts: 2995
Joined: Mon Sep 01, 2014 1:00 am
Location: sudo rm -rf /
Name: Kailey Truscott

Re: Move from helper->error to exception

Post by KaileyT »

OK, I have an extension which allows users to contribute news items. My route looks like this: /frontnews/id=xx (with xx being the news_id). If no news_id is specified or the SQL query returns no results, I need to tell the user so. In the old days (3.0), I would have the following code:

Code: Select all

if (!$news_id)
{
    trigger_error('NO_NEWS');
} 
With 3.1, I'm using

Code: Select all

if (!$news_id)
{
    return $this->helper->error($this->user->lang('NO_NEWS'));
} 
Since I want to move to exceptions now instead of later, I need to replace that code with the exception equivalent.
Kailey Truscott - Community Team
nicofuma
3.2 Release Manager
3.2 Release Manager
Posts: 546
Joined: Sun Apr 13, 2014 1:47 am
Location: Grenoble - France

Re: Move from helper->error to exception

Post by nicofuma »

in this case the correct answer should be an error 404 I think and so
throw new \phpbb\exception\http_exception(Response::HTTP_NOT_FOUND, "NO_NEWS");
Member of phpBB Development-Team
No Support via PM
User avatar
KaileyT
Community Team Member
Community Team Member
Posts: 2995
Joined: Mon Sep 01, 2014 1:00 am
Location: sudo rm -rf /
Name: Kailey Truscott

Re: Move from helper->error to exception

Post by KaileyT »

OK, thanks!
Kailey Truscott - Community Team
User avatar
MattF
Extensions Development Coordinator
Extensions Development Coordinator
Posts: 5376
Joined: Sat Jan 17, 2009 9:37 am
Location: Los Angeles, CA
Name: Matt Friedman
Contact:

Re: Move from helper->error to exception

Post by MattF »

nicofuma wrote:200 means that everything is okay and if everything is okay you shouldn't throw an exception or use trigger_error. Instead you should use the template explicitly (by the way we could add a method for that in the helper)
But that is what helper->error already does.
https://github.com/phpbb/phpbb/blob/dev ... #L193-L201

$helper->error() so far has been to replace trigger_error, and show the user a nice error message:
$helper->error($user->lang('MSG_TO_USER), 200);

Now it sounds like you are saying we don't have a replacement for the above line, even though throwing an http_exception(200, 'MSG_TO_USER') results in the exact same behavior as above, and seems the logical replacement. But you're saying NOT to throw an exception if we just want to show the user a message that uses the 'message_body.html' template, such as in usages like this:
https://github.com/phpbb-extensions/boa ... hp#L64-L67

Maybe you should duplicate the helper->error() method, and call it helper->message() but hard-code it's $code argument to 200. Otherwise I'm just confused now :?
Formerly known as VSEMy ExtensionsPlease do not PM me for support.
nicofuma
3.2 Release Manager
3.2 Release Manager
Posts: 546
Joined: Sun Apr 13, 2014 1:47 am
Location: Grenoble - France

Re: Move from helper->error to exception

Post by nicofuma »

Actually the usage of trigger_error is pretty bad in Olympus an in phpBB in general: the idea of trigger_error is to display an error, not to show a message to the user and it's the same for the exceptions.
Do you want to use the same way to say "Everything is okay, have a good day" and "Your page doesn't exist" ? They don't have the same meaning. The first one is a good news, a normal page and so require a normal response (ie: 200) but the second one isn't and require an error code (404 here).

So yes, we should add an other method called message() which will have the same code as error() but this time you will use throw new http_exception(404,"NO_PAGE"); and $this->message("ALL_OK"); instead of $this->error("NO_PAGE"); and $this->error("ALL_OK");.
Completely different, no? (and yes we could use an exception for the ALL_OK message, but it doesn't have any sense... the idea is to move to a clear and meaningful api instead of keeping the old, not really existent and completely messy api)
Member of phpBB Development-Team
No Support via PM
User avatar
Wolfsblvt
Registered User
Posts: 634
Joined: Sun Oct 26, 2014 9:12 pm
Location: Solingen, Germany
Contact:

Re: Move from helper->error to exception

Post by Wolfsblvt »

This sounds pretty good, nicofuma, and also the way how it should work in programming.
Throwing exceptions for program flow is always bad, so it shouldn't be the default way to write messages for users display.
"Exception" (in it's original meaning) means something not normal happened, that is an exception from the normal program flow and often even an unexpected behavior.

Where I am not sure if I agree is using exceptions for user "error" messages. For example if the user wrote too less characters for a post, there needs to be some kind of error. I would never consider this to be an exception.
Or if user is trying to ignore a moderator for example. He should get an error message, but why should that be an exception?
If you have a specific extension request and you are willing to pay for, you can write me a PM.
My extensions (Trending: @Mention SystemAdvanced PollsUser Online Time)

»Du kamst zu uns. Deine Stimme kam. Du zeigtest uns die Sterne. Sie funkelten. Wir konnten sehen.«
User avatar
KaileyT
Community Team Member
Community Team Member
Posts: 2995
Joined: Mon Sep 01, 2014 1:00 am
Location: sudo rm -rf /
Name: Kailey Truscott

Re: Move from helper->error to exception

Post by KaileyT »

nicofuma wrote:not to show a message to the user and it's the same for the exceptions.
Do you want to use the same way to say "Everything is okay, have a good day" and "Your page doesn't exist"
You're reading too far into this. The example I gave above was one message I have migrate. Do we really need to go through each and ever single case? trigger_error() was much easier - something went wrong? trigger an error, whether that be no news, unauthorized access, too few characters for a news post, etc.

I don't mind exceptions, as long as there is clear documentation and examples of using them. From the discussion we are having here, that doesn't seem to be the case.
Kailey Truscott - Community Team
User avatar
MattF
Extensions Development Coordinator
Extensions Development Coordinator
Posts: 5376
Joined: Sat Jan 17, 2009 9:37 am
Location: Los Angeles, CA
Name: Matt Friedman
Contact:

Re: Move from helper->error to exception

Post by MattF »

nicofuma wrote:in this case the correct answer should be an error 404 I think and so
throw new \phpbb\exception\http_exception(Response::HTTP_NOT_FOUND, "NO_NEWS");
Response::HTTP_NOT_FOUND will not work in the version of Symfony currently used by phpBB 3.1.3, and needs to be the actual error code instead (404), until phpBB 3.2.0 (or until Symfony 2.5 or later is added to phpBB).
Formerly known as VSEMy ExtensionsPlease do not PM me for support.
nicofuma
3.2 Release Manager
3.2 Release Manager
Posts: 546
Joined: Sun Apr 13, 2014 1:47 am
Location: Grenoble - France

Re: Move from helper->error to exception

Post by nicofuma »

VSE wrote:
nicofuma wrote:in this case the correct answer should be an error 404 I think and so
throw new \phpbb\exception\http_exception(Response::HTTP_NOT_FOUND, "NO_NEWS");
Response::HTTP_NOT_FOUND will not work in the version of Symfony currently used by phpBB 3.1.3, and needs to be the actual error code instead (404), until phpBB 3.2.0 (or until Symfony 2.5 or later is added to phpBB).
right... sorry I'm too often on develop
I don't mind exceptions, as long as there is clear documentation and examples of using them. From the discussion we are having here, that doesn't seem to be the case.
Actually it's not difficult, an exception it's like an error: it's something exceptional that shouldn't happen. And in your example it's clearly a use case for an exception: there is an error in the user input.

So today, what we are trying to do is to fix the api so later we could do specific actions (such as logging) when an exception is thrown, and that's why we advise you to use the corrected api.
Member of phpBB Development-Team
No Support via PM
User avatar
nickvergessen
Former Team Member
Posts: 4397
Joined: Mon Apr 30, 2007 5:33 pm
Location: Stuttgart, Germany
Name: Joas Schilling
Contact:

Re: Move from helper->error to exception

Post by nickvergessen »

Basically you should render your own template:
https://github.com/phpbb/phpbb/pull/3353/files
You can do that yourself aswell, we only add it to the helper to have a short cut.
No Support via PM
Post Reply

Return to “Extension Writers Discussion”