Skip to content
This repository was archived by the owner on May 1, 2019. It is now read-only.

[WIP] Fix: Regular expression in route definition #462

Closed

Conversation

localheinz
Copy link
Member

This PR

  • adds a failing test, asserting the route doesn't match if the user name is invalid
  • fixes a config key which should be constraints instead of constrains
  • fixes the regular expression for the route match with the feedback of @Ocramius and @ins0

Follows #444.

@localheinz localheinz self-assigned this Mar 5, 2015
@localheinz localheinz changed the title Fix: Regular expression in route defintion [WIP] Fix: Regular expression in route defintion Mar 5, 2015
@localheinz localheinz changed the title [WIP] Fix: Regular expression in route defintion [WIP] Fix: Regular expression in route definition Mar 5, 2015
@localheinz localheinz force-pushed the fix/regular-expression branch 2 times, most recently from 773f2ca to 000a674 Compare March 5, 2015 14:37
@localheinz
Copy link
Member Author

@gianarb @ins0 @Ocramius

In regard to this, it seems we've got a few more unresolved issues, see the reason for the failing test above:

1. There are more routes which would need the same constraints

'view-module' => [
    'type'    => 'Segment',
    'options' => [
        'route'    => '/:vendor/:module',
        'defaults' => [
            'controller' => Controller\IndexController::class,
            'action'     => 'view',
        ],
        // constraints required here
        'constraints' => [
            'vendor' => '[a-zA-Z][a-zA-Z0-9_-]*', // same regular expression required here
            'module' => '', // some regular expression required here
        ],
    ],
],
'zf-module'   => [
    'type'          => 'Segment',
    'options'       => [
        'route'    => '/module',
        'defaults' => [
            'controller' => Controller\IndexController::class,
            'action'     => 'index',
        ],
    ],
    'may_terminate' => true,
    'child_routes'  => [
        'list'   => [
            'type'    => 'Segment',
            'options' => [
                'route'      => '/list[/:owner]',
                'constrains' => [ // should be constraints, by the way
                    'owner' => '[a-zA-Z][a-zA-Z0-9_-]*', // same regular expression required here
                ],
                'defaults'   => [
                    'action' => 'organization',
                ],
            ],
        ],
    ],
],

2. Yes, there is a user named user and also a user named module

Right here they are:

h/t @gianarb

@gianarb
Copy link
Contributor

gianarb commented Mar 5, 2015

also exist module username :P :trollface:

@ins0
Copy link
Contributor

ins0 commented Mar 5, 2015

#444 (comment) covers the username format but i feel uncomfortable writing such regex everytime we need a username in the url 😃

@localheinz
Copy link
Member Author

@gianarb

True, here she is: https://github.com/module!

@localheinz
Copy link
Member Author

@ins0 @gianarb

I'd prefer to extract a constant here, what do you guys think?

@ins0
Copy link
Contributor

ins0 commented Mar 5, 2015

@localheinz

vendor => ^(?!.*--.*)([a-zA-Z0-9][a-zA-Z0-9\-]+?[a-zA-Z0-9])$|(^[a-zA-Z0-9]+$)
module => ^(?![^a-zA-Z0-9.\-_])[a-zA-Z0-9.\-_]+$

I'd prefer to extract a constant here

don't not know really what you mean

@localheinz
Copy link
Member Author

@ins0

I would define constants containing the regular expressions, then refer to these constants in the route definitions to avoid duplication.

@ins0
Copy link
Contributor

ins0 commented Mar 5, 2015

👍

@localheinz localheinz force-pushed the fix/regular-expression branch 2 times, most recently from fae8ddf to a262856 Compare March 6, 2015 11:22
@localheinz localheinz force-pushed the fix/regular-expression branch from a262856 to 25d4db7 Compare March 6, 2015 14:00
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants