Skip to content

[Console] End of options (--) signal support #11431

Closed
wants to merge 3 commits into from

5 participants

@Seldaek
Symfony member
Seldaek commented Jul 20, 2014
Q A
Bug fix? yes
New feature? yes
BC breaks? yes-ish
Deprecations? no
Tests pass? yes
Fixed tickets
License MIT
Doc PR

The first commit is only about -- support + fixes a few other little issues I discovered while adding tests to cover my feature.
The second commit makes use of the first in the Application class, so that if you do console foo -- --help for example, it should not trigger the help anymore, but simply have --help available as a dummy arg.

To keep BC, I did this by adding a param to hasParameterOption/getParameterOption instead of doing it by default. This creates another BC issue though in that it changes the InputInterface. I don't think that's a major concern but it's worth mentioning.

The other only minor BC change is if you do getParameterOption() of a flag option i.e. --foo without a value. Before it was returning null in ArgvInput and true in ArrayInput. I normalized this to true for both since I think that makes more sense, but strictly speaking while it improves interface interchangeability, it's a BC break.

@WouterJ
Symfony member
WouterJ commented Jul 20, 2014

Big 👍 for supporting --, however ...

To keep BC, I did this by adding a param to hasParameterOption/getParameterOption instead of doing it by default. This creates another BC issue though in that it changes the InputInterface. I don't think that's a major concern but it's worth mentioning.

Following the bc promise, you are allowed to do this since the InputInterface isn't tagged as public API, but you have to follow: "Should be avoided. When done, this change must be documented in the UPGRADE file."

However, it seems more like a bug that the InputInterface is not tagged as public API, since ,for instance, OutputInterface is. In case this is true (@fabpot?), you are no longer allowed to do this.

The other only minor BC change is if you do getParameterOption() of a flag option i.e. --foo without a value. Before it was returning null in ArgvInput and true in ArrayInput. I normalized this to true for both since I think that makes more sense, but strictly speaking while it improves interface interchangeability, it's a BC break.

Since ArgvInput is part of the public API, you are not allowed to change the return type following the BC promise. Also, changing null to true means every statement using it will fail (since null is a falsey value and true is not).

@Seldaek
Symfony member
Seldaek commented Jul 20, 2014

@WouterJ regarding the interface. If we can't change it, we can just skip those changes on the interface. Since the new param is optional implementors can add that to the interface AFAIK without breaking anything.

As for ArgvInput.. I can make ArrayInput return null if that's preferable. I don't really mind either way, but anyway it's very unlikely anyone called getParameterOption on an option that doesn't take a value, that'd be kinda useless since hasParameterOption tells you if it's there or not already. Not sure how strictly we want to adhere to the book. By the book isn't my specialty so I'll defer to others :)

@TomasVotruba

What's the current state of this?

@Seldaek
Symfony member
Seldaek commented Oct 31, 2015

I am not sure. I think it was ready but sure should be reviewed. /cc @symfony/deciders

@xabbuh
Symfony member
xabbuh commented Nov 1, 2015

I think this is a good idea and since we need to change the interface (there is no other way to solve this, is there?) it would be a good fit for 3.0.

@xabbuh
Symfony member
xabbuh commented Nov 1, 2015

@Seldaek Is it much work to rebase this on master? Would be interesting to if tests still pass.

Seldaek added some commits Jul 20, 2014
@Seldaek Seldaek [Console] Add end of options (--) signal support in inputs 28c8bca
@Seldaek Seldaek [Console] Make use of new end of options support to only parse args w…
…here it makes sense
2483247
@Seldaek
Symfony member
Seldaek commented Nov 2, 2015

Rebased.. It can apply on 2.8 or master.

@xabbuh
Symfony member
xabbuh commented Nov 2, 2015

Something seems to be broken (tests in the FrameworkBundle report not existing arguments).

@Seldaek Seldaek Undo argument parsing rules changes
d9dd8b7
@Seldaek
Symfony member
Seldaek commented Nov 3, 2015

OK build is fixed, travis anyway, appveyor seems unrelated.

@fabpot
Symfony member
fabpot commented Nov 5, 2015

Thank you @Seldaek.

@fabpot fabpot added a commit that referenced this pull request Nov 5, 2015
@fabpot fabpot feature #11431 [Console] End of options (--) signal support (Seldaek)
This PR was squashed before being merged into the 3.0-dev branch (closes #11431).

Discussion
----------

[Console] End of options (--) signal support

| Q             | A
| ------------- | ---
| Bug fix?      | yes
| New feature?  | yes
| BC breaks?    | yes-ish
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets |
| License       | MIT
| Doc PR        |

The first commit is only about -- support + fixes a few other little issues I discovered while adding tests to cover my feature.
The second commit makes use of the first in the Application class, so that if you do `console foo -- --help` for example, it should not trigger the help anymore, but simply have --help available as a dummy arg.

To keep BC, I did this by adding a param to hasParameterOption/getParameterOption instead of doing it by default. This creates another BC issue though in that it changes the InputInterface. I don't think that's a major concern but it's worth mentioning.

The other only minor BC change is if you do getParameterOption() of a flag option i.e. `--foo` without a value. Before it was returning null in ArgvInput and true in ArrayInput. I normalized this to true for both since I think that makes more sense, but strictly speaking while it improves interface interchangeability, it's a BC break.

Commits
-------

834218e [Console] End of options (--) signal support
4acfc3e
@fabpot fabpot closed this Nov 5, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.