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

Issue with removing a pre_get_posts hook within itself #1444

Closed
bradyvercher opened this Issue Jul 18, 2014 · 5 comments

Comments

Projects
None yet
2 participants
@bradyvercher
Contributor

bradyvercher commented Jul 18, 2014

The wpsc_split_the_query() hook is being removed from within itself, which causes any hooks attached at the next priority to be skipped. The bug originates in WP, but modifying the implementation in WP eCommerce will help prevent a few headaches.

While writing a hotfix, I came up with a couple of possible solutions. Either:

  1. Attach a dummy hook just after wpsc_split_the_query() is registered so that at least two callbacks exist at the same priority: add_filter( 'pre_get_posts', '__return_null', 8 );
  2. Or move the remove_filter() call from wpsc_split_the_query() to a template_redirect hook so that it's only fired for the main query.

@bradyvercher bradyvercher changed the title from Issue with removing a pre_get_hosts_hook within itself to Issue with removing a pre_get_posts hook within itself Jul 18, 2014

@JustinSainton

This comment has been minimized.

Show comment
Hide comment
@JustinSainton

JustinSainton Jul 24, 2014

Member

Hi @bradyvercher!

I think the first solution, though a bit of a hack, would be less likely to break back compat. What do you think? Happy to take a PR for this.

Member

JustinSainton commented Jul 24, 2014

Hi @bradyvercher!

I think the first solution, though a bit of a hack, would be less likely to break back compat. What do you think? Happy to take a PR for this.

@JustinSainton JustinSainton self-assigned this Jul 24, 2014

@JustinSainton JustinSainton added this to the 3.9.0 milestone Jul 24, 2014

@bradyvercher

This comment has been minimized.

Show comment
Hide comment
@bradyvercher

bradyvercher Jul 24, 2014

Contributor

Hi @JustinSainton!

The first option is a hack, but it shouldn't cause any issues and it looks like that part of the code is being phased out anyways. Let me know if that PR works for you.

Contributor

bradyvercher commented Jul 24, 2014

Hi @JustinSainton!

The first option is a hack, but it shouldn't cause any issues and it looks like that part of the code is being phased out anyways. Let me know if that PR works for you.

@JustinSainton

This comment has been minimized.

Show comment
Hide comment
@JustinSainton

JustinSainton Jul 24, 2014

Member

Works for me, thanks Brady!

Member

JustinSainton commented Jul 24, 2014

Works for me, thanks Brady!

@JustinSainton JustinSainton reopened this Jan 20, 2015

@JustinSainton

This comment has been minimized.

Show comment
Hide comment
@JustinSainton

JustinSainton Jan 20, 2015

Member

This commit seems to have broken some search layouts for different themes.

Member

JustinSainton commented Jan 20, 2015

This commit seems to have broken some search layouts for different themes.

@JustinSainton JustinSainton modified the milestones: 3.9.2, 3.9.0 Jan 26, 2015

@JustinSainton

This comment has been minimized.

Show comment
Hide comment
@JustinSainton

JustinSainton Mar 21, 2015

Member

Reverting this for 3.9.2. It is causing more weird, obscure, hard to replicate bugs than it fixes. If people want to add the add_filter( 'pre_get_posts', '__return_null', 8 ); to a functions.php file or plugin - they can, and should, if it's causing them problems.

Until then, we'll revert and wait for WP17817 to land, hopefully in 4.3.

Member

JustinSainton commented Mar 21, 2015

Reverting this for 3.9.2. It is causing more weird, obscure, hard to replicate bugs than it fixes. If people want to add the add_filter( 'pre_get_posts', '__return_null', 8 ); to a functions.php file or plugin - they can, and should, if it's causing them problems.

Until then, we'll revert and wait for WP17817 to land, hopefully in 4.3.

JustinSainton added a commit that referenced this issue Mar 21, 2015

JustinSainton added a commit that referenced this issue Mar 21, 2015

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