WordPress.org

Make WordPress Core

Opened 5 years ago

Closed 3 weeks ago

#9153 closed defect (bug) (fixed)

wp_list_pages cannot handle multiple exclude_tree arguments

Reported by: tbrams Owned by: tbrams
Milestone: 3.9 Priority: normal
Severity: major Version: 2.7
Component: Posts, Post Types Keywords: has-patch
Focuses: template Cc:

Description

When trying to set up a menu based on wp_list_pages and a long list of chapters I did not want on the front page, I learned that wp_list_pages ignored all exclude_tree arguments - except for the first one.

For example, using:

wp_list_pages('exclude_tree=132,502,715,852,109,153,149&title_li=&sort_column=menu_order' );  

to generate the menu at http://ttu.no was a no go, because it returned far to many sections in the menubar (despite my efforts to explicitly exclude a lot of these)

Although it is certainly not a show stopper, it is really annoying when you need a drop down menu on your website and know exactly how it should work in theory, so I have found a solution for this already and am just trying to figure out how I can convey my suggested fix to the official Open Source Repository.

Attachments (8)

post.php.diff (1.3 KB) - added by tbrams 5 years ago.
this is a patch for includes/post.php
9153.patch (1.2 KB) - added by hakre 5 years ago.
Low-Cost solution with not a single DB Call.
9153.2.patch (1.2 KB) - added by hakre 5 years ago.
Code Beautification
9153.3.patch (933 bytes) - added by hakre 5 years ago.
get_result() call fixed.
9153.4.patch (1.4 KB) - added by hakre 5 years ago.
Code-Cleanup for Excludes for 2.8 as well.
9153.5.patch (1.2 KB) - added by hakre 5 years ago.
Preg_Split again
get_pages_exclude_tree_multi.patch (1.3 KB) - added by roothorick 3 years ago.
I think this is the optimization Denis is talking about.
post.php-exclude_tree.patch (1.2 KB) - added by cgaffga 2 months ago.
roothorick patch updated to patch agains 3.8.1, where this problem persists

Download all attachments as: .zip

Change History (58)

comment:1 tbrams5 years ago

  • Owner changed from anonymous to tbrams
  • Status changed from new to assigned

tbrams5 years ago

this is a patch for includes/post.php

comment:2 tbrams5 years ago

  • Keywords has_patch added
  • Resolution set to fixed
  • Status changed from assigned to closed

This patch works nicely on my installation. Let me know if you have questions, comments or need anyting else

Cheers
Torben Brams

comment:3 filosofo5 years ago

  • Keywords has-patch wp_list_pages added; has_patch removed
  • Resolution fixed deleted
  • Status changed from closed to reopened

Don't close until it's actually committed. :)

comment:4 tbrams5 years ago

sorry - this is my first path ever.
Thanks for the guidance :-)

comment:5 tbrams5 years ago

  • Cc tbrams added

comment:6 MichaelH5 years ago

Patch seems to fix the problem. Also a suggested fix at http://wordpress.org/support/topic/251227 is very similar to this patch.

comment:7 tbrams5 years ago

I posted the same fix in the support forum before I was introduced to TRAC - you can see the original thread here: http://wordpress.org/support/topic/234808

So, how do we move on from here?

Kind regards
Torben Brams

comment:8 Denis-de-Bernardy5 years ago

  • Keywords needs-testing added; wp_list_pages removed

might also be related to #9580

comment:9 Denis-de-Bernardy5 years ago

patch could be optimized by grabbing the full list of children first, and then stripping them all at once from the pages list.

hakre5 years ago

Low-Cost solution with not a single DB Call.

comment:10 hakre5 years ago

Database was already queried and since this is a exclude function only comparing against ID and post_parent is necessary. That should do the trick at first. Pages that are deeper then one level to an excluded ID are not removed. But that would stress the Database as well (querying of all pages must be done then to establish such a functionality).

hakre5 years ago

Code Beautification

comment:11 Denis-de-Bernardy5 years ago

$wpdb->geresults($query); is fatal

hakre5 years ago

get_result() call fixed.

hakre5 years ago

Code-Cleanup for Excludes for 2.8 as well.

comment:12 hakre5 years ago

therefore see patch in #8683.

comment:13 Denis-de-Bernardy5 years ago

  • Component changed from General to Template

comment:14 ryan5 years ago

Going only one level deep is a bug. We also need to retain the preg_split that allows separating by comma or spaces.

comment:15 Denis-de-Bernardy5 years ago

  • Keywords needs-patch added; has-patch needs-testing removed

hakre5 years ago

Preg_Split again

comment:16 hakre5 years ago

  • Keywords has-patch added; needs-patch removed

Deepness of one level is the documented functionality and not a bug.

comment:17 ryan5 years ago

It's a change of behavior and will compromise the parent dropdown list on the edit screen.

comment:18 hakre5 years ago

then i tend to say the documentation has the bug. and lets do not forget that the old behaviour is not working. so a bugfix will be a change of behaviour anyway. to draw the line.

comment:19 Denis-de-Bernardy5 years ago

  • Keywords needs-patch added; has-patch removed

@harke: doc should be amended, and the needed behavior for the parent dropdown list should be enforced. I'll give your new patch a shot if you write it.

comment:20 hakre5 years ago

you want it n-depth? or should be a limit considered, because afaik you can create parent / child loops with the used datastructure.

comment:21 Denis-de-Bernardy5 years ago

  • Milestone changed from 2.8 to Future Release

punting pending patch

comment:22 Denis-de-Bernardy5 years ago

  • Milestone changed from Future Release to 2.9

comment:23 hakre5 years ago

Didn't we had some suggestions on how to better deal with parents? The bug should be technically easy to solve. I'm just asking because of performance.

comment:24 keatch5 years ago

  • Cc keatch added

comment:25 azaozz4 years ago

  • Milestone changed from 2.9 to Future Release

Don't think changing the behavior would be good here. Excluding only the specified page + direct children bit still showing 2nd, 3rd, etc. level (grand)children would make this unusable in many cases. Perhaps the performance problems will be solved when/if MPTT is used.

comment:26 follow-up: CreativeNotice4 years ago

Bit new here... can I get a brief synopsis of why this isn't fixed as it sounds rather simple. Not gripping, just don't understand the hold up. Is it a DB resources issue?

comment:27 in reply to: ↑ 26 ; follow-up: hakre4 years ago

Replying to CreativeNotice:

Bit new here... can I get a brief synopsis of why this isn't fixed as it sounds rather simple. Not gripping, just don't understand the hold up. Is it a DB resources issue?

Changing this function would break the known behavior. That means that changes might break backwards compability.

A solution could be to introduce a new function which does it properly.

comment:28 hakre4 years ago

Another solution might be to add a new parameter to that function that does exactly what you're after. That won't break backwards compability either and would save to introduce a new function. Since we already have a patch, this might be the route to go to preserve the code from being postponed until the end of days.

comment:29 in reply to: ↑ 27 eduplessis4 years ago

  • Version changed from 2.7 to 3.0

Replying to hakre:

Replying to CreativeNotice:

Bit new here... can I get a brief synopsis of why this isn't fixed as it sounds rather simple. Not gripping, just don't understand the hold up. Is it a DB resources issue?

Changing this function would break the known behavior. That means that changes might break backwards compability.

A solution could be to introduce a new function which does it properly.

How can it's break backwards compability if it's not working... for the first time??

comment:30 hakre3 years ago

Replying to eduplessis:

How can it's break backwards compability if it's not working... for the first
time??

Probably because some (not me and you obviously) might rate that bug a feature.

comment:31 roothorick3 years ago

Why hasn't the original, first patch been committed yet? It's only slower in the cases where the bug manifests, so there is no sane reason why this bug has been left open for two years.

I've been over get_pages() a couple times now, and the optimization suggested by Denis-de-Bernardy isn't realistic. You'd have to rewrite get_pages() and essentially have two copies of the same function -- one that uncondititonally gathers all pages from the DB and filters them by hand, and one that filters at the database level like it does now. Unless there's a magic SQL trick to recursively look up keys by parent...

You should commit the original patch that tbrams submitted. Barring something unreasonably clever, it's the most appropriate solution possible.

comment:32 roothorick3 years ago

I've re-read the comments, and now I'm just confused. I think I misinterpreted Denis' suggestion, but hakre's patches don't make sense to me. What DB call, exactly, are you optimizing out? get_page_children() doesn't call the DB.

I'm now taking a closer look at get_page_children() to try to see what Denis was thinking. I may or may not produce a new patch.

roothorick3 years ago

I think this is the optimization Denis is talking about.

comment:33 roothorick3 years ago

  • Keywords has-patch added; needs-patch removed

comment:34 Denis-de-Bernardy3 years ago

I think this is the optimization Denis is talking about.

Looks like it...

Unless there's a magic SQL trick to recursively look up keys by parent...

There is (that would be a recursive with statement), but not in MySQL:

comment:35 lkraav2 years ago

  • Cc lkraav added

comment:36 lkraav2 years ago

What state is this in? It appears I'm stuck between a rock and a hard place with WPML, where a second wp_page_menu fails to be correctly ID-adjusted for translations with 'include'. And then this core bug is apparently stopping 'exclude_tree' from working with multiple trees, which would be a workaround for the WPML bug.

Is the latest get_pages_exclude_tree_multi.patch enough alone or does it depend on the earlier ones?

comment:37 treb0r2313 months ago

I would just like to confirm that this bug is still present in Wordpress 3.5.1.

What can be done?

Last edited 13 months ago by treb0r23 (previous) (diff)

comment:38 treb0r2313 months ago

  • Cc treb0r23 added

comment:39 SergeyBiryukov12 months ago

#19478 was marked as a duplicate.

comment:40 SergeyBiryukov12 months ago

#16202 was marked as a duplicate.

comment:41 SergeyBiryukov12 months ago

#24249 was marked as a duplicate.

comment:42 SergeyBiryukov5 months ago

#25900 was marked as a duplicate.

comment:43 SergeyBiryukov5 months ago

  • Milestone changed from Future Release to 3.8
  • Version changed from 3.0 to 2.7

comment:44 dd325 months ago

  • Type changed from defect (bug) to enhancement

comment:45 wonderboymusic5 months ago

  • Milestone changed from 3.8 to Future Release

No action as of late

comment:46 nacin3 months ago

  • Component changed from Template to Posts, Post Types
  • Focuses template added

cgaffga2 months ago

roothorick patch updated to patch agains 3.8.1, where this problem persists

comment:47 cgaffga2 months ago

  • Severity changed from normal to major
  • Type changed from enhancement to defect (bug)

This problem seems to persist as of version 3.8.1 :( It was driving me mad. I just added the updated patch agains WP 3.8.1 that makes the exclude_treeparameter work for more than one page ID.

Can't this patch be finally added to the core code!?

See also #27092, recreated to update the version number to WP 3.8.1.

Last edited 2 months ago by cgaffga (previous) (diff)

comment:48 SergeyBiryukov2 months ago

#27092 was marked as a duplicate.

comment:49 SergeyBiryukov2 months ago

  • Milestone changed from Future Release to 3.9

comment:50 wonderboymusic3 weeks ago

  • Resolution set to fixed
  • Status changed from reopened to closed

In 27767:

Use wp_parse_id_list() when parsing exclude_tree in get_pages(). Add unit tests to ensure a URL string, array with string as value, and array with array as value for exclude_tree can be used to specify multiple IDs.

Props cgaffga, roothorick, hakre, tbrams for patches across the years.
Fixes #9153.

Note: See TracTickets for help on using tickets.