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

Stricter Command arg/option API #17647

Closed
dereuromark opened this issue Mar 31, 2024 · 3 comments · Fixed by #17654
Closed

Stricter Command arg/option API #17647

dereuromark opened this issue Mar 31, 2024 · 3 comments · Fixed by #17654

Comments

@dereuromark
Copy link
Member

dereuromark commented Mar 31, 2024

Description

When upgrading some of the 3.x/4.x code (from shells) into 5.x commands, including

	])->addOption('dry-run', [
		'short' => 'd',
		'help' => 'Dry Run (only output, do not modify INI files).',
		'boolean' => true,
	]);

I noticed some scripts directly executed things.
When I debugged, I saw some

    if (!$args->getArgument('dry-run')) {}

instead of

    if (!$args->getOption('dry-run')) {}

Now this is super dangerous and does not in any way throw a warning/error in code execution, tests or static analyzer (phpstan etc).

I would expect it to say that this option does not exist.
There is a hasOption() and hasArgument() which so far only checks against null, not array key existence.

Since we know the API and the possible keys for those, we could easily check on it and throw a developer exception if used wrongly.

I suggest two options:

  • Hardcode it into inside the getter and throw exception there (5.x)
  • Provide new method hasOptionValue()/hasArgumentValue() with former hasOption()/hasArgument() behavior and use current hasOption()/hasArgument() for this exact check of plausibility (6.x)

or

  • Provide optionExists()/argumentExists() for this check (5.x) and use it inside the getters prior to returning it.
  • Possible alternative naming could be isOptionDefined()/isArgumentDefined()

The former has been done exactly like this in 5.x for EntityTrait and the properties there.

CakePHP Version

5.x

@dereuromark dereuromark added this to the 5.1.0 milestone Mar 31, 2024
@markstory
Copy link
Member

Hardcode it into inside the getter and throw exception there (5.x)

Having Arguments raise errors when getArgument(), getOption() and similar methods seems reasonable to me. If no key/index exists for the argument option, we could raise an error instead of returning null. I think we would want to return any null values as before though because options can be optional.

I don't think we need to change the behavior of hasOption and hasArgument as there isn't a useful distinction between set and null, and unset for command line options. There is no command line string that would result in an option having a useful null as --opt=null is a string, so I don't know why we would need more methods to handle this scenario.

@dereuromark
Copy link
Member Author

I looked into it and it seems it needs a bit more work to get the options sorted.
Arguments are available as argNames list even if not passed, so this can be checked.
But options are only available as keys once actually set/used, otherwise the key does not exist inside Arguments class.

@dereuromark
Copy link
Member Author

dereuromark commented Apr 2, 2024

Based on #17650 I think we can also just check the configured arguments.
Unconfigured or "indexed but configured" arguments are still possible and would not be verifiable either.

But that could be fair enough, since here the mistakes are mainly happening between options and arguments on named side.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants