Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!

Thank you for the PR! This needs a test to verify any new behavior.

That being said, I'm not totally sure if this is the right approach. If the goal is to accept any field and pass them to wp_update_user, It should be the job of that function to return this error. Unfortunately, after a bit of quick testing it looks like wp_update_user doesn't return an error when you pass it a random value, so I guess relying on that isn't possible : (

If that isn't possible, it seems better to just keep all of the valid options in the DocBlock, not accept arbitrary --field=<value> and use the existing wp cli parameter validation which already has code to handle wrong arguments like this. This happens automatically if you remove this code from the doc bloc:

/**
* --<field>=<value>
* : One or more fields to update. For accepted fields, see wp_update_user().
*
**/

And even better, it gets built in support for similar commands. So if we removed that, you would then see this:

$ wp user update 1 --user-pass=test --user-email=test@example.com
Error: Parameter errors:
 unknown --user-pass parameter
Did you mean '--user_pass'?
 unknown --user-email parameter
Did you mean '--user_email'?

Then all of the fields would be forced to have to have documentation as well, which seems like a benefit. It doesn't feel like it makes sense to keep some of the accepted fields in the DocBlock, then say we accept other arbitrary fields (not documented), but then not accept them based on a second hardcoded list. I think it would be better to just keep them all in the DocBlock and get the advantages above.

Maybe other maintainers have thoughts.

FWIW, this was the original suggestion on the mentioned issue, which wasn‘t addressed here:

Yes, I agree, WP-CLI should verify the flag against a whitelist. This would need to be kept in sync with the WordPress Core version, though, and we need to deal with the changes between Core versions as well.

I'm thinking about an external list of entities and their properties, with starting and/or ending version numbers for each. Having that outside of the individual commands would make it easier to maintain it and check it against new releases.

Some properties were only introduced in a specific WordPress version, or might have been removed again (though that seems unlikely to me, because of back compat), so I get where this is coming from. We should at least discuss the fessibility of this more. NB this goes not only for users but also other entities

I do like the suggestion with the docblock though, because it makes for a much nicer user experience. But we probably can‘t combine that with such a dynamic allowlist…

That said, maybe plugins can add support for more properties too, at which point the allowlist would be too limiting.

Thanks for the feedback, @mrsdizzie @swissspidy !

I've updated the DocBlock to list all accepted fields and removed --= validation, letting WP-CLI handle errors automatically.

Regarding the dynamic allowlist, I see the value in keeping it in sync with WordPress Core, but it could add maintenance overhead and limit plugin flexibility. We should discuss the feasibility of this further—not just for users, but for other entities as well.

Looking forward to more thoughts! 🚀

@swissspidy I agree I'm not 100% convinced it feels right to filter properties here, though it seems the trade off is a worse user experience for wp cli users. I do think ideally this would be the responsibility of the wp user update function or something in core. I am also not sure if plugins can/do change this behavior, but if so it would be another reason not to do this here.

The suggested path in the issue feels complicated to me given the seemingly small amount of changes upstream to what they accept. And it wouldn't really handle the case of plugins being able to alter what is accepted (as you say, no list can).

I think a single line in a docblock that says *WordPress 4.7 and later feels 'good enough' vs an entire abstraction system.

I am also not sure if plugins can/do change this behavior, but if so it would be another reason not to do this here.

I thought maybe via the wp_pre_insert_user_data filter, but couldn't find any plugins doing this.

So yeah, perhaps having a thorough docblock is good enough.

We already list some fields in the docblock, but not all of them. So expanding that list would be helpful either way.

An alternative/additional solution could be to provide suggestions for typos like in the reported --user-pass vs --user_pass use case. Why doesn't this result in an "Did you mean --user_pass" message?

An alternative/additional solution could be to provide suggestions for typos like in the reported --user-pass vs --user_pass use case. Why doesn't this result in an "Did you mean --user_pass" message?

This happens automatically via WP CLI when validating what the user passed against what the command supports (via the DockBlock) but that check never happens when a command has

--<field>=<value>

Because the command accepts anything as valid, so it doesn't run any validation code. Removing that provides the validation / suggestion at the expense of not allowing arbitrary fields anymore -- which is probably what is best in this specific example (and to make sure the command knows and documents all of the accepted values).

Because the command accepts anything as valid, so it doesn't run any validation code.

Ah, I see, that's it. I wonder if we could still do validation but then only emit a warning instead of an error. But then one probably would need a way to ignore the warning...

So yeah, I suppose starting with docblock improvements is the best next step.

Here are all occurrences of --<field>=<value> across all repos btw: https://github.com/search?q=org%3Awp-cli+%22--%3Cfield%3E%3D%3Cvalue%3E%22+path%3Aphp&type=code

and in entity-command alone: https://github.com/search?q=repo%3Awp-cli%2Fentity-command+%22--%3Cfield%3E%3D%3Cvalue%3E%22+language%3APHP&type=code&l=PHP

Lot of room for improvement there IMO.

I think the problem with a warning is that you still have the same initial confusion that wp_update_user will return success. So you'd end up with:

wp user update 1 --user-pass=test
Warning: unknown --user-pass parameter
Did you mean '--user_pass'?
Success: Updated user 1.

It isn't really clear what is happening there -- is the warning wrong? Did it update something? You also don't end up with an error code and wouldn't see the warning if using --quiet (I think?). I think an error would be better for the user.

Valid point 👍

@mrsdizzie - When there is invalid field mentioned, there is only Error message and not a success message. Please have a look at this screenshot.

Yeah I know, but my idea was a warning, in which case you would see both. But let's discard the warning idea :)

I think I'd be fine with more thorough documentation in favor of the dynamic catch-all --<field>=<value> bit. But we would need to do this for all commands, not just users.

@swissspidy To address our main issue and improve parameter validation across WP-CLI commands, these are the changes proposed. Can I proceed with these?

Remove --= syntax from the DocBlock of wp user update ( which is already done ) and replace it with listed parameters (e.g., --user_pass, --user_email, etc.).

Extend this pattern to other commands (e.g., post update, term update) in follow-up PRs, replacing dynamic field syntax with documented parameters.

@karthick-murugan To be honest I'd like to think about this a moment and hold off on this PR and more PRs related to this feature -- these PRs often need almost every line changed based on review and it can feel like more work than just doing it. I think there is an idea / path forward we can take from here and probably update everything in a larger PR.

If you want to work on wp-cli/wp-cli#6066 which is waiting on response instead.

@mrsdizzie - I have added a video here in wp-cli/wp-cli#6025 mentioning that I was not able to reproduce the issue mentioned.

As per #528 (comment) I am closing this for now, in favor of a more thorough discussion/approach for this scenario. Thanks again for your PR!