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

Cli App Injection #18878

Merged
merged 3 commits into from
Nov 27, 2017
Merged

Cli App Injection #18878

merged 3 commits into from
Nov 27, 2017

Conversation

wilsonge
Copy link
Contributor

Summary of Changes

Fixes 2 of the 3 remaining unit tests by allowing JInputCli objects to be injected into the \Joomla\CMS\Application\CliApplication class

Testing

Check unit tests pass + code review

@mbabker
Copy link
Contributor

mbabker commented Nov 26, 2017

Well, it can work, but you wouldn't be able to parse the CLI arguments out without doing $this->input->cli->get(). For me it works, I just wonder if the extra check is really needed.

@wilsonge
Copy link
Contributor Author

I thought it was better safe than sorry to stop stupid. But in theory I guess it should be fine

@mbabker
Copy link
Contributor

mbabker commented Nov 27, 2017

Let's be honest here. We're dealing with fully custom CLI architecture at this point. I think there's less to be defensive about here.

@wilsonge
Copy link
Contributor Author

okies :) removed

@mbabker mbabker merged commit cb08754 into joomla:4.0-dev Nov 27, 2017
@wilsonge wilsonge deleted the cli-app-injection branch November 27, 2017 00:47
@zero-24 zero-24 added this to the Joomla 4.0 milestone Nov 29, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants