How to display user messages in ext.php

Discussion forum for Extension Writers regarding Extension Development.
User avatar
canonknipser
Registered User
Posts: 2096
Joined: Thu Sep 08, 2011 4:16 am
Location: Germany
Name: Frank Jakobs
Contact:

Re: How to display user messages in ext.php

Post by canonknipser »

As you say, it depends ...
In my extension, I need a specific PHP-function installed by a PHP-library (regular delivered in every installation, but you never know ...)
So I use a function_exist-call to check if that library is installed -> https://github.com/canonknipser/viewexi ... xt.php#L25
Greetings, Frank
phpbb.de support team member
English is not my native language - no support via PM or mail
New arrival - Extensions and scripts for phpBB
User avatar
rubencm
Development Team Member
Development Team Member
Posts: 104
Joined: Fri Oct 05, 2007 2:24 pm
Location: Spain

Re: How to display user messages in ext.php

Post by rubencm »

What do you think about using exceptions? It would be necessary to modify a little bit the phpBB core (it whould be for 3.3.x i think). This is my idea:

Code: Select all

	public function is_enableable()
	{
		if (this extension should not be enabled)
		{
			$this->container->get('user')->add_lang_ext('vendor/extension', 'errormessage');
			throw new extension_exception($this->container->get('user')->lang('MEANINGFUL_ERROR_MESSAGE']);
		}
		return true;
	}
And in the ACP doing something like this:

Code: Select all

try
{
	$extension_manager->enable(); // enable or wathever
}
catch(\phpbb\extensions\exception $e)
{
	$lang_key = $e->getMessage();
	trigger_error($user->lang($lang_key).adm_back_link(...));
}
catch(\phpbb\extensions\extension_exception $e)
{
	trigger_error($e->getMessage().adm_back_link(...));
}
This would be also compatible with CLI:

Code: Select all

try
{
	$extension_manager->enable(); // enable or wathever
}
catch(\phpbb\extensions\exception $e)
{
	$lang_key = $e->getMessage();
	$io->error($user->lang($lang_key));
}
catch(\phpbb\extensions\extension_exception $e)
{
	$io->error($e->getMessage());
}
For now, documentation only say that ext.php methods sould only return true or false, so this modification would be compatible with all exceptions except those that have used "hacks" to do this.
User avatar
javiexin
Code Contributor
Posts: 1157
Joined: Wed Oct 12, 2011 11:46 pm
Location: Madrid, Spain
Name: Javier
Contact:

Re: How to display user messages in ext.php

Post by javiexin »

If we were to go that route, I would propose the use of exceptions without translating, and let that to the presentation layer.

So, the code would be something like:

Code: Select all

	public function is_enableable()
	{
		if (this extension should not be enabled)
		{
			$this->container->get('user')->add_lang_ext('vendor/extension', 'errormessage');
			throw new extension_exception('MEANINGFUL_ERROR_MESSAGE', array(any relevant args));
		}
		return true;
	}
Then, in the ACP:

Code: Select all

	try
	{
		if (!$this->ext_manager->get_extension($ext_name)->is_enableable())
		{
			throw new extension_exception('EXTENSION_NOT_ENABLEABLE');
		}
	}
	catch (exception_interface $e)
	{
		$message = call_user_func_array(array($this->user, 'lang'), array_merge(array($e->getMessage()), $e->get_parameters()));
		trigger_error($message, E_USER_WARNING);
	}
And in extension manager:

Code: Select all

	try
	{
		if (!$extension->is_enableable())
		{
			return false;
		}
	}
	catch (exception_interface $e)
	{
		return false;
	}
No need to change the CLI.

Note that the code is slightly more complex to accommodate both the use of exceptions and the current use of booleans as return code. This way, it would be 100% bc, plus it would allow extensions to throw exceptions with meaningful errors without the need to trigger_error, that breaks in the CLI.

The other way to go would be to add an error handler to the CLI so that trigger_error works as expected there, but that was already ruled out.
-javiexin
User avatar
MattF
Extensions Development Coordinator
Extensions Development Coordinator
Posts: 5859
Joined: Sat Jan 17, 2009 9:37 am
Location: Los Angeles, CA
Name: Matt Friedman

Re: How to display user messages in ext.php

Post by MattF »

When an extension is not installed, it's not part of the container, so you can't load any of its services...you can not:

Code: Select all

$this->container->get('user')->add_lang_ext('vendor/extension', 'errormessage');
Also, in 3.3, extensions will be installed through composer within the ACP so composer error messages will be output instead of what's being discussed here.

using ext.php to do any of this has always just been a temporary workaround until composer is supported. https://github.com/phpbb/phpbb/pull/3909
Formerly known as VSEMy ExtensionsPlease do not PM me for support.
User avatar
javiexin
Code Contributor
Posts: 1157
Joined: Wed Oct 12, 2011 11:46 pm
Location: Madrid, Spain
Name: Javier
Contact:

Re: How to display user messages in ext.php

Post by javiexin »

VSE wrote: Fri Apr 21, 2017 6:33 am When an extension is not installed, it's not part of the container, so you can't load any of its services...you can not:

Code: Select all

$this->container->get('user')->add_lang_ext('vendor/extension', 'errormessage');
Also, in 3.3, extensions will be installed through composer within the ACP so composer error messages will be output instead of what's being discussed here.

using ext.php to do any of this has always just been a temporary workaround until composer is supported. https://github.com/phpbb/phpbb/pull/3909
Correction: no extension service is being loaded, only "core" service(s) (user or language) are loaded from the container. And the language file does not depend on the enableability (if that is a word) of the extension, but only on the availability of the extension (already tested and validated) and the language file itself (responsibility of the developer and the forum admin). So this code is perfectly fine, and there are a number of extensions showing it and using it without any issue.

In any case, with or without composer taking control, there is currently NO WAY for an extension to "explain" the end user what is the problem and why it may not be enabled. Installing through composer will not make any difference, as it will not be able to "guess" the reason why the extension is not enableable. Only the extension author will know, and may be able to provide useful information to the end user.

And regarding the installation through composer... well, we have had this conversation before: leaving behind a fix that could be applied today for something that is maybe two years down the road is not something I like, but if that's the way it is, so be it. And in this case, it will not make any difference, so better take it into consideration before completing the implementation of composer installation anyway.

-javiexin
User avatar
RMcGirr83
Former Team Member
Posts: 22016
Joined: Wed Jun 22, 2005 4:33 pm
Location: Your display
Name: Rich McGirr

Re: How to display user messages in ext.php

Post by RMcGirr83 »

This seems to work just fine in 3.1 and 3.2?

https://github.com/rmcgirr83/phpBB-3.1- ... er/ext.php
Former Modifications/Extensions Team Member | My extensions | github | All requests for support via PM will be ignored
Appreciate the extensions/mods/support then buy me a beer Image
User avatar
javiexin
Code Contributor
Posts: 1157
Joined: Wed Oct 12, 2011 11:46 pm
Location: Madrid, Spain
Name: Javier
Contact:

Re: How to display user messages in ext.php

Post by javiexin »

RMcGirr83 wrote: Fri Apr 21, 2017 2:45 pm This seems to work just fine in 3.1 and 3.2?

https://github.com/rmcgirr83/phpBB-3.1- ... er/ext.php
Yes, except trigger error gives an error/warning in the CLI.
User avatar
david63
Registered User
Posts: 20646
Joined: Thu Dec 19, 2002 8:08 am

Re: How to display user messages in ext.php

Post by david63 »

javiexin wrote: Fri Apr 21, 2017 2:58 pm Yes, except trigger error gives an error/warning in the CLI.
But how many actually install, or need to install, extensions using CLI?
David
Remember: You only know what you know and - you don't know what you don't know!

I now no longer support any of my extensions but they will start to become available here
Paul
Infrastructure Team Leader
Infrastructure Team Leader
Posts: 28619
Joined: Sat Dec 04, 2004 3:44 pm
Location: The netherlands.
Name: Paul Sohier
Contact:

Re: How to display user messages in ext.php

Post by Paul »

If you return false (With trigger_error as well) it will prevent the installation.
User avatar
RMcGirr83
Former Team Member
Posts: 22016
Joined: Wed Jun 22, 2005 4:33 pm
Location: Your display
Name: Rich McGirr

Re: How to display user messages in ext.php

Post by RMcGirr83 »

david63 wrote: Fri Apr 21, 2017 3:29 pm
javiexin wrote: Fri Apr 21, 2017 2:58 pm Yes, except trigger error gives an error/warning in the CLI.
But how many actually install, or need to install, extensions using CLI?
Speaking for myself, I haven't used it ever.
Former Modifications/Extensions Team Member | My extensions | github | All requests for support via PM will be ignored
Appreciate the extensions/mods/support then buy me a beer Image
User avatar
javiexin
Code Contributor
Posts: 1157
Joined: Wed Oct 12, 2011 11:46 pm
Location: Madrid, Spain
Name: Javier
Contact:

Re: How to display user messages in ext.php

Post by javiexin »

Let me quote the initial message from this thread, as it seems it is not clear.
javiexin wrote: Sat Mar 11, 2017 11:44 pm Hello,

I would like to know what is the right way to display "meaningful" messages for the user in ext.php.

What I was doing is:

Code: Select all

	public function is_enableable()
	{
		if (this extension should not be enabled)
		{
			$this->container->get('user')->add_lang_ext('vendor/extension', 'errormessage');
			trigger_error($this->container->get('user')->lang('MEANINGFUL_ERROR_MESSAGE'], E_USER_WARNING);
		}
		return true;
	}
Well, this works just fine in acp_extension, failing when it should and showing the message correctly. But if I run phpbbcli extension:enable then there is a kind of stack trace (with the error message showing twice), and what is worse, the extension is enabled. Maybe I have to put a return false; after trigger_error but I think that an error should be enough to interrupt code...

Is this a CLI error/limitation, or is something that I am not doing quite right?
After a rather long investigation and very useful comments from VSE, this was the final conclusion:
javiexin wrote: Tue Mar 14, 2017 6:07 pm What I will finally use as a pattern, is

Code: Select all

	public function is_enableable()
	{
		$is_enableable = <condition that must be met>;
		if (!$is_enableable)
		{
			$this->container->get('user')->add_lang_ext('vendor/extension', 'errormessage');
			@trigger_error($this->container->get('user')->lang['MEANINGFUL_ERROR_MESSAGE'], E_USER_WARNING);
		}
		return $is_enableable;
	}
Tested both with the ACP and CLI, works fine.
Special facts collected during the discussion:
  • The call to trigger_error stops any further execution within the ACP, but it continues from where it left it in the CLI, so you MUST return false after calling trigger_error for the function to work in the CLI as well, not assuming that trigger_error would stop execution.
  • There is no error handler registered in the CLI, so that creates additional errors that may be suppressed by using the '@' operator.
  • Both of the above can be remedied by adding a custom error handler in the CLI, simplifying the one in functions.php; sample working code was provided (viewtopic.php?p=14685191#p14685191).
But this was regarded as a "non-issue". Ok, fine, as there is a relatively clean workaround (although not documented, and that costed quite some time in investigating the issue).

The "new" aspect here (when the discussion was reopened) is the suggestion by rubencm to use exceptions rather than trigger_error for these types of situations. Other than that, the previous workaround was fine (except I still think that the CLI is broken if it does not honor trigger_error and stops execution).

And yes, I do not normally use the CLI either. But if it is included, it should work correctly. Otherwise, it might be better to drop it altogether. Maybe it is not such a bad idea.
-javiexin
User avatar
RMcGirr83
Former Team Member
Posts: 22016
Joined: Wed Jun 22, 2005 4:33 pm
Location: Your display
Name: Rich McGirr

Re: How to display user messages in ext.php

Post by RMcGirr83 »

javiexin wrote: Fri Apr 21, 2017 5:18 pm But if it is included, it should work correctly. Otherwise, it might be better to drop it altogether. Maybe it is not such a bad idea.
-javiexin
I would think the worthless captchas would be the first to go. They haven't worked correctly and/or have been beaten for years.
Former Modifications/Extensions Team Member | My extensions | github | All requests for support via PM will be ignored
Appreciate the extensions/mods/support then buy me a beer Image
Paul
Infrastructure Team Leader
Infrastructure Team Leader
Posts: 28619
Joined: Sat Dec 04, 2004 3:44 pm
Location: The netherlands.
Name: Paul Sohier
Contact:

Re: How to display user messages in ext.php

Post by Paul »

The problem is that a WARNING never stops execution. thats why it is a warning ;). phpBB is misusing the trigger_errors to display messages to the user, so it is the GUI that actually needs fixing, not the CLI (That is how the behaviour should be).

The is_enableable was never really ment to have trigger_error called, but to only return a boolean. That boolean determs if a extension can be enabled or not.
User avatar
javiexin
Code Contributor
Posts: 1157
Joined: Wed Oct 12, 2011 11:46 pm
Location: Madrid, Spain
Name: Javier
Contact:

Re: How to display user messages in ext.php

Post by javiexin »

Paul wrote: Fri Apr 21, 2017 6:02 pm The problem is that a WARNING never stops execution. thats why it is a warning ;). phpBB is misusing the trigger_errors to display messages to the user, so it is the GUI that actually needs fixing, not the CLI (That is how the behaviour should be).

The is_enableable was never really meant to have trigger_error called, but to only return a boolean. That boolean determines if a extension can be enabled or not.
I agree with both of your comments.

However, trigger_error(...,E_USER_WARNING); does stop the execution in the GUI. If we were to look at it from that "correct" viewpoint, then quite a lot of code in the ACP would be "incorrect" as it assumes that after trigger_error, execution is stopped. The behaviour of trigger_error should be consistent across phpBB: if it stops execution in the GUI, it should do so in the CLI, and if it does not stop in the CLI, then it should not stop in the GUI. The source of the problem is inconsistency. And by the way, this is also heavily used in extensions, so if this were to be changed, it would break a lot of them (most of the ones that have an ACP module for that matter).

Now, if we take the above conversation aside for a minute, we have yet another issue: in what "correct" way may an extension communicate an end user the reason why it is not accepting being enabled? The "EXTENSION_IS_NOT_ENABLEABLE" message that the ACP shows (by the way, using trigger_error) seems to me quite vague so that someone may act accordingly.

If we rule out trigger_error, then the exception option proposed by rubencm is quite correct. It is flexible, requires minimum code changes in the core, and is simple enough for extension authors to implement (throw an exception with the right error instead of return false in is_enableable). What other alternatives exist?

-javiexin
Paul
Infrastructure Team Leader
Infrastructure Team Leader
Posts: 28619
Joined: Sat Dec 04, 2004 3:44 pm
Location: The netherlands.
Name: Paul Sohier
Contact:

Re: How to display user messages in ext.php

Post by Paul »

javiexin wrote: Fri Apr 21, 2017 6:28 pm
Paul wrote: Fri Apr 21, 2017 6:02 pm The problem is that a WARNING never stops execution. thats why it is a warning ;). phpBB is misusing the trigger_errors to display messages to the user, so it is the GUI that actually needs fixing, not the CLI (That is how the behaviour should be).

The is_enableable was never really meant to have trigger_error called, but to only return a boolean. That boolean determines if a extension can be enabled or not.
I agree with both of your comments.

However, trigger_error(...,E_USER_WARNING); does stop the execution in the GUI. If we were to look at it from that "correct" viewpoint, then quite a lot of code in the ACP would be "incorrect" as it assumes that after trigger_error, execution is stopped. The behaviour of trigger_error should be consistent across phpBB: if it stops execution in the GUI, it should do so in the CLI, and if it does not stop in the CLI, then it should not stop in the GUI. The source of the problem is inconsistency. And by the way, this is also heavily used in extensions, so if this were to be changed, it would break a lot of them (most of the ones that have an ACP module for that matter).
Because we are not rewriting phpBB within 1 release, so you have a some code that is already rewritten (Or just new), like the CLI, and some code that is old, that can behave differently.
Extensions should, (Except for the ACP I guess), not use trigger_error. And we do warn extension authors regarding this during validation. However, in some cases it is simply the only/best solution to use trigger_error.
Now, if we take the above conversation aside for a minute, we have yet another issue: in what "correct" way may an extension communicate an end user the reason why it is not accepting being enabled? The "EXTENSION_IS_NOT_ENABLEABLE" message that the ACP shows (by the way, using trigger_error) seems to me quite vague so that someone may act accordingly.
In general within the description of the extension ;).

If we rule out trigger_error, then the exception option proposed by rubencm is quite correct. It is flexible, requires minimum code changes in the core, and is simple enough for extension authors to implement (throw an exception with the right error instead of return false in is_enableable). What other alternatives exist?

-javiexin
Like said before, the real solution will be the composer integration, which is coming in 3.3.
Post Reply

Return to “Extension Writers Discussion”