Ticket #9153 (reopened defect (bug))

Opened 3 years ago

Last modified 7 months ago

wp_list_pages cannot handle multiple exclude_tree arguments

Reported by: tbrams Owned by: tbrams
Priority: normal Milestone: Future Release
Component: Template Version: 3.0
Severity: normal Keywords: has-patch
Cc: tbrams, keatch

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

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

Change History

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

tbrams3 years ago

this is a patch for includes/post.php

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

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

Cheers Torben Brams

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

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

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

  • Cc tbrams added

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

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

  • Keywords needs-testing added; wp_list_pages removed

might also be related to #9580

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

hakre3 years ago

Low-Cost solution with not a single DB Call.

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).

hakre3 years ago

Code Beautification

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

hakre3 years ago

get_result() call fixed.

hakre3 years ago

Code-Cleanup for Excludes for 2.8 as well.

therefore see patch in #8683.

  • Component changed from General to Template

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

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

hakre3 years ago

Preg_Split again

  • Keywords has-patch added; needs-patch removed

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

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

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.

  • 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.

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

  • Milestone changed from 2.8 to Future Release

punting pending patch

  • Milestone changed from Future Release to 2.9

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.

  • Cc keatch added
  • 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: ↓ 27   CreativeNotice2 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: ↓ 29   hakre2 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.

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   eduplessis19 months 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??

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.

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.

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.

I think this is the optimization Denis is talking about.

  • Keywords has-patch added; needs-patch removed

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:

Note: See TracTickets for help on using tickets.