Yeah, let's call taxonomy_get_tree() on every vocabulary on a site to make a form no one will ever, ever be able to use, because if it doesn't crash the server, it still might crash their browser.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Dries’s picture

Status: Needs review » Fixed

Committed to CVS HEAD. Thanks!

Dave Reid’s picture

Title: taxonomy_form_all() is dead code, and dangerous » [Needs rollback] taxonomy_form_all() is dead code, and dangerous
Priority: Normal » Critical
Status: Fixed » Needs work

This is the second culprit of test failures. When I rolled back this patch, the Taxonomy term filters in node forms (TaxonomyNodeFilterTestCase) test passed locally in contrast to 18 fails with current non-rolled-back clean HEAD.

Dave Reid’s picture

In fact the test bot was going to mark this as a fail (http://qa.drupal.org/pifr/test/26456) but I think it was committed before it had the chance to report back.

Dave Reid’s picture

Status: Needs work » Needs review
FileSize
1.02 KB

Reversal of the original patch for now.

catch’s picture

Status: Needs review » Reviewed & tested by the community

Node module does module_invoke('taxonomy', 'form_all'). That's evil.

Dries’s picture

Status: Reviewed & tested by the community » Fixed

Rolled back. Sorry about that.

catch’s picture

Title: [Needs rollback] taxonomy_form_all() is dead code, and dangerous » taxonomy_form_all() is dangerous
Priority: Critical » Normal
Status: Fixed » Active
catch’s picture

Status: Active » Needs review
Issue tags: +Performance
FileSize
8.73 KB

So the reason tests fail is because admin/content builds a list of every single term in every vocabulary for a select list, and search/node does the same. Both of these are nasty, nasty resource hogs once you go past a certain number of terms. They're also use cases which are much better served by #497804: [meta] Search entities (nodes, terms, etc.) within the administrative interface for admin/content, faceted search instead of advanced search, or views (where you can use autocomplete, or restrict filters to a vocabulary which doesn't have thousands of terms in it).

So here's a patch to just rip that functionality out completely. If that's not acceptable for D7, then we should at least do a COUNT(*) query here and bail out after a certain number, lots of sites use tagging.

moshe weitzman’s picture

Status: Needs review » Reviewed & tested by the community

I agree with stripping this feature. Core just can't adequately do what it is trying to do here (show all terms in a select).

Dries’s picture

Status: Reviewed & tested by the community » Fixed

I support this change as well, and committed the patch to CVS HEAD. It is just not a scalable solution for some sites, and better alternatives are available if necessary.

Status: Fixed » Closed (fixed)
Issue tags: -Performance

Automatically closed -- issue fixed for 2 weeks with no activity.