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

Re-implement logic for previewing WP_Query and introduce non-multiple postmeta #248

Merged
merged 15 commits into from Sep 21, 2016

Conversation

Projects
None yet
3 participants
@westonruter
Copy link
Contributor

commented Sep 5, 2016

This introduces a somewhat radical approach to getting customized data to appear as expected in previewed post queries: the customized data is UNION'ed with the wp_posts and wp_postmeta table via inline literal SELECTs which then serve as a subselect for the main select which gets the results. The effect is that we can rely on MySQL as normal to handle all of the various WP_Query vars, including any Meta Queries that may be thrown at it.

This fixes what the #247 PR set out to do, which was to hackily force a customized post to appear on the first page of results (even if it doesn't belong there). By going deep into the MySQL query to inject the customized data, it seems that we get complete support for WP Query, amazingly. It's still hard to believe that this works and I want to consider this experimental and get testing on it.

Fixes #246

Non-single (multiple) postmeta is credited to @PatelUtkarsh.

  • Add unit tests for multi-postmeta.
  • Change priority of the_posts filter to apply as early as possible since post fields will get hydrated there since SELECT literals will omit data not needed.
  • Add customize_previewed_postmeta_rows filter to allow meta to be filtered before being injected into a query:
    foreach ( $postmeta_rows as $postmeta_row ) {
  • Fix queries made when fields => ids is used.
  • Ensure that customize_previewed_posts_for_query filter can indeed be removed.
  • Consider reversing the order of the UNIONs so that customized posts will be returned first if a list of posts have identical values to orderby, as then the sorting becomes unstable, and the same order would not be previewed. This is an edge case.
  • Preserve order of plural postmeta?
foreach ( $table_fields as $field_name => $type ) {
$select_field = sprintf(
'CAST( %s AS %s) AS %s',
$wpdb->prepare( '%s', $postmeta_row[ $field_name ] ),

This comment has been minimized.

Copy link
@PatelUtkarsh

PatelUtkarsh Sep 5, 2016

Collaborator

@westonruter Not accepting a case when $meta_value is an array. (Takes fist value and ignores other.)

This comment has been minimized.

Copy link
@westonruter

westonruter Sep 5, 2016

Author Contributor

@PatelUtkarsh isn't that handled by the building up of the $postmeta_rows array above? It checks if it is not single, and of not, it iterates over each of the meta values to then add as separate rows here.

  • One deficiency I do notice, however, is that it needs to maybe_serialize().

This comment has been minimized.

Copy link
@PatelUtkarsh

PatelUtkarsh Sep 5, 2016

Collaborator

@westonruter oh my bad I didn't declare it single => false

@westonruter

This comment has been minimized.

Copy link
Contributor Author

commented Sep 5, 2016

@PatelUtkarsh so you could integrate with the project by adding $single = false to the Customize_Postmeta_Multi_Setting class and it would then “just work”, right?

@PatelUtkarsh

This comment has been minimized.

Copy link
Collaborator

commented Sep 6, 2016

@westonruter I passed it in Customize_Postmeta_Multi_Setting 's constructor $args to make it work as $single property will be initialized in the constructor call of WP_Customize_Postmeta_Setting.

@westonruter

This comment has been minimized.

Copy link
Contributor Author

commented Sep 6, 2016

@PatelUtkarsh that will work too.

I'm trying to figure out if it makes more sense for the customize_sanitize_{$this->id} filter to apply on each individual multii-postmeta value separately (as it is doing in this PR now), or if the array of values should be all passed into the applied filter once. If applied individually to each multi-value then it would allow the same callback to be used for single and non-single postmeta.

@PatelUtkarsh

This comment has been minimized.

Copy link
Collaborator

commented Sep 6, 2016

@westonruter I can not think of a use-case where I would need all multi-post meta values together for the filter. Maybe to limit the multi-meta or removing duplication but that can be handled by control.

Edit: Ordering of multiple-meta? Sync post meta code does not preserve order.

westonruter added some commits Sep 6, 2016

Add character set and collation to each char/text field exported
Fixes failure to union selects due to mismatched collations

* Move `maybe_serialize()` to postmeta instead of post data.
* Move field mention checks outside of loop.
* Omit exporting any customized TEXT fields unless they are mentioned.
* Eliminate unnecessary placeholder_meta_id, replacing with NULL.
@westonruter

This comment has been minimized.

Copy link
Contributor Author

commented Sep 6, 2016

Comparing some queries with and sans the customized data injected: https://gist.github.com/westonruter/36563b2356a7a3c891f178176fc99b3a

westonruter added some commits Sep 7, 2016

Eliminate provisions for allowing postmeta for placeholder posts
Creating postmeta settings for posts that have negative post IDs does not work because get_post_meta() uses absint() on the post ID

@westonruter westonruter added this to the 0.8.0 milestone Sep 15, 2016

@westonruter

This comment has been minimized.

Copy link
Contributor Author

commented Sep 16, 2016

@PatelUtkarsh let me know when you're confident this can be merged.

@PatelUtkarsh

This comment has been minimized.

Copy link
Collaborator

commented Sep 20, 2016

@westonruter Should I update the code for preserve an order of plural postmeta?

@westonruter

This comment has been minimized.

Copy link
Contributor Author

commented Sep 20, 2016

@PatelUtkarsh I'm leading towards no. It's current behavior for the order to not be stable in the entities plugin, right? As such, making the order stable would be an enhancement. But if you think otherwise, please do amend the PR with persistent ordering.

@valendesigns

This comment has been minimized.

Copy link
Member

commented Sep 21, 2016

@westonruter Are you suggesting the order in the Select2 is an enhancement?

@westonruter

This comment has been minimized.

Copy link
Contributor Author

commented Sep 21, 2016

@valendesigns this is about the order in which the plural postmeta are written into the database, whether existing postmeta which are unchanged should be deleted so that they can be reinserted with a meta_id that increments correspondingly to the order in the plural meta. The non-singlular postmeta code in this PR is replicated from the plural-postmeta class in the entities plugin that @PatelUtkarsh wrote previously. So this PR wouldn't change that existing behavior.

@westonruter

This comment has been minimized.

Copy link
Contributor Author

commented Sep 21, 2016

@PatelUtkarsh please connect with @sayedwp and @miina as the ordering of the plural postmeta may indeed be what is affecting the ordering of repeater controls. If so, then we should simplify the update logic to delete all postmeta for a given key, and then re-add each value for the plural meta to ensure that the meta_id is auto-incrementing and preserves the order of the meta that were originally saved.

@westonruter westonruter merged commit db67775 into develop Sep 21, 2016

3 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
coverage/coveralls Coverage increased (+0.09%) to 94.753%
Details

@westonruter westonruter deleted the feature/query-preview-refactor branch Sep 21, 2016

@PatelUtkarsh

This comment has been minimized.

Copy link
Collaborator

commented Sep 21, 2016

@westonruter There is no such need for preserving order right now, Even in repeater control we don't need the order of plural post-meta as we are storing it in serialize for ordering and grouping(because grouping is difficult with plural post-meta).

If any special case arises we can handle it by overriding update method as you mentioned.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.