Link and Plink macros: produce E_WARNING if link is not valid
- Filip Klimeš
- Nette Blogger | 156
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)
- Filip Klimeš
- Nette Blogger | 156
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).
- Filip Klimeš
- Nette Blogger | 156
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
- Nette Core | 8212
Actual state is really bad, I am for change exception to E_WARNING and enable it by default on production.
- bckp
- Member | 12
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š
- Nette Blogger | 156
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;
}
- Filip Klimeš
- Nette Blogger | 156
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
- Nette Core | 8212
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š
- Nette Blogger | 156
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
- Member | 108
@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…
- Filip Klimeš
- Nette Blogger | 156
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š
- Nette Blogger | 156
Bump.
I have edited and updated the RFC, please see the initial post.
- David Grudl
- Nette Core | 8212
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
.