While trying to optimize a big form by setting $element[#sorted] to TRUE, it found out that FAPI resets this value in two different places.

In form_builder(), $element['#sorted'] is reset to TRUE and then set to FALSE if any child has a #weight specified.

In form_pre_render_fieldset(), $element['#sorted'] is set to FALSE, even if no children were added (e.g. when $element['#group_members'] only contains one entry, array('#group_exists' => TRUE)).

The latter makes it possible to skip sorting of the individual fieldsets (“Core” etc.) on the modules overview page, admin/config/modules.

CommentFileSizeAuthor
#3 fapi-sorted-2.patch3.66 KBc960657
fapi-sorted-1.patch5.33 KBc960657

Comments

dries’s picture

dries commented

I think we make that assumption because sorting a huge array is expensive -- and the sorting can happen for every page (e.g. when the search form is enabled, or when the user login block is enabled). It would be good to do some performance testing on this patch.

c960657’s picture

c960657 commented

I assume you are referring to the assumption mentioned in this comment that was deleted:

-  // We start off assuming all form elements are in the correct order.
-  $element['#sorted'] = TRUE;

I deleted the line in question, but the assumption is good enough, i.e. form_builder() currently sets #sorted to TRUE and then iterates through the child elements and sets it to FALSE, if it encounters an element with a #weight.

The problem with this approach is that if $element['#sorted'] was already TRUE when the method was called, this information is discarded. form_builder() does not change the order of the elements, so it should not change $element['#sorted'] from TRUE to FALSE, even if a child element has a #weight.

Does this address your concern, or did I misunderstand you?

c960657’s picture

c960657 commented
StatusFileSize
new fapi-sorted-2.patch3.66 KB

Reroll.

The problem in form_pre_render_fieldset() was fixed in #558666: UX/security: Revamp text format/filter configuration. The patch now only includes a fix for form_builder() and then uses this new optimization in system_modules().

catch’s picture

catch
he/him
Primary language English
commented

I can't see any change profiling this - should there be less calls to element_sort() on admin/config/modules?

c960657’s picture

c960657 commented
Status: Needs review » Needs work

It looks like #558666: UX/security: Revamp text format/filter configuration removed the extra sorting on admin/config/modules (I checked). I will need to check whether that fix makes the rest of the patch redundant too.

Status: Needs work » Closed (outdated)

Automatically closed because Drupal 7 security and bugfix support has ended as of 5 January 2025. If the issue verifiably applies to later versions, please reopen with details and update the version.