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

Enable --format=<format> for db search #247

Merged
merged 5 commits into from
Apr 27, 2024

Conversation

2ndkauboy
Copy link
Contributor

@2ndkauboy 2ndkauboy commented Nov 10, 2023

Closes #158

Related wp-cli/wp-cli#5859

@2ndkauboy 2ndkauboy requested a review from a team as a code owner November 10, 2023 18:08
@2ndkauboy 2ndkauboy force-pushed the fix/158-enable-formatter branch 2 times, most recently from 6d4b69d to ab28315 Compare November 10, 2023 19:55
src/DB_Command.php Outdated Show resolved Hide resolved
src/DB_Command.php Outdated Show resolved Hide resolved
src/DB_Command.php Outdated Show resolved Hide resolved
@swissspidy
Copy link
Member

cc @johnbillion since you were involved in the discussion about this, maybe you'd like to take a look as well

Copy link
Member

@danielbachhuber danielbachhuber left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It'd be great to have a more detailed pull request description that describes what this pull request does, and how it changes the nature of the command.

@@ -1208,10 +1208,10 @@ public function prefix() {
* : Output the 'table:column' line once before all matching row lines in the table column rather than before each matching row.
*
* [--one_line]
* : Place the 'table:column' output on the same line as the row id and match ('table:column:id:match'). Overrides --table_column_once.
* : Deprecated: use `--format` instead. Place the 'table:column' output on the same line as the row id and match ('table:column:id:match'). Overrides --table_column_once.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What argument should they use with --format?

*
* [--matches_only]
* : Only output the string matches (including context). No 'table:column's or row ids are outputted.
* : Deprecated: use `--format` instead. Only output the string matches (including context). No 'table:column's or row ids are outputted.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What argument should they use with --format?

* : Get a specific subset of the fields.
*
* [--format=<format>]
* : Render output in a particular format.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should document supported --format=<format> options with YAML:

[--format=<format>]
---
options:
  - TK
---

@@ -1225,6 +1225,12 @@ public function prefix() {
* [--match_color=<color_code>]
* : Percent color code to use for the match (unless both before and after context are 0, when no color code is used). For a list of available percent color codes, see below. Default '%3%k' (black on a mustard background).
*
* [--fields=<fields>]
* : Get a specific subset of the fields.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What fields are supported here?

'table' => $table,
'column' => $column,
'key' => $primary_key,
'ID' => $result->$primary_key,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this be 'value' instead of 'ID'? It seems a little bit weird to call this 'ID' when the primary key column could be called something else.

@danielbachhuber
Copy link
Member

@2ndkauboy Are you planning to continue working on this, or should we close the PR for now?

@2ndkauboy
Copy link
Contributor Author

@danielbachhuber i was hoping to get feedback from @johnbillion about this new feature, since he opened the issue.

@johnbillion
Copy link
Contributor

I'll take a look!

@johnbillion
Copy link
Contributor

@2ndkauboy I agree that introducing a --format parameter here is a good idea. Daniel left some good feedback and I agree with all his points. Is this something you'd like to continue with?

@2ndkauboy
Copy link
Contributor Author

Daniel left some good feedback and I agree with all his points. Is this something you'd like to continue with?

So you would also rename the ID to value?

@2ndkauboy
Copy link
Contributor Author

Would people understand that value is for the key column?

+------------+--------------+-----------+-------+-----------------------------+
| table      | column       | key       | value | match                       |
+------------+--------------+-----------+-------+-----------------------------+
| wp_options | option_value | option_id | 1     | https://localhost:8889      |
| wp_options | option_value | option_id | 2     | https://localhost:8889      |
| wp_posts   | guid         | ID        | 1     | https://localhost:8889/?p=1 |
| wp_users   | user_url     | ID        | 1     | https://localhost:8889      |
+------------+--------------+-----------+-------+-----------------------------+

@johnbillion
Copy link
Contributor

What's the command for that output?

@2ndkauboy
Copy link
Contributor Author

The command for the output above would be:

wp db search https://localhost:8889 --format=table

@johnbillion
Copy link
Contributor

I think value is better than ID but overall that table isn't particularly clear.

Perhaps the order of the columns should change to: column, match, key, value.

If we didn't need to support output from multiple database tables we could skip the key column completely and use the key as the name for the value column.

+------------+--------------+-----------+-----------------------------+
| table      | column       | option_id | match                       |
+------------+--------------+-----------+-----------------------------+
| wp_options | option_value | 2         | https://localhost:8889      |
+------------+--------------+-----------+-----------------------------+
+------------+--------------+-------+-----------------------------+
| table      | column       | ID    | match                       |
+------------+--------------+-------+-----------------------------+
| wp_posts   | guid         | 1     | https://localhost:8889/?p=1 |
+------------+--------------+-------+-----------------------------+

@2ndkauboy
Copy link
Contributor Author

If we didn't need to support output from multiple database tables we could skip the key column completely and use the key as the name for the value column.

By default, the wp db search command searches in all tables and outputs something like this:

wp_comments:comment_author_email
244936:[email protected]
wp_options:option_value
14:mail.example.com
wp_options:option_value
15:[email protected]

So this includes: table, column, primary key value, match.

This is why decided to have all these columns as well. But then the "primary key value" column needed a headline/name, and since it could be different column names, I added that key column as well.

If we allow searching all tables, which the command does by default, we cannot use the "primary key name" as the headline/name, since it's different names. We could just not have that column, but then we need another headline. Maybe something like pk? 🤔

@johnbillion
Copy link
Contributor

Yeah. Perhaps primary_key_name and primary_key_value? It's very verbose but it's clear.

@2ndkauboy
Copy link
Contributor Author

I thought about that as well. How about pk_column or pk_key_name and pk_value?

@2ndkauboy
Copy link
Contributor Author

2ndkauboy commented Apr 26, 2024

@johnbillion I've simulated some variant, which one would you pick?

Only the primary key value

1. Column pk

+------------+--------------+----+-----------------------------+
| table      | column       | pk | match                       |
+------------+--------------+----+-----------------------------+
| wp_options | option_value | 1  | https://localhost:8889      |
| wp_options | option_value | 2  | https://localhost:8889      |
| wp_posts   | guid         | 1  | https://localhost:8889/?p=1 |
| wp_users   | user_url     | 1  | https://localhost:8889      |
+------------+--------------+----+-----------------------------+

2. Column value

+------------+--------------+-----+-----------------------------+
| table      | column       | key | match                       |
+------------+--------------+-----+-----------------------------+
| wp_options | option_value | 1   | https://localhost:8889      |
| wp_options | option_value | 2   | https://localhost:8889      |
| wp_posts   | guid         | 1   | https://localhost:8889/?p=1 |
| wp_users   | user_url     | 1   | https://localhost:8889      |
+------------+--------------+-----+-----------------------------+

3. Column primary_key_value

+------------+--------------+-------------------+-----------------------------+
| table      | column       | primary_key_value | match                       |
+------------+--------------+-------------------+-----------------------------+
| wp_options | option_value | 1                 | https://localhost:8889      |
| wp_options | option_value | 2                 | https://localhost:8889      |
| wp_posts   | guid         | 1                 | https://localhost:8889/?p=1 |
| wp_users   | user_url     | 1                 | https://localhost:8889      |
+------------+--------------+-------------------+-----------------------------+

Primary key column name and value

4. Columns key and value

+------------+--------------+-----------+-------+-----------------------------+
| table      | column       | key       | value | match                       |
+------------+--------------+-----------+-------+-----------------------------+
| wp_options | option_value | option_id | 1     | https://localhost:8889      |
| wp_options | option_value | option_id | 2     | https://localhost:8889      |
| wp_posts   | guid         | ID        | 1     | https://localhost:8889/?p=1 |
| wp_users   | user_url     | ID        | 1     | https://localhost:8889      |
+------------+--------------+-----------+-------+-----------------------------+

5. Columns pk_column and pk_value

+------------+--------------+-----------+----------+-----------------------------+
| table      | column       | pk_column | pk_value | match                       |
+------------+--------------+-----------+----------+-----------------------------+
| wp_options | option_value | option_id | 1        | https://localhost:8889      |
| wp_options | option_value | option_id | 2        | https://localhost:8889      |
| wp_posts   | guid         | ID        | 1        | https://localhost:8889/?p=1 |
| wp_users   | user_url     | ID        | 1        | https://localhost:8889      |
+------------+--------------+-----------+----------+-----------------------------+

6. Columns pk_key_name and pk_value

+------------+--------------+-------------+----------+-----------------------------+
| table      | column       | pk_key_name | pk_value | match                       |
+------------+--------------+-------------+----------+-----------------------------+
| wp_options | option_value | option_id   | 1        | https://localhost:8889      |
| wp_options | option_value | option_id   | 2        | https://localhost:8889      |
| wp_posts   | guid         | ID          | 1        | https://localhost:8889/?p=1 |
| wp_users   | user_url     | ID          | 1        | https://localhost:8889      |
+------------+--------------+-------------+----------+-----------------------------+

7. Columns priamry_key_*

+------------+--------------+------------------+-------------------+-----------------------------+
| table      | column       | priamry_key_name | priamry_key_value | match                       |
+------------+--------------+------------------+-------------------+-----------------------------+
| wp_options | option_value | option_id        | 1                 | https://localhost:8889      |
| wp_options | option_value | option_id        | 2                 | https://localhost:8889      |
| wp_posts   | guid         | ID               | 1                 | https://localhost:8889/?p=1 |
| wp_users   | user_url     | ID               | 1                 | https://localhost:8889      |
+------------+--------------+------------------+-------------------+-----------------------------+

Different order

8. Columns pk_key_* and match_*

+------------+-------------+----------+--------------+-----------------------------+
| table      | pk_key_name | pk_value | match_column | match_value                 |
+------------+-------------+----------+--------------+-----------------------------+
| wp_options | option_id   | 1        | option_value | https://localhost:8889      |
| wp_options | option_id   | 2        | option_value | https://localhost:8889      |
| wp_posts   | ID          | 1        | guid         | https://localhost:8889/?p=1 |
| wp_users   | ID          | 1        | user_url     | https://localhost:8889      |
+------------+-------------+----------+--------------+-----------------------------+

9. Columns obj_id_* and match_*

+------------+------------+--------+--------------+-----------------------------+
| table      | obj_id_key | obj_id | match_column | match_value                 |
+------------+------------+--------+--------------+-----------------------------+
| wp_options | option_id  | 1      | option_value | https://localhost:8889      |
| wp_options | option_id  | 2      | option_value | https://localhost:8889      |
| wp_posts   | ID         | 1      | guid         | https://localhost:8889/?p=1 |
| wp_users   | ID         | 1      | user_url     | https://localhost:8889      |
+------------+------------+--------+--------------+-----------------------------+

@danielbachhuber
Copy link
Member

Let's go with this:

+------------+--------------+-----------+------------------+-----------------------------+
| table      | column       | match     | primary_key_name | primary_key_value           |
+------------+--------------+-----------+------------------+-----------------------------+

@2ndkauboy
Copy link
Contributor Author

The result would look like this now:

$wp db search admin --format=table
+-------------+---------------+-----------------------------------------------------------------------------------------------+------------------+-------------------+
| table       | column        | match                                                                                         | primary_key_name | primary_key_value |
+-------------+---------------+-----------------------------------------------------------------------------------------------+------------------+-------------------+
| wp_options  | option_name   | admin_email                                                                                   | option_id        | 6                 |
| wp_options  | option_name   | admin_email_lifespan                                                                          | option_id        | 280               |
| wp_options  | option_value  | a:5:{s:13:"administrator";a:2:{s:4:"name";s:13:"Administrator";s:12:"capabilities";a:61:{s:13 | option_id        | 89                |
| wp_options  | option_value  | e notification of site activity via the admin toolbar and your Mobile devices.";s:4:"         | option_id        | 164               |
| wp_usermeta | meta_key      | admin_color                                                                                   | umeta_id         | 7                 |
| wp_usermeta | meta_key      | show_admin_bar_front                                                                          | umeta_id         | 9                 |
| wp_usermeta | meta_value    | admin                                                                                         | umeta_id         | 1                 |
| wp_usermeta | meta_value    | a:1:{s:13:"administrator";b:1;}                                                               | umeta_id         | 10                |
| wp_users    | user_login    | admin                                                                                         | ID               | 1                 |
| wp_users    | user_nicename | admin                                                                                         | ID               | 1                 |
| wp_users    | display_name  | admin                                                                                         | ID               | 1                 |
+-------------+---------------+-----------------------------------------------------------------------------------------------+------------------+-------------------+

@danielbachhuber
Copy link
Member

@2ndkauboy I think that looks good. What do you think?

@2ndkauboy
Copy link
Contributor Author

Let's merge it then :) After my updated tests run ;)

@danielbachhuber danielbachhuber changed the title Enable formatter for search command Enable --format=<format> for db search Apr 27, 2024
Copy link
Member

@danielbachhuber danielbachhuber left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for your work on this, @2ndkauboy !

@danielbachhuber danielbachhuber added this to the 2.0.28 milestone Apr 27, 2024
@danielbachhuber danielbachhuber merged commit bf741eb into wp-cli:main Apr 27, 2024
27 of 38 checks passed
@2ndkauboy 2ndkauboy deleted the fix/158-enable-formatter branch April 27, 2024 08:50
@2ndkauboy
Copy link
Contributor Author

2ndkauboy commented Apr 29, 2024

Updated at WP-CLI Hack Day April 2024 #5935

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

Successfully merging this pull request may close these issues.

Output only matching IDs in db search
4 participants