Resolve optional parameters

[ghost]

Currently getPath does not correctly resolve optional parameters, this commit adds the method resolvePath which is called by default and unit tests to confirm a non breaking change

[philipobenito]

I'm away from a computer until Monday but first impressions on this is that I'm not overly keen on it.

You can already achieve the same effect by defining two routes that use the same controller.

If we were going to officially add optional route segments I'd prefer to add functionality to properly support them instead of making it work with one segment that optionally has a second within it (that's not currently intended use), but again, I'm not convinced it's required when app code can already explicitly define them as separate mappings.

[ghost]

Optional parts / parameters are already supported as stated in the readme ?

https://github.com/nikic/FastRoute/commit/79843dce62ac52e9b628e73d5f1264cad10c65a6

[philipobenito]

league/route is built on top of FastRoute but that doesn't necessarily mean feature parity.

However, that isn't really my point, what I'm saying is that the extra complexity introduced here shouldn't be required. I'd expect those params to already be in the route data and if it's not there must be a simpler way from FastRoute to make sure that it is, so to officially support optional segments I'd prefer to find that solution.

As mentioned though, the same effect is already achievable with multiple explicit definitions.

[philipobenito]

@luminateonedev okay I've taken a deeper look now.

So the issue isn't that optional routes aren't working, but that when you use getPath it keeps the square brackets? And keeps in parts that don't exist?

I'm not against solving that problem, however, I'm not keen on the current implementation in the PR. I think it can be done in a simpler way with maybe one more preg_replace to strip the segments that haven't been given a param, and adjusting the existing regular expression to remove square brackets that make it optional.

Let me know if you'd like to have a go at it, if not I will look when I get a little more time.

[ghost]

:) that's the issue we were getting urls that read /path[/{id}] when id was not supplied , I actually started trying to modify the regular expression but it starts to get messy due to edge cases

• The square brackets could a prefix or suffix ie /path[/prefix-{id}-suffix]
• Optional parameters can be nested
• Insure you don't leave an inner parameter unreplaced

At this point it seemed like duplication of the existing parsing and we opted to reuse that instead

[philipobenito]

@luminateonedev this commit should cover the criteria you listed above https://github.com/thephpleague/route/commit/f129860fb7f24f22eb85f42e5f70f8bf81535d7e

One note, if a route segment is not optional, and a replacement is not passed, the original string remains, while if it is optional and no replacement is provided, the segment is removed.

Wanna put it through it's paces? I'll have a think in case of any potential issues too. Only one I can think of it that it probably needs to stop processing segments as soon as it finds an optional one that doesn't have a replacement, I'll take a look at that in a moment.

Edit: Commit to address issue mentioned in previous paragraph https://github.com/thephpleague/route/commit/34d93089bd499b8a0c45e729fb4e73ec5763debb

[ghost]

Close, Added a couple of unit tests testGetPathReplacesOptionalMissingPrefixSuffix is failing

[philipobenito]

Close, Added a couple of unit tests testGetPathReplacesOptionalMissingPrefixSuffix is failing

Will get that passing tomorrow then 👍

[philipobenito]

@luminateonedev I think this covers everything, without getting ridiculous with it anyway https://github.com/thephpleague/route/commit/aeae4d1f9ba0eae4d6dcbb73c7a03c925114d81b

[ghost]

Thank you @philipobenito appreciate the work