Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Parameters for "daux serve" mode have no effect #171

Closed
schakko opened this issue Nov 21, 2019 · 8 comments
Closed

Parameters for "daux serve" mode have no effect #171

schakko opened this issue Nov 21, 2019 · 8 comments
Labels
bug Waiting for release fixed, waiting for a release

Comments

@schakko
Copy link

schakko commented Nov 21, 2019

Describe the bug
None of the command line parameters for "daux serve" are not reflected when using the live mode.

To Reproduce
Steps to reproduce the behavior:

a)

  1. Execute daux serve --processor=InvalidProcessor
  2. No error is printed that InvalidProcessor does not exist.

b)

  1. Execute daux serve --processor=ValidProcessor
  2. ValidProcessor is not used.

c)

  1. Execute daux serve --verbose
  2. No verbose mode is enabled

Expected behavior
See above.

Additional context
The problem resides in the way Daux is calling itself. When using daux serve, Symfony is used with its command line arguments but they are not reflected back when using PHP's passthru method in Console\DauxCommand.php. I've changed the file accordingly so we could theoretically reuse the command line arguments:

        # GH: strip "serve" command from command line arguments
        $arguments = $input->__toString();
        $arguments = str_replace(" serve", "", $arguments);
				
        putenv('DAUX_SOURCE=' . $daux->getParams()->getDocumentationDirectory());
        putenv('DAUX_THEME=' . $daux->getParams()->getThemesPath());
        putenv('DAUX_CONFIGURATION=' . $daux->getParams()->getConfigurationOverrideFile());
        putenv('DAUX_EXTENSION=' . DAUX_EXTENSION);

        $base = escapeshellarg(__DIR__ . '/../../');
        $binary = escapeshellarg((new PhpExecutableFinder)->find(false));

        echo "Daux development server started on http://{$host}:{$port}/\n";
		
        # GH: append arguments
        passthru("{$binary} -S {$host}:{$port} {$base}/index.php {$arguments}");

Nevertheless, the arguments are not taken into credit as index.php does not use any of Symfony's application or command line context but directly instantiates the Daux class by calling

require_once __DIR__ . '/libs/bootstrap.php';

\Todaymade\Daux\Server\Server::serve();

The situation can be improved in a few different ways:

  1. (Quick fix): Remove the unused command line parameters like verbose and processor from the serve command
  2. (Quick fix): Update https://daux.io/For_Developers/Creating_a_Processor.html to describe that the processor argument can only be used with daux generate OR by providing the class name inside the config.json file
  3. Refactor the index.php so that Daux is instantiated by Symfony based upon the command line parameters provided. If it is not running in SAPI/CLI mode but in a webserver environment, don't provide any parameters but fall back to the config.json.
@onigoetz
Copy link
Contributor

Hello, thanks for the detailed report. it is indeed a problem that the processor is not taken in account through the command line.

If I may recommend, try to use the processor through the config.json as it is read in both the generate and serve commands.

For the verbose argument though, it will probably not be as easy, as an echo from the command will be output to the web page in the serve mode. I will see how we can make sure the output goes to stdout.

@onigoetz
Copy link
Contributor

I have looked into it and I think we can't solve this with a quick fix.

As a first step I've made it possible to have verbose logging from serve but I don't want to just add the support for --processor I want all possible arguments to be supported.

For this I will need a bit more time. maybe I'll write a configuration file to a temporary path and simplify the configuration loading logic.

@schakko
Copy link
Author

schakko commented Nov 27, 2019

@onigoetz Thank you for the reply and don't rush into any fast decisions ;-)
I'd had fixed the issue by myself but with the current architecture it's relatively extensive to implement a working and clean solution.

My 2 cents:

  • Use Symfony's DI system
  • Create a builder class for the Daux instance to prevent duplicate code for instantiating Daux and evaluating the parameters
  • Remove the width constructor parameter from Todaymade\Daux\Processor
  • Refactor index.php so Symfony's application is started
  • Pass all parameters from the command line given by daux serve into a DAUX_COMMAND_LINE_PARAMETERS environment variable and write a simple wrapper for Symfony command line options so it can take a single string as argument

@onigoetz
Copy link
Contributor

Interesting, thanks for the ideas ! I'll definitely look into that :)

One thing to keep in mind though is that in serve mode, it has to parse the whole docs folder in order to make every single page. so I want that to be as fast as possible.

@schakko
Copy link
Author

schakko commented Nov 27, 2019

@onigoetz Just to give you some feedback regarding the performance. https://active-directory-wp.com/docs has around ~130 files and larger docs and daux work in serve mode without any performance problems.

onigoetz added a commit that referenced this issue Dec 5, 2019
@onigoetz onigoetz added the Waiting for release fixed, waiting for a release label Dec 5, 2019
@onigoetz
Copy link
Contributor

onigoetz commented Dec 5, 2019

So I made some changes that handles the configuration layer better, and when using the serve command creates a serialized configuration that is read by the index.php, thus skipping this on each subsequent request.

This makes all command line settings supported in the serve mode.

I looked into Symfony's application but that seemed even more work to get it right.

@schakko
Copy link
Author

schakko commented Dec 6, 2019

@onigoetz I really appreciate the effort you have put into fixing this issue and looking forward to a new release :-)

@onigoetz
Copy link
Contributor

The fixes discussed in this issue are available in version 0.12

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Waiting for release fixed, waiting for a release
Projects
None yet
Development

No branches or pull requests

2 participants