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

Ignore the --no-debug option when creating the console #626

Merged
merged 5 commits into from Aug 15, 2019

Conversation

@aschempp
Copy link
Contributor

commented Aug 9, 2019

Correctly fixes #617

@aschempp aschempp added the defect label Aug 9, 2019

@aschempp aschempp added this to the 4.8 milestone Aug 9, 2019

@Toflar

Toflar approved these changes Aug 9, 2019

@leofeyer
Copy link
Member

left a comment

I do not like the longish method names createFromConsoleInput() and createFromRequest(). Can we have one ContaoKernel::create() method that accepts either a request object or an input object and handles both accordingly?

@aschempp

This comment has been minimized.

Copy link
Contributor Author

commented Aug 10, 2019

I don‘t think thats a good idea. There would be no type hinting and no IDE support?

@aschempp

This comment has been minimized.

Copy link
Contributor Author

commented Aug 12, 2019

Btw. we could shorten the names to fromInput and fromRequest. I see that Symfony does similarly: https://github.com/symfony/symfony/blob/4.4/src/Symfony/Component/Process/Process.php#L191

@leofeyer

This comment has been minimized.

Copy link
Member

commented Aug 12, 2019

Btw. we could shorten the names to fromInput and fromRequest.

I like this. 👍

@aschempp

This comment has been minimized.

Copy link
Contributor Author

commented Aug 13, 2019

Changed in 5efb653

@leofeyer

This comment has been minimized.

Copy link
Member

commented on manager-bundle/src/Console/ContaoApplication.php in db76b90 Aug 14, 2019

Why this?

This comment has been minimized.

Copy link
Contributor Author

replied Aug 14, 2019

just because… it will always return that type, because we require that type in the constructor.

This comment has been minimized.

Copy link
Member

replied Aug 14, 2019

Nice.

aschempp and others added some commits Aug 14, 2019

@leofeyer leofeyer merged commit b7a50fe into 4.8 Aug 15, 2019

4 of 5 checks passed

Travis CI - Pull Request Build Failed
Details
Travis CI - Branch Build Passed
Details
continuous-integration/appveyor/branch AppVeyor build succeeded
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
coverage/coveralls Coverage increased (+0.6%) to 86.701%
Details

@leofeyer leofeyer deleted the bugfix/debug-argument branch Aug 15, 2019

@leofeyer

This comment has been minimized.

Copy link
Member

commented Aug 15, 2019

Thank you @aschempp.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.