flightphp / core

An extensible micro-framework for PHP
https://docs.flightphp.com
MIT License
2.63k stars 409 forks source link

Named Parameters Are Actually In Order... #584

Closed starfishpatkhoo closed 4 months ago

starfishpatkhoo commented 5 months ago

I mistyped my own code, but in so doing, accidentally ended up with this...

http://localhost/elonmusk/profile
    Flight::route("/@user/@module", function(string $module, string $user) {
        var_dump($module);  // elonmusk
        var_dump($user);  // profile
        return false;
    });

I always had the impression that Named Parameters was .. named parameters.. 🤣 But it seems to follow the parameter order instead? Is it a bug or I just misunderstood the documentation?

PS. Please excuse me, I'm very confused by my own code now. 🤣

n0nag0n commented 5 months ago

So you are correct, but these aren't actually named parameters. If you check out the code in https://github.com/flightphp/core/blob/master/flight/Engine.php#L525-L527 it calls $this->dispatcher->execute() which takes the params that were pulled out of the url via regex. It passes those in and eventually calls call_user_func_array() at https://github.com/flightphp/core/blob/master/flight/core/Dispatcher.php#L354 which has no concept of named parameters or what order things should be passed in as.

So I wouldn't call this as much a bug as much as maybe a misunderstanding? The only way I could possibly see this happening is if the framework uses reflection to determine how you called your parameters and what you named your variables and if they match up then it would reorder them (or remove them) from the $params variable. That would be a good size performance hit though it if did that. Reflection is fun, but it ain't cheap :sweat_smile: It would also add complexity cause what if you typed your variable name wrong? What if you only had 1 of the 4 variables in your URL? What about typing and what if you accidentally typed your variable wrong and it kept coming through as an int? (I guess that's a problem right now too...)

Might be worth visiting in the future, I'd have to think about it and if it'd be worth it. It's the reason why the middlewares have an array of params instead of individual params because a grouped middleware might have routes with different numbers of params in their.

krmu commented 5 months ago

+1 for this idea!

starfishpatkhoo commented 5 months ago

While it would be a nice convenience, I don't think it is worth the time and effort on this one.. ^_^ .. dependency injectors are around for a reason... Leave it as parameters in order.. Even if one doesn't use dependency injectors, can add a few extra lines and use a closure to pass to another function in the "appropriate" order..

I just think that usually when I read "Named Parameters", it has a specific meaning. So maybe just need to update the docs and/or give it a new name.. 🤣

fadrian06 commented 4 months ago

@n0nag0n Parameter capture is done with normal groups ( ) or with named groups (?<>) ???

fadrian06 commented 4 months ago

On the one hand, it is true that named parameters are quite confusing if they are actually positional parameters... so we are sorry for the confusion in the documentation.

fadrian06 commented 4 months ago

on the other hand it is true that reordering the parameters based on the url pattern may require reflection which is quite expensive, and to implement more flags in flight, we will end with 40 configuration flags in no time xD

n0nag0n commented 4 months ago

Yeah true....we'll get the documentation corrected and close this issue for now.

n3storm commented 2 months ago

So you are correct, but these aren't actually named parameters. If you check out the code in https://github.com/flightphp/core/blob/master/flight/Engine.php#L525-L527 it calls $this->dispatcher->execute() which takes the params that were pulled out of the url via regex. It passes those in and eventually calls call_user_func_array() at https://github.com/flightphp/core/blob/master/flight/core/Dispatcher.php#L354 which has no concept of named parameters or what order things should be passed in as.

So I wouldn't call this as much a bug as much as maybe a misunderstanding? The only way I could possibly see this happening is if the framework uses reflection to determine how you called your parameters and what you named your variables and if they match up then it would reorder them (or remove them) from the $params variable. That would be a good size performance hit though it if did that. Reflection is fun, but it ain't cheap 😅 It would also add complexity cause what if you typed your variable name wrong? What if you only had 1 of the 4 variables in your URL? What about typing and what if you accidentally typed your variable wrong and it kept coming through as an int? (I guess that's a problem right now too...)

Might be worth visiting in the future, I'd have to think about it and if it'd be worth it. It's the reason why the middlewares have an array of params instead of individual params because a grouped middleware might have routes with different numbers of params in their.

Sorry If I am saying silly things I come from Python :D

Is there a chance to build something like?:

Flight::route("/@user/@module", function(array $params) {
        var_dump($params['module']);  // elonmusk
        var_dump($params['user']);  // profile
        return false;
    });
n0nag0n commented 2 months ago

So, yes technically could do that as you outlined, but in the current context you would be dispatching to a "mini router" It'd be something like:


// routes
Flight::route("/@user/@module", 'Mini_Router->call');

// Mini_Router.php
class Mini_Router {
    public static function call(string $user, string $module, ...$args) {
        if(file_exists('path/to/controllers/'.$user.'.php') === false) {
            throw new Exception('Class not found');
        }

        call_user_func_array([ $user, $module ], $args);
    }
}

Untested but this is the gist of it.