-
Notifications
You must be signed in to change notification settings - Fork 86
Make command lazy #96
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
Conversation
kunicmarko20
commented
May 4, 2018
Q | A |
---|---|
Bug fix? | yes/no |
New feature? | yes/no |
BC breaks? | yes/no |
Deprecations? | yes/no |
Fixed tickets | #27 |
License | MIT |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can remove name definition now, isn't it?
Not sure we can? we still support symfony 2.8? This is on |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a question.
@@ -7,7 +7,7 @@ | |||
<services> | |||
<service id="fos_ck_editor.command.installer" class="FOS\CKEditorBundle\Command\CKEditorInstallerCommand"> | |||
<argument type="service" id="fos_ck_editor.installer" /> | |||
<tag name="console.command" /> | |||
<tag name="console.command" command="ckeditor:install" /> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
wouldn't that mean, we can omit the name definition in the class now?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not sure, don't we still need it for sf 2.8?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
as any sf version < 3.4 doesn't know how to read command="ckeditor:install"
and will just ignore it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ElectricMaxxx so?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok. Then lets drop it later.
This reverts commit e9cf107.