Skip to content

Display metadata in the table#1462

Open
akirk wants to merge 9 commits into
xwp:developfrom
akirk:meta-field-display
Open

Display metadata in the table#1462
akirk wants to merge 9 commits into
xwp:developfrom
akirk:meta-field-display

Conversation

@akirk
Copy link
Copy Markdown

@akirk akirk commented Oct 11, 2023

Fixes #947.

The metadata that can be stored along side records is burried in the stream_meta table. This make the data accessible in the table.

metadata-display
Screenshot 2026-05-24 at 04 34 29

Checklist

  • I have tested the changes in the local development environment (see contributing.md).
  • I have added phpunit tests.

I have run the code. I am unable to run the local development environment on a recent Macbook Pro. Maybe the environment should be updated in a separate PR.

Error: Cask virtualbox depends on hardware architecture being one of [{:type=>:intel, :bits=>64}], but you are running {:type=>:arm, :bits=>64}.

Thus, I have also not been able to check the unit tests yet.

Release Changelog

  • New: Expose stream metadata in the admin UI.

Release Checklist

  • This pull request is to the master branch.
  • Release version follows semantic versioning. Does it include breaking changes?
  • Update changelog in readme.txt.
  • Bump version in stream.php.
  • Bump Stable tag in readme.txt.
  • Bump version in classes/class-plugin.php.
  • Draft a release on GitHub.

Change [ ] to [x] to mark the items as done.

@akirk
Copy link
Copy Markdown
Author

akirk commented Feb 8, 2024

@kasparsd any chance of getting this merged? I've rebased this on the current release, it still works.

@hokoo
Copy link
Copy Markdown

hokoo commented Apr 6, 2024

Nice to have an option like "Retrieve meta data to display" in order to not overload DB when it is not needed.

@marcinkrzeminski
Copy link
Copy Markdown
Contributor

@akirk thanks for the PR. I agree that it would be best to make this feature optional. Do you have the capacity to add a setting that enables it?

@akirk akirk force-pushed the meta-field-display branch from e70f46c to 5838f7b Compare May 22, 2025 10:12
@akirk akirk force-pushed the meta-field-display branch 3 times, most recently from 52e6d2d to cec619a Compare May 24, 2026 07:23
@akirk akirk force-pushed the meta-field-display branch from e77db3a to 099f795 Compare May 26, 2026 11:49
@PatelUtkarsh
Copy link
Copy Markdown
Member

Hi @akirk, are you still interested in landing this? The metadata display feature is useful and your recent rebase suggests you might be.

To move forward, two things:

  1. Rebase onto current develop; the branch is conflicting.
  2. Review the AI-assisted code review below (I reviewed all comments) and respond to the findings. Push back on anything that doesn't hold up.

Happy to help with either or take it over if you'd prefer to hand it off.


AI-assisted code review

Findings on the diff between this PR's HEAD and current develop. Each was double-checked against the actual code on this branch and the WordPress meta API docs before posting. Classified as blocker, warning, or nit. Numbered for easy reference (e.g. "comment 3").

Blockers

1. classes/class-db-driver-wpdb.php (insert_record meta loop) β€” UTF-8 mid-byte truncation via substr()

substr( $val, 0, 200 ) cuts on byte boundaries. Any multibyte character that straddles byte 200 produces invalid UTF-8 in stream_meta.meta_value. The schema is varchar(200) so MySQL truncates at the character boundary anyway; doing the chop in PHP is both redundant and unsafe. Either use mb_strimwidth( $val, 0, 200, '' ) / mb_substr, or drop the PHP-side truncation entirely.

2. classes/class-settings.php and classes/class-list-table.php β€” bypasses network-aware accessor

List_Table::should_display_metadata() reads $this->plugin->settings->options['advanced_display_metadata']. Settings::$options is populated from get_site_option( 'wp_stream' ) only inside is_network_admin(); in the regular per-site records page it reads get_option( 'wp_stream' ) β€” the per-site option store. On a network-activated install the authoritative store is wp_stream_network, so the toggle won't take effect.

Use $this->plugin->settings->get_setting_value( 'advanced_display_metadata', 0 ) instead. That accessor was introduced for the Abilities/MCP work and resolves the network-vs-site read centrally.

3. classes/class-record.php (get_meta) β€” $single=false wraps the result in an extra array layer

public function get_meta( $meta_key = '', $single = false ) {
    $meta = get_metadata( 'record', $this->ID, $meta_key, $single );
    if ( '' === $meta_key || ! empty( $meta ) ) {
        return '' === $meta_key ? self::normalize_meta( $meta ) : $meta;
    }
    $all_meta = self::normalize_meta( get_metadata( 'record', $this->ID ) );
    if ( ! array_key_exists( $meta_key, $all_meta ) ) {
        return $single ? '' : array();
    }
    return $single ? $all_meta[ $meta_key ] : array( $all_meta[ $meta_key ] );
}

Trace get_meta('user_meta', false) against a record with bracketed sub-keys:

  1. get_metadata( ..., 'user_meta', false ) returns [] (no row with that literal key).
  2. The ! empty( $meta ) short-circuit is false, so it falls through.
  3. Second get_metadata returns the bracketed rows; normalize_meta produces ['user_meta' => ['agent' => 'wp_cli', 'display_name' => 'HAL']].
  4. $single is false, returns array( ['agent' => ..., 'display_name' => ...] ) β€” wrapped in an extra outer array.

WP's get_metadata( ..., $single=false ) returns a flat array of values; the new behavior is [ [keyed] ]. The existing test (test_query_fetches_array_meta_when_requested) only exercises $single=true, which works. The $single=false path is the bug.

Also: the fall-through executes a second SQL query on every key miss. On a list-table render of 50 rows, that's an extra 50 queries when none of the records have the requested key.

Simplest fix: drop the magic. Keep get_meta() as a pass-through to get_metadata() and document that callers needing bracketed-key reconstruction call Record::normalize_meta() themselves.

Warnings

4. classes/class-db-driver-wpdb.php (insert_record meta loop) β€” user_meta shape splits across the write boundary

Before this PR, nested-array meta was written with sub-keys discarded. meta = ['user_meta' => ['user_email' => 'admin@example.com', 'display_name' => 'admin', ...]] produced one row per value, all keyed user_meta, sub-keys lost at write time. Verified empirically against the live stream_meta table on a current install: rows keyed user_meta hold bare values like admin@example.com, admin, Administrator.

After this PR, the same input writes user_meta[user_email], user_meta[display_name], etc. Reads via Record::normalize_meta() reconstruct the nested object.

Both shapes coexist in stream_meta after deploy. Observable in two places:

  • The new "Display Metadata" column in the list table renders both shapes in adjacent rows. An admin browsing across the upgrade boundary will see "user_meta": ["admin@example.com", "admin", "admin", "Administrator"] for old records and "user_meta": {"user_email": "admin@example.com", ...} for new ones.
  • Ability_Get_Record (MCP/REST stream/get-record) returns the same split. The output schema declares meta as object, which both shapes satisfy at the top level, but user_meta itself changes from a JSON array to a JSON object.

Where this does not matter on current core: the list table's own Summary/User columns, connectors, exporters, and alerts do not read user_meta from the stored rows. They construct Author from get_userdata() live, so the storage difference is invisible to them. The data has been effectively unused at the admin-UI layer.

Recommendation: don't ship a migration; document the split. Old records are still useful (the values are searchable strings even without sub-keys), so a lossy-delete migration removes data unnecessarily. Position-based remapping is fragile (the old write loop's empty-value filter shifts positions per-record) and shouldn't be attempted. A changelog note plus a paragraph in the MCP ability docs is enough: "Records logged before X.Y.Z return user_meta as an indexed array; records from X.Y.Z onward as an object. MCP clients should handle both shapes."

5. classes/class-record.php (get_by_id) β€” meta shape change is a public-API change for MCP/REST

Before: $row['meta'] = (array) get_metadata( 'record', $row['ID'] );
After: $row['meta'] = self::normalize_meta( get_metadata( 'record', $row['ID'] ) );

abilities/class-ability-get-record.php (the stream/get-record ability) returns $row['meta'] directly. After this PR, scalar single-row meta gets unwrapped from ['v'] to 'v' (via normalize_meta_value()'s count === 1 collapse), and bracketed rows get re-grouped. MCP/REST clients of stream/get-record see a different JSON shape after deploy.

The unwrap arguably matches what most callers expected anyway ("space_helmet": "false" reads more naturally than "space_helmet": ["false"]), so this is an intentional API improvement, not a regression. But it is a shape change. Mention it in the changelog alongside finding 4 and the MCP doc.

6. classes/class-record.php (normalize_meta_value) β€” collapses 1-element arrays unconditionally

if ( is_array( $value ) ) {
    $value = array_map( 'maybe_unserialize', $value );
    if ( 1 === count( $value ) ) {
        return reset( $value );
    }
    return $value;
}

This is what produces the unwrap in finding 5. It diverges from get_metadata's documented shape but matches what's intuitive. Decision needed: keep the collapse (and document) or drop it (and let callers index). My weak preference is keep + document; the collapsed shape is friendlier for MCP clients.

7. classes/class-query.php (count query) β€” removed {$join}

-		FROM $wpdb->stream
-		{$join}
-		WHERE 1=1 {$where}";
+		FROM $wpdb->stream
+		WHERE 1=1 {$where}";

$join is '' in stock Stream so this is a no-op today. But the wp_stream_db_query_where / wp_stream_db_count_query filters and custom DB_Driver subclasses are documented extension points; a downstream consumer that injects a JOIN via filter expected count and select to apply symmetrically. The commit message says "Fix count query" β€” what was the actual count miscount you were addressing? If it was meta-join row multiplication, SELECT COUNT(DISTINCT $wpdb->stream.ID) is the structural fix and preserves the extension point.

8. classes/class-query.php (meta fetch loop) β€” ordering + per-record cap

The second SQL uses ORDER BY meta_id ASC. Fine, but two notes:

  1. With records_per_page = 50 on a chatty site (5-10 meta rows per record) this pulls ~500 rows per admin page render. Acceptable given the opt-in setting, but a single pathological record could spike the response size. Consider documenting the expectation or adding a soft cap.
  2. Legacy multi-row records depend on meta_id ASC to produce a stable read order. The test fixture (dummy_meta_data()) returns a flat array, so the legacy path isn't exercised. Please add a test that inserts the legacy shape and asserts the read order.

9. classes/class-query.php (synthetic record_id alias)

if ( $with_meta ) {
    if ( ! empty( $fields ) && ! in_array( 'ID', $fields, true ) ) {
        $selects[] = "$wpdb->stream.ID AS record_id";
    }
}

Cleaner: if with_meta is on, just append ID to $selects when missing. Eliminates the aliasing and the unset( $item->record_id ) cleanup further down.

10. classes/class-record.php (bracketed-key parser) β€” edge cases

'foo[bar][baz]' (nested brackets) parses main_key = 'foo', sub_key = 'bar][baz'. A meta key from another plugin that happens to contain [...] could also be misparsed. Recommend either rejecting nested brackets at the writer or documenting the format as one-level-only and adding a test that proves either decision.

Nits

11. connectors/class-connector-buddypress.php: pure whitespace re-alignment. Split or revert; drift in feature PRs hurts bisects.

12. classes/class-settings.php: field name is display_metadata but the read uses advanced_display_metadata. Correct (Stream prefixes by tab slug), but a code comment near the read would prevent confusion.

13. classes/class-query.php: the new comma-string fields parsing isn't documented in the args docblock. Either remove (internal callers use the array form) or document.

14. classes/class-record.php (normalize_meta): if both a flat user_meta row and a bracketed user_meta[agent] row coexist for the same record, the flat value gets silently overwritten by the array. Document the precedence or guard at the writer.

15. Test coverage: please add cases for the legacy multi-row format (insert old shape via raw SQL, read via get_by_id), multibyte values >200 bytes, a key with literal [ from an unrelated source, a record with both flat and bracketed keys, and get_meta('user_meta', false) against bracketed storage (to lock in the fix for finding 3).

@akirk
Copy link
Copy Markdown
Author

akirk commented May 29, 2026

Yes! Thanks for that review. I didn't have enough time to test it thoroughly enough after my changes, so I hadn't continued the conversation yet.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Meta not included into Record object?

4 participants