Link and Plink macros: produce E_WARNING if link is not valid

Filip Klimeš

Nette\Application\UI\PresenterComponent::link() should produce E_USER_WARNING

Motivation

Recently, a bug escaped into our product enviroment even though we have covered the code with integration tests. When invalid links are generated via {link} and {plink} macros, only text describing the error is printed out. To be able to detect such error, E_USER_WARNING should be produced by the macros, or more precisely by the PresenterComponent::link() function.

Solution

I suggest changing the PresenterComponent::invalidLinkMode values to:

INVALID_LINK_SILENT: produces E_USER_WARNING and displays ‘#’ (default on production)
INVALID_LINK_VISUAL: produces nothing, displays red link
INVALID_LINK_WARNING: produces E_USER_WARNING, displays red link in case warnings are suppressed (default in development)

And setting INVALID_LINK_SILENT by default on production and INVALID_LINK_WARNING by default in dev environment.

Configuring

You could directly change PresenterComponent::invalidLinkMode as it is now, or you could force silent mode via configuration

application:
	silentInvalidLinks: yes # default value is "no"

Which keeps INVALID_LINK_SILENT on production and sets INVALID_LINK_VISUAL in dev environment.

Use cases

This way, you could be able to log warnings on production with ‘#’ being displayed in place of an invalid link. In dev environment, you could detect invalid links by unit/integration tests, because warning would be produced.

Last edited by Filip Klimeš (2015-02-20 09:06)

Aurielle

What's wrong with something like this?

protected function startup()
{
	parent::startup();
	$this->invalidLinkMode = self::INVALID_LINK_EXCEPTION;
}

Filip Klimeš

Well, this would produce errors in product environment. Of course, I could somehow detect the env and set invalidLinkMode accordingly, but IMHO that's not clean.

Furthermore, INVALID_LINK_WARNING implies there should be a warning (not only text saying I got it wrong).

bckp

But warning should be only on production environment, so it is same like your solution, with your detecting env and setting invalidLinkMode.

Last edited by bckp (2015-02-12 12:22)

Filip Klimeš

Warning is automagically turned on according to debugMode value. This is clean, because it's done in a factory, not in the presenter itself. I don't want to write my own logic which decides what invalidLinkMode to use.

I suggest either producing E_WARNING or having an ability to set which mode (INVALID_LINK_WARNING or INVALID_LINK_EXCEPTION) I prefer in the configuration. I am willing to post a pull-request, if this gets approved.

Last edited by FilipKlimeš (2015-02-12 12:38)

David Grudl

Actual state is really bad, I am for change exception to E_WARNING and enable it by default on production.

Filip Klimeš

Do you mean producing the E_WARNING on INVALID_LINK_WARNING?

David Grudl

Maybe. This is RFC, so it is on you to present your solution.

Filip Klimeš

Cool, I just didn't fully understand what you meant :)

bckp

What about adding new option, E_LINK_ERROR, that will trigger E_USER_ERROR with message of exception. On production this will be saved to the error log, on develop Tracy will show this.

//EDIT:
Question is, what about INVALID_LINK_WARNING, should this trigger E_USER_ERROR to or E_USER_WARNING, or not?

Last edited by bckp (2015-02-12 16:20)

Filip Klimeš

My solution would be quite simple:

protected function handleInvalidLink(InvalidLinkException $e)
{
	if ($this->invalidLinkMode === self::INVALID_LINK_SILENT) {
		return '#';
	} elseif ($this->invalidLinkMode === self::INVALID_LINK_WARNING) {
		trigger_error('Invalid link: ' . $e->getMessage(), E_USER_WARNING); // <-- Here is the change
		return '#error: ' . $e->getMessage();
	} else { // self::INVALID_LINK_EXCEPTION
		throw $e;
	}
}

and alter the PresenterFactory to make things more strict:

if ($presenter instanceof UI\Presenter && $presenter->invalidLinkMode === NULL) {
	$presenter->invalidLinkMode = $this->container->parameters['debugMode'] ? UI\Presenter::INVALID_LINK_EXCEPTION : UI\Presenter::INVALID_LINK_WARNING;
}

David Grudl

It should be configurable via config.neon

Filip Klimeš

David Grudl wrote:

It should be configurable via config.neon

Okay, what about:

application:
	invalidLinkMode: warning # silent / exception ('warning' by default)

and then in PresenterFactory:

public function __construct(Nette\DI\Container $container, $autoRebuild = FALSE, $invalidLinkMode = UI\Presenter::INVALID_LINK_WARNING)
{
	// ...
	$this->invalidLinkMode = $invalidLinkMode;
}

// ...

if ($presenter instanceof UI\Presenter && $presenter->invalidLinkMode === NULL) {
	$presenter->invalidLinkMode = $this->invalidLinkMode;
}

EDIT: oh, and it's also configured here. Well, these implementation details would be addressed in the PR.

Last edited by FilipKlimeš (2015-02-12 16:54)

David Grudl

My usecases are

I am developing and want to see red links (by default)
I am developing and want to see warning
App is on production and I want to see warning in logs and # in HTML

So: no, it is not suitable for me ;)

Filip Klimeš

Okay then, valid point! What about:

protected function handleInvalidLink(InvalidLinkException $e)
{
	switch($this->invalidLinkMode) {
		case self::INVALID_LINK_SILENT:
			trigger_error('Invalid link: ' . $e->getMessage(), E_USER_WARNING);
			return '#';

		case self::INVALID_LINK_WARNING:
			trigger_error('Invalid link: ' . $e->getMessage(), E_USER_WARNING);
		case self::INVALID_LINK_VISUAL:
			return '#error: ' . $e->getMessage();

		case self::INVALID_LINK_EXCEPTION:
			throw $e;
	}
}

I would prefer having INVALID_LINK_WARNING by default in development (so I'm able to discover the invalid link by automated tests) and INVALID_LINK_SILENT by default on production.

EDIT: Thank you very much for the patience, this is my first RFC. :)

EDIT2: I'm not having a good day. There was no option for just displaying the red links without warning, so I edited the snippet. Maybe we could create INVALID_LINK_SILENT (default on production), INVALID_LINK_VISUAL and INVALID_LINK_WARNING (default in dev). INVALID_LINK_EXCEPTION could be left there for marginal use cases and could be configured.

Last edited by FilipKlimeš (2015-02-12 17:27)

Quinix

@FilipKlimeš I personally don't see much difference between proposed INVALID_LINK_WARNING and current INVALID_LINK_EXCEPTION in dev mode. Both will result in Tracy screen and has to be fixed when they occur.

Logging of invalid link in production is usefull, but maybe it should log in to separate file…

David Grudl

EXCEPTION should be removed

Filip Klimeš

Oh god, my brain was probably on vacation earlier today.

Three states of invalidLinkMode:

INVALID_LINK_SILENT: produces E_USER_WARNING and displays ‘#’ (default on production)
INVALID_LINK_VISUAL: produces nothing, displays red link
INVALID_LINK_WARNING: produces E_USER_WARNING, displays red link in case warnings are suppressed (default in development)

I stand by the INVALID_LINK_WARNING being default in development, because of automated tests.

InvalidLinkMode could be set in configuration via:

application:
	invalidLinkMode: silent/visual/warning

The silent mode would be enforced in the production, though. Displaying exception message could be a security issue.

Filip Klimeš

Bump.
I have edited and updated the RFC, please see the initial post.

David Grudl

These modes are fine, but invalidLinkMode: silent/visual/warning is not very intuitive. Mode warning sounds like something I want to enable on production, but the reality is exactly the opposite. Better names are maybe development/production/peace :-)

In fact, you do not want to switch between three modes, but only between two: to disable/enable warnings during development. So maybe is enough to switch tracy.strictMode and invalidLinkMode is unnecessary. Or we can use switch (for example) quietInvalidLinks: true/false.

norbe

Please do not use tracy.strictMode for this. I always develop my app at strict mode. But at the beginning I have o lot of links that are invalid (becouse functionality is not implemented). quietInvalidLinks: true/false looks good for me..

Filip Klimeš

Thank you, gentlemen, good points.

Filip Klimeš · Pull-request created *Last edited by Filip Klimeš (2015-02-22 13:13)*

Pull-request created

Last edited by Filip Klimeš (2015-02-22 13:13)