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.

CommentFileSizeAuthor
#8 taxonomy_form_all_die.patch8.73 KBcatch
#4 taxonomy_form_all.patch1.02 KBdave reid
taxonomy_form_all.patch1.02 KBcatch

Comments

dries’s picture

dries commented
Status: Needs review » Fixed

Committed to CVS HEAD. Thanks!

dave reid’s picture

dave reid
he/him
Primary language English
Location Nebraska USA
commented
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

dave reid
he/him
Primary language English
Location Nebraska USA
commented

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

dave reid
he/him
Primary language English
Location Nebraska USA
commented
Status: Needs work » Needs review
StatusFileSize
new taxonomy_form_all.patch1.02 KB

Reversal of the original patch for now.

catch’s picture

catch
he/him
Primary language English
commented
Status: Needs review » Reviewed & tested by the community

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

dries’s picture

dries commented
Status: Reviewed & tested by the community » Fixed

Rolled back. Sorry about that.

catch’s picture

catch
he/him
Primary language English
commented
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

catch
he/him
Primary language English
commented
Status: Active » Needs review
Issue tags: +Performance
StatusFileSize
new taxonomy_form_all_die.patch8.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

moshe weitzman commented
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

dries commented
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.

Issue tags: +Performance

Restoring issue tags, see #2125755: System messages removed all issue tags during D7 upgrade.