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

Use unique keys for stream meta #1038

Open
wants to merge 2 commits into
base: develop
Choose a base branch
from
Open

Use unique keys for stream meta #1038

wants to merge 2 commits into from

Conversation

nprasath002
Copy link

Right now stream use the same key if meta has an array. See the screenshot below. Stream usesuser_meta as key for all the data in array user_meta instead of array keys

Screen Shot 2019-10-16 at 3 26 44 PM

image

Checklist

  • Project documentation has been updated to reflect the changes in this pull request, if applicable.
  • I have tested the changes in the local development environment (see contributing.md).
  • I have added phpunit tests.

Release Changelog

  • Fix: Describe a bug fix included in this release.
  • New: Describe a new feature in this release.

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.

@kasparsd
Copy link
Contributor

This looks great, thank you so much for the contribution @nprasath002!

On a sidenote -- are you using the bundled development environment to get Xdebug working with the plugin or you have custom setup for that?

@kasparsd kasparsd added the bug label Oct 16, 2019
@nprasath002
Copy link
Author

https://github.com/Chassis/Chassis comes with xdebug

@nprasath002 nprasath002 changed the title Use unique kets for stream meta Use unique keys for stream meta Oct 24, 2019
@lkraav
Copy link
Contributor

lkraav commented May 4, 2020

@nprasath002 what if there's a meta key name conflict across object types.. is that possible?

Here's my live db meta key breakdown for reference, sorted by COUNT DESC. It's true that user_meta definitely takes the cake.

How is user_meta data read back? If we rename the keys, are the display operations still working fine?

meta_key | Count
----------------
user_meta | 37027773
post_title | 7006941
singular_name | 3911511
post_id | 3097690
comment_type | 3097611
user_name | 3007114
user_id | 1865416
comment_status | 1168822
is_spam | 1140055
display_name | 420376
option | 253911
label | 253843
context | 253701
value | 248995
new_status | 248243
old_status | 248243
post_date | 245711
post_date_gmt | 245711
old_value | 232780
option_key | 204325
revision_id | 66936
form_id | 41335
form_title | 40949
lead_id | 40829
roles | 12877
name | 10031
email | 8269
url | 6105
parent_id | 5992
parent_title | 4828
meta_key | 4655
post_type | 4655
meta_value | 4622
user_login | 4597
id | 1437
slug | 1430
type | 1402
success | 1233
version | 1233
new_status_name | 1225
old_status_name | 1225
title | 1207
taxonomy | 1135
taxonomy_label | 1135
term_id | 1135
term_name | 1135
term_parent | 1133
old_version | 1125
action | 1120
lead_status | 851
old_instance | 656
menu_id | 600
menu_data | 580
is_new | 317
status | 275
prev | 258
confirmation | 212
page | 169
sidebar_id | 164
widget_id | 164
section | 141
tab | 141
sidebar_name | 138
form_status | 110
filename | 80
notification | 59
new_value | 50
option_title | 50
new_role | 47
old_role | 47
is_update | 44
author | 28
item_id | 28
ids | 25
count | 20
titles | 20
note_id | 15
order_id | 15
location | 6
location_id | 6
tax_rate | 5
auto_updated | 4
new_version | 4
page_id | 4
tax_rate_country | 4
tax_rate_name | 4
tax_rate_state | 4
attribute_label | 3
attribute_name | 3
attribute_orderby | 3
attribute_public | 3
attribute_type | 3
end_date | 3
start_date | 3
tax_rate_class | 3
tax_rate_compound | 3
tax_rate_order | 3
tax_rate_priority | 3
tax_rate_shipping | 3
error | 2
args | 1
event_type | 1

@lkraav
Copy link
Contributor

lkraav commented May 4, 2020

@eugenekireev seems to have worked on it way back in the day in 452d6e1

I wonder if he remembers anything about the why's?

@lkraav
Copy link
Contributor

lkraav commented Mar 8, 2022

Now that #1080 is merged, I'll take care of this next.

@lkraav lkraav mentioned this pull request Mar 9, 2022
@lkraav
Copy link
Contributor

lkraav commented Mar 9, 2022

I've looked into this. It seems $user_meta is a special object, and I think we need to continue giving it special treatment to the end.

Stream meta keys don't don't really lend themselves for de-duplication... I think the meta key will simply have to be array notation string, like user_meta[user_role_label] etc.

If no objections @kasparsd @kopepasah I'll file a PR towards this.

lkraav added a commit to lkraav/stream that referenced this pull request Mar 9, 2022
@lkraav lkraav mentioned this pull request Mar 9, 2022
@lkraav
Copy link
Contributor

lkraav commented Mar 9, 2022

PR filed.

I don't see how generic user_meta meta keys are useful in any way, since you can't query for values based on anything, so we should probably do something here.

I left original (array) conversions as-is for the final record insert, because maybe users are relying on ability to pass arrays in.

Also @kasparsd develop branch isn't up to date with master.

lkraav added a commit to conversionxl/stream that referenced this pull request Apr 25, 2023
lkraav added a commit to conversionxl/stream that referenced this pull request Nov 13, 2023
lkraav added a commit to conversionxl/stream that referenced this pull request Mar 1, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants