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

5 years ago

Filip Klimeš
Member | 157
+
+1
-

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)

5 years ago

Aurielle
Member | 1283
+
0
-

What's wrong with something like this?

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

5 years ago

Filip Klimeš
Member | 157
+
0
-

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).

5 years ago

bckp
Member | 12
+
0
-

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)

5 years ago

Filip Klimeš
Member | 157
+
0
-

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)

5 years ago

David Grudl
Nette Core | 6887
+
+2
-

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

5 years ago

Filip Klimeš
Member | 157
+
0
-

Do you mean producing the E_WARNING on INVALID_LINK_WARNING?

5 years ago

David Grudl
Nette Core | 6887
+
0
-

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

5 years ago

Filip Klimeš
Member | 157
+
0
-

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

5 years ago

bckp
Member | 12
+
0
-

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)

5 years ago

Filip Klimeš
Member | 157
+
0
-

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;
}

5 years ago

David Grudl
Nette Core | 6887
+
+2
-

It should be configurable via config.neon

5 years ago

Filip Klimeš
Member | 157
+
0
-

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)

5 years ago

David Grudl
Nette Core | 6887
+
+2
-

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 ;)

5 years ago

Filip Klimeš
Member | 157
+
+1
-

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)

5 years ago

Quinix
Member | 108
+
0
-

@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…

5 years ago

David Grudl
Nette Core | 6887
+
0
-

EXCEPTION should be removed

5 years ago

Filip Klimeš
Member | 157
+
0
-

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.

5 years ago

Filip Klimeš
Member | 157
+
0
-

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

5 years ago

David Grudl
Nette Core | 6887
+
+1
-

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.

5 years ago

norbe
Backer | 404
+
+1
-

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..

5 years ago

Filip Klimeš
Member | 157
+
0
-

Thank you, gentlemen, good points.

5 years ago

Filip Klimeš
Member | 157
+
0
-

Pull-request created

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