Skip to content

[CLI] Add WordPress constants using CLI arguments #1811

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

Closed
wants to merge 337 commits into from

Conversation

bgrgicak
Copy link
Collaborator

Motivation for the change, related issues

I was testing #1789 and added support for defining constants to the CLI during development.

If feels like a useful feature, so I want to include it in the CLI.

Implementation details

A new --constants argument enables users to add WordPress constants to Playground.

Testing Instructions (or ideally a Blueprint)

  • Run Playground bun ./packages/playground/cli/src/cli.ts server --constants PLAYGROUND_FORCE_AUTO_LOGIN_ENABLED=true
  • Open this URL
  • Confirm that you are logged in

adamziel and others added 30 commits September 14, 2024 10:20
It confused me to see a type like:
true | 'not-available' | 'origin-mismatch'

All values are truthy so you have to know to compare
directly with true. And at that point, it's clearer to use
the string literal "available" instead.
Initially, we do this by writing one file at a time in Safari,
but we can try doing Safari writes in batches in a subsequent commit.
If we don't do this, a quick error-retry scenario could run into cases
where the second attempt conflicts with ongoing writes from the
first attempt.
@adamziel
Copy link
Collaborator

adamziel commented Oct 7, 2024

Would the same make sense for the Query API?

@bgrgicak
Copy link
Collaborator Author

bgrgicak commented Oct 8, 2024

Would the same make sense for the Query API?

Adding features to the query API always sounds like an improvement, but at the same time, is it needed? 🤔

Let's add it. The Query API is great for quick edits to your site and with all other APIs supporting it, let's not leave the query API behind.

How would the format look?

A few things that come to my mind:

?constant=DISABLE_WP_CRON=true
?constants={DISABLE_WP_CRON: true}
?constant-DISABLE_WP_CRON=true

@bgrgicak
Copy link
Collaborator Author

bgrgicak commented Oct 8, 2024

This will also allow us to store database credentials in the URL. 🤣

@adamziel
Copy link
Collaborator

adamziel commented Oct 8, 2024

Would the same make sense for the
How would the format look?

A few things that come to my mind:


?constant=DISABLE_WP_CRON=true

?constants={DISABLE_WP_CRON: true}

?constant-DISABLE_WP_CRON=true

Let's use the same format as in CLI, I'd like the CLI and the query API to eventually be handled by the same function.

@bgrgicak
Copy link
Collaborator Author

@adamziel I'm still not sure how to do that.

This is how it looks like the CLI bun ./packages/playground/cli/src/cli.ts server --constants PLAYGROUND_FORCE_AUTO_LOGIN_ENABLED=true WP_DEBUG_DISPLAY=true

One option could be
?constants=PLAYGROUND_FORCE_AUTO_LOGIN_ENABLED=true&constants=WP_DEBUG_DISPLAY=true

Or we could use only one argument and have a delimiter, but I'm worried that the delimiter could cause issues if it appears in the constant value.
?constants=PLAYGROUND_FORCE_AUTO_LOGIN_ENABLED=true,WP_DEBUG_DISPLAY=true

@bgrgicak bgrgicak changed the base branch from fix/1706-login-with-site-email-verification to trunk October 15, 2024 07:36
@adamziel
Copy link
Collaborator

This could be similar to ENV variables:

// cli:
bun ./packages/playground/cli/src/cli.ts server --constant PLAYGROUND_FORCE_AUTO_LOGIN_ENABLED=true --constant WP_DEBUG_DISPLAY=true

This would be consistent with how you can specify multiple --plugin options in CLI and query API

However, this gets a bit weird in the query API:

// query API:
?constant=WP_DEBUG_DISPLAY=true

So maybe...

bun ./packages/playground/cli/src/cli.ts server --constant[PLAYGROUND_FORCE_AUTO_LOGIN_ENABLED]=true
?constant[WP_DEBUG_DISPLAY]=true

I'm having second thoughts here, though. PHP itself has no CLI option for defining constants, only .ini entries. There may be a good reason for that. When true is specified as that constant's value, is it a boolean true or is it a string "true"? Same for numbers. We'd have to make a choice, and there will be cases where the API consumer would expect that other choice. Perhaps requiring a Blueprint for this would be for the best? cc @brandonpayton for thoughts

@bgrgicak
Copy link
Collaborator Author

Perhaps requiring a Blueprint for this would be for the best?

That sounds ok to me, we need to enable it and blueprints do the job.

CLI and Query arguments should be available for the things that are modified frequently. Are constants something people frequently modify?

@adamziel
Copy link
Collaborator

Are constants something people frequently modify?

I'd guess WP_DEBUG et al. are the most frequently modified constants, we could have --wp-debug=yes and ?wp_debug=yes for that.

@bgrgicak
Copy link
Collaborator Author

I'd guess WP_DEBUG et al. are the most frequently modified constants, we could have --wp-debug=yes and ?wp_debug=yes for that.

That one is true by default 🙂

I'm closing this PR as it seems like we don't need a dedicated constants argument. We can reopen it if we change our minds in the future.

@bgrgicak bgrgicak closed this Oct 16, 2024
@brandonpayton
Copy link
Member

I'm having second thoughts here, though. PHP itself has no CLI option for defining constants, only .ini entries. There may be a good reason for that. When true is specified as that constant's value, is it a boolean true or is it a string "true"? Same for numbers. We'd have to make a choice, and there will be cases where the API consumer would expect that other choice. Perhaps requiring a Blueprint for this would be for the best? cc @brandonpayton for thoughts

@adamziel, even though we're not doing this, I'll offer some thoughts:
I think this seems like a fine feature. I was also uncomfortable with the ambiguity of some types but then thought maybe PHP's natural, implicit type conversions might make it usable enough. But I'm sure it would bite people.

Maybe if we reconsider this feature, we could make our best guess with --constant values and also offer explicit variants like --constant_bool, --constant_string, etc.

@adamziel
Copy link
Collaborator

adamziel commented Oct 16, 2024

Oh the suffix sounds nice.

Thinking out loud, I wonder how much value would that add over a simple Blueprint. Once we can merge Blueprints that could be a more practical way vs having an option representing each step. Although CLI flags are natural when working with CLI programs

@brandonpayton
Copy link
Member

Thinking out loud, I wonder how much value would that add over a simple Blueprint. Once we can merge Blueprints that could be a more practical way vs having an option representing each step. Although CLI flags are natural when working with CLI programs

👍 That seems like good thinking -- Leverage the more expansive capabilities in Blueprint rather than expanding the CLI flags.

adamziel pushed a commit that referenced this pull request Nov 5, 2024
This PR returns support for login in the CLI using a `login` argument. 

The support was previously removed in #1811.

## Testing Instructions (or ideally a Blueprint)

- Run `bun ./packages/playground/cli/src/cli.ts server --login`
- Open Playground and see that you are logged in
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

5 participants