Console route defaults should be overridden by entered values

[ezk84]

The suggested fix for issue #3358 (pull request #4182) violates the assumption that the defaults are fallback values; instead the defaults are being used to override any values entered at the console.

This PR includes a test case for this reasonable/expected behaviour.

[weierophinney]

We have a case of competing expectations of the component happening here.

I think the root of the problem is this:

• Developers do not expect literal parameters to appear in the route match (e.g., the route create controller <name> is not expected to create "create" and "controller" parameters in the route match).
• Developers do expect parameters that act as flags or which accept values to appear in the route match (e.g., the route create controller <name> should create a "name" parameter in the route match).

As such, what we need to test are the following:

• create controller should have an empty route match
• create controller with a default value for "controller" specified should return that default value
• create controller <controller> should have a "controller" key in the route match, with the value captured for <controller>

Any solution at this point that does not include those three test cases (and, obviously, have them passing) will not work.

@ezk84 - are you willing to write those test cases and/or provide the solution for them?

Also, @Thinkscape @bakura10 @robertboloc -- does the above synopsis make sense to each of you?

[ezk84]

@weierophinney I can provide those test cases though this change will also affect previous test cases that assume the current functionality.

Another aspect to consider is where there are alternatives or optional literals:

• create [controller] would have to have a key for "controller" in route match
• create (controller | foo | bar) would have to have the same for "controller", "foo" and "bar"

In effect, this change should only apply to strictly mandatory literals.

[bakura10]

This makes sense @weierophinney. RouteMatch object should have two different arrays to store custom param names and parameters that are set internally. The problem is that $this->params()-fromRoute() method will be more difficult. We could do a merge of both arrays here, but what if someone want to retrieve the action name if it has been overrided by a custom param in a route ?

[Thinkscape]

create controller should have an empty route match
create controller with a default value for "controller" specified should return that default value
create controller <controller> should have a "controller" key in the route match, with the value captured for <controller>

That logic makes sense. It'd need some explaining in docs, but it should do the trick.

However:

create [controller]

With a default value it'd be impossible to determine if controller has indeed been specified by the user.

create ( controller | foo | bar )

This must allow for probing for each of those options.

The question that remains is what kind of logic applies to those? According to with a default value for "controller" specified should return that default value, in case controller has been used on the console, we'd fetch its value from defaults. Similarly, we could fetch default value for controller, foo and bar in the case of alternating params.

[robertmarsal]

I also think this makes perfect sense.

[weierophinney]

@Thinkscape Yes -- I guess the last point I should have made was:

• Any alternatives or optional literal segments should continue to be returned as flag values

With that said.. @ezk84 do you want to take this on? If not, I can either give you a PR later this week with changes, or do an alternate PR that incorporates your work so far. Thoughts?

[Thinkscape]

Question 1:

1. Route contains alternatives (foo | bar)
2. Route contains a default value for bar => "something"
3. User types foo at the command line.
4. Should ->get("bar") return false or "something" ?

Question 2:

1. Route contains an optional literal param [foo]
2. Route contains a default value for foo => "something"
3. User does not type foo at the command line.
4. Should ->get("foo") return false or "something" ?

[weierophinney]

@Thinkscape

Re: Question 1: In the case of literal alternatives, I'd expect a boolean false value if the alternative is not matched, and the default value if matched. (This is, however, where literals on the console are acting very differently than on the URL; on the URL, they are only used to match, but are not returned in the matches.)

Re: Question 2: same answer as the first. Alternatives are really simply another form of optional value.

To be honest, I think the cleanest solution is not to return matched literals in the matches, and thus behave more like the HTTP routers. The only way to really accomplish it, however, would be to remove the possibility of optional literals, and, by extension, alternative literals -- which would be a big BC break. I think if instead we define the behavior as above, we'd have a clear story, and would support the expected use cases as outlined in the various issues.

[ezk84]

@weierophinney okay I'll take this on, I think we have a good grasp of the cases so will proceed as discussed.

[Thinkscape]

Ok, we can proceed this way. The only downside is a need for even more detailed explaining in docs, as the logic is now quite complex - one has to observe different kinds of param definitions and default/non-defaults cases (not going into the discrepancies as compared to http routes).

@weierophinney Matched literals are required for proper branching, for example:

perform action [ quickly ] ( left | right )

Granted, you could split that into 4 separate routes, but it's much more convenient to just query if "quickly" has been used, and which direction the user wants to take (left or right). If you removed matched literals from RM you completely loose that functionality.

[weierophinney]

@Thinkscape One way to manage branching in the future (aka, ZF3) would be to implement trees in the console routing, just like in the HTTP router. That would allow defining the route like:

'perform' => array(
    'type' => 'Simple',
    'options' => array(
       'route' => 'perform action [quickly]',
       'defaults' => array( /* ... */),
    ),
    'may_terminate' => false,
    'child_routes' => array(
        'left' => array('options' => array(
            'route' => 'left',
            'defaults' => array(/* ... */),
        )),
        'right' => array('options' => array(
            'route' => 'right',
            'defaults' => array(/* ... */),
        )),
    ),
)

It's more verbose, but allows setting different values for the matched literals, and the alternatives are more clearly separated. It also has the side effect of making the rules more explicit -- literals do not map to match values.

Thoughts?

[weierophinney]

The only downside is a need for even more detailed explaining in docs, as the logic is now quite complex - one has to observe different kinds of param definitions and default/non-defaults cases (not going into the discrepancies as compared to http routes).

Agreed -- but it's also the route (no pun intended) that provides the least breakage to existing applications. We can make it simpler and more consistent in ZF3.

[weierophinney]

@ezk84 Will you be able to finish by next Monday? Do you need any assistance?

[ezk84]

@weierophinney I should be able to handle it over the weekend, but will let you know if it do end up needing help, cheers.

[ezk84]

@weierophinney Please review.

I almost had a change of heart regarding the second second part of this discussion; i.e. for flags, default values do not represent the default 'state' of the flag but the value that will be used when the flag is 'on'. Whilst implementing that, it seemed fanciful and contrived (as in, what's a reasonable use case?).

This is why i've split this into two commits, the first gets us to solving the original issue by not including non-optional literal params in the route match. The second one takes it the rest of the way, if you guys still believe it's nessesary.

Seems there's been a few added test cases since I branched :S

[weierophinney]

@ezk84 The test cases look perfect -- thanks so much for doing this work!

[weierophinney]

Merged to develop for release with 2.2.0.