flightphp / runway

Console app for the Flight PHP Framework.
https://docs.flightphp.com/awesome-plugins/runway
MIT License
2 stars 2 forks source link

Not loading commands from app_root folder #3

Open carrer opened 1 month ago

carrer commented 1 month ago

Commands from /app/commands aren't being loaded. The "app_root" property of runaway-config.json is not being taken into consideration for loading up command files.

n0nag0n commented 1 month ago

So I'm curious how your project is setup? What runway does is it scans all these directories to try and find where your commands are. Let me know which one you are expecting it to be in and maybe I can help from there.

markhughes commented 1 month ago

@n0nag0n I think the issue is that in those paths, it doesn't consider app_root from $config, it just checks DIR etc. Could we have it use that value?

I would also consider the wording to take on "command" and "commands" as some project use singular phrasing :)

n0nag0n commented 1 month ago

So app root is actually specified in each of the commands that you want to run. For example in the ControllerCommand in the core repo, you can see that it's used here. I didn't include it in the base because some commands don't need the app root. https://github.com/flightphp/core/blob/master/flight/commands/ControllerCommand.php#L32-L41

n0nag0n commented 1 month ago

And with the comment about command vs commands, the other directories in the skeleton are all plural for scaling reasons. Yes you may only have one command in there, but as projects grow, you'll likely add some additional commands and as soon as you do, you'll now have the command/ folder with multiple commands in there. If you want to refactor it, and depending on how you setup your namespaces and such, you could unintentionally introduce a bug into your project. I would just leave it plural and wait for your project to become so awesome that you plan on making another command in the future!

markhughes commented 1 month ago

So, it is misleading as "app root" sounds like where all the controllers, commands, services, etc, are located.

I think the problem here is: There is no way to specify the commands directory.

The solution could be:

  1. Use app_root (like my PR)
  2. Add another option to the configuration
n0nag0n commented 1 month ago

I guess it would help me to understand a) where you are running php runway (like your project root or from a random place in your server/vm/environment) and b) where your app root is and what it's defined as. The code I shared earlier, specifically https://github.com/flightphp/runway/blob/master/runway#L30 would hit your commands folder just fine if your commands are in projectroot/app/commands/. The only variable is the current working directory changes depending on where you run the runway command.

app_root is specifically for actions like creating new controllers, active record models, etc.

I suppose I could see the error if you are trying to run php vendor/bin/runway and you're trying to hit your app directory. I think that might need to be changed in the paths to search.

Btw, I saw your PR, and it could be added, but I would need to make some changes because as of right now it would throw notices if $config['app_root'] happened to not be defined in a project.

n0nag0n commented 1 month ago

I had some time tonight, what do you think of this pull? Should include everything that you were talking about https://github.com/flightphp/runway/pull/5

markhughes commented 1 month ago

I haven't had a chance to test it out but from a read over it looks good to me!

carrer commented 1 month ago

Hey guys, sorry for the late reply.

What I did basically was to create a new project using composer create-project flightphp/skeleton cool-project-name . You'll notice the "commands" folder is created inside the "app" subdirectory. Then I started to create my commands inside that folder and noticed they didn't work as expected, even though .runway-config.json had the right app_root attribute. In my case, I did the following modification to the runway.php (mostly because my commands have different namespace then flight\commands: $addedCommands = []; foreach($paths as $path) { foreach(glob($path) as $commandPath) { if(basename($commandPath) === 'AbstractBaseCommand.php') { continue; }

if (preg_match('/Command.php$/', $commandPath) === 0) { continue; } // $command = str_replace('.php', '', basename($commandPath)); // $command = 'flight\commands\'.$command; $namespace = preg_match('/namespace (.*);/', file_get_contents($commandPath), $matches) ? $matches[1] : 'flight\commands';

$command = $namespace.'\'.str_replace('.php', '', basename($commandPath)); if(in_array($command, $addedCommands, true) === true) { continue; } $addedCommands[] = $command; require_once $commandPath; $consoleApp->add(new $command($config)); } }

I also added $cwd.'/app/commands/*.php' into $paths array (as I was in a hurry), but the folder coming from "app_root" attribute from runway-config.json should also be considered. On Fri, 27 Sept 2024 at 03:51, Mark Hughes @.***> wrote:

I haven't had a chance to test it out but from a read over it looks good to me!

— Reply to this email directly, view it on GitHub https://github.com/flightphp/runway/issues/3#issuecomment-2378248283, or unsubscribe https://github.com/notifications/unsubscribe-auth/ABRHMAVUEMPFAIPVYPQLEJDZYS2ZBAVCNFSM6AAAAABOHQ47YCVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDGNZYGI2DQMRYGM . You are receiving this because you authored the thread.Message ID: @.***>

n0nag0n commented 1 month ago

ahh the OG speaks! This is helpful. So with the changes I've done this should also help out. But you do bring up a point that other namespaces need to be considered as well.

I added the ability for it to dynamically find the namespaces and added a while bunch of config options you can just add in via the config file now.

https://github.com/flightphp/runway/releases/tag/v1.1.0

Let me know if this fixes your issues.