Bugfix/mandatory and optional args

[ynnoig]

📚 Description

PR to solve the route pattern match in case of mandatory and optional args.

🔖 Changes

• Added a pathPattern class variable in Route class to make this customizable in route definition
• Thank pathPattern class variable the pathPattern is calculated only one time
• introduced new method calculateDefaultPathPattern() that try to split the route path on / and for each part find is a mandatory arg or an optional arg.

[ynnoig]

@Chemaclass This PR is not fully completed and it is just a first draft to try to solve the problem.

[antonio-gg-dev]

Hello @ynnoig ! I've taken a quick look at your proposal, and I honestly think it has the potential to become the final solution we're looking for. However, I have some questions and would like to hear your opinion.

One of the indirectly related changes you made is adding a new attribute called "pathPattern" to the routes. In this attribute, you're storing the calculations performed by the "getPathPattern" method so that they don't need to be repeated when the method is called again later.

All my questions are focused on this attribute. I want to fully understand your perspective.

This method "getPathPattern" is typically called once per route until a match is found. Once this happens, it is called two more times within the implementation of the RouteParams constructor (which could be reduced to one if we store the result in a variable and use that variable). In any case, except for the matching route, the calculation is performed between 0 and 1 times for the rest of the routes. Have you conducted any tests to see if there are performance or resource usage improvements? Are there any other reasons why you believe this is a better approach?

Additionally, you not only added this attribute to the Route, but you also allow it to be optionally defined during construction. In what cases could this new functionality be useful? If we decide to keep this attribute, wouldn't it be better for it to be simply an internal attribute that cannot be defined during construction?

Keep in mind that Gacela's premise is simplicity, both in the interfaces we expose in our classes and in the implementations we provide. We like to ensure that there is a significant benefit before going against our goals. We don't want to add these types of optimizations prematurely, especially when we don't have a guarantee of results. Even though it's just one more attribute, adding another option to the API, along with others, can gradually increase complexity without us realizing it.

When I have some more time, I'll delve deeper. We're eager to have this in production. Thank you again for your time and contribution.

[Chemaclass]

After checking this out in detail, I found that adding this in RouteParams::55 then makes the feature test pass. most precisely the ?? ''.

'string' => $pathParams[$paramName] ?? '',

The only missing thing now is 2 "mutant tests" that need some attention (aka there are no tests covering this new behavior completely.) Do we need the U flag on the regex expressions from the Route (lines 111,113)?

TL;DR: @ynnoig @Tito-Kati apply these diff changes and your current branch will be 100% green 🍏

[ynnoig]

This is just what i have in my working copy. I have only to add some more tests and then I push everything.

[ynnoig]

Hallo @Tito-Kati

Thank you for taking a look at my suggested solution (as said is still a draft, wip).
To answer your first question I think you did it yourself by asking the second question. As you precisely noted I included the possibility of deciding in the configuration phase of a routing the definition of a custom pattern. In this case the default pattern calculation method is not called and the pattern injected at configuration time is used. Hence the choice to insert the class variable to achieve precisely this change. The reason might be simpler than you might think: for example, the user can define that a given parameter should be hexadecimal or numeric and so on...

At the level of performance or resource usage, I don't think there is an impact that we need to consider, other than that we have an extra class variable that contains a string. The pattern is always as before calculated on the fly (in case is not provided from user) at the time the route match is required, and then as you have well pointed out the calculation is performed 0 or maximum 1 time for the rest of the routes.

[Chemaclass]

Awesome work! Thank you 💯