Invalid filters #128

Open
recck opened this Issue Aug 30, 2012 · 4 comments

Projects

None yet

2 participants

recck commented Aug 30, 2012

Should alert the user that the filter is invalid, rather than omitting the 404 page.

Such as saying:

'rule' => '/what/:ever',
'filters' => ['ever' => ':badFilter']

or

'filters' => ['ever' => 'a-zA-Z']

Either will throw the 404 page. Perhaps validate that if the filter begins with : and isn't a key within $default_filters that it returns the default filter instead, or if the filter returns false on a preg_match call, return the default filter.

Owner
trq commented Aug 30, 2012

Hmmm. I like the idea, but maybe an exception would be better suited to the situation. After all, it's a developer issue if they use an undefined filter.

I think defaulting to the default filter might actually cause more confusion.

recck commented Aug 30, 2012

Yea, I was trying to keep the confusion in mind, but at least an additional message could suffice.

Owner
trq commented Aug 30, 2012

I should spend more time thinking before I reply to these things.

Routes need to silently fail for a reason. That reason is, so that the next route might get a chance to resolve. For example:

<?php

$payload = $router
    ->attach(
        'home-page',
        new \Proem\Routing\Route\Standard([
            'rule' => '/what/:ever',
            'filters' => ['ever' => ':badFilter']
        ])
    )->attach(
        'login',
        new \Proem\Routing\Route\Standard([
            'rule' => '/what/:ever',
            'filters' => ['ever' => ':alpha']
        ])
    )->route();

The first route will fail to match because of the use of :badFilter, the second however will potentially resolve because it uses a defined filter.

It is only after all routes have failed to resolve that a 404 is thrown and by that time, there is no way to backtrack through each route to see why it might have failed.

Owner
trq commented Aug 30, 2012

I guess though, as you said, checking if the filter begins with : and then throwing (an exception) if it's not defined might still be the best idea. It's quite obviously never going to match anything.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment