Custom field fixes

Bloke · 2010-03-22 15:35:38

Continuing from gerhard’s thread I’d just like to raise a small issue that I think I’ve fixed.

As it stands, the core uses the preference value called max_custom_fields as a basis for drawing custom field input widgets on the Write tab, saving CF data in articles, populating CF info in articles on the public site, etc. The trouble with using a max_custom_field value — which simply counts the number of custom_N fields in the textpattern table — is that if you delete, say, custom_12 the max_custom_field count goes down by one but there’s a “hole” in the list; you now jump from custom_11 to custom_13 and omit custom_12. With a counter incrementing by one each time, this causes non-sequential custom field errors.

Some may argue this is a failing of glz_custom_fields but I think otherwise. IMO, if you delete a custom field (currently >10) — even from a plugin — then the core should honour your choice and not display an error on the admin side. Expecting a plugin to reshuffle the custom field data around in the textpattern table to maintain sequential column numbers is perhaps going to cause more hassle than it solves — and is certainly prone to error.

Various solutions have been put forward but they all have a fairly hefty performance impact. gerhard’s proposal unfortunately doesn’t preserve the custom field’s ID number and also extracts the prefs array once again, which is time-expensive (in my tests, that getCustomFields() function was nearly twenty times as slow as the current one — mainly because of having to get the prefs a second time).

In my continual search for _there must be a better way_™ I think I found something, but before I commit it I’d like a little feedback. What I propose is this:

Alter the get_prefs() function so plugin authors can optionally grab a sub-categorized prefs array if they wish. This isn’t vital to the actual patch but a nice-to-have for plugin authors to easily operate on a sub-section of the prefs array
Move getCustomFields() from publish.php to txplib_misc.php so it can be accessed from both admin and public sides of the interface
Alter the getCustomFields() function so it returns a non-contiguous array of what is actually set in your current setup; use of preg_grep and preg_match was surprisingly fast
In txp_article.php, alter any place where a static counter is used to find the custom fields, i.e replace:

$max = get_pref('max_custom_fields', 10);
for ($i = 1; $i <= $max; $i++) { ... }

with:

$cfs = getCustomFields();
foreach($cfs as $i => $cf_name) { ... }

The use of $prefs['max_custom_fields'] in txp_diag is fine as it is.

This is the most optimal solution I can find; I tested 5 or 6 different approaches to find the best one. If anybody has anything better to offer, please feel free. This version has a performance penalty of adding approximately 140 microseconds to the page render time, although the difference overall on the admin side was negligible due to all the other database stuff going on [the original getCustomFields() function took around 60μs on my test rig vs around 200μs with the new version].

Question: is that an acceptable performance hit given the fact that you will be able add and delete custom fields at will (technically even ones < 10, though I wouldn’t want to advise that right now!) without causing textpattern to cry or issue warnings?

Last edited by Bloke (2010-04-01 11:50:44)

mrdale · 2010-03-22 15:58:16

I know you’re looking for useful feedback (I’m unqualified) so I’ll resist the temptation to put on my cheerleader outfit…

…er “gimme an ‘S’?” …oops couldn’t resist.

Last edited by mrdale (2010-03-22 15:58:57)

Bloke · 2010-03-22 16:00:05

Ya spoon :-)

Thanks for the cheer, though.

wet · 2010-03-23 06:41:12

Bloke wrote:

Alter the get_prefs() function so plugin authors can optionally grab a sub-categorized prefs array if they wish.

I’d prefer to object this proposal. It would burden us with a “compatibility promise” for not just prefs names, but also prefs categories with no real benefit, AFAICS.

The prefs data model is a simple dictionary, not a tree graph, therefore we should not imply that something like this would be supported:

$prefs['foo']['bar'] = 'baz';
$prefs['omg']['bar'] = 'bbq';

Bloke · 2010-03-23 08:44:44

wet wrote:

I’d prefer to object this proposal.

Good point. Consider it rescinded.

Textpattern CMS

Textpattern CMS support forum

#1 2010-03-22 15:35:38

Custom field fixes

#2 2010-03-22 15:58:16

Re: Custom field fixes

#3 2010-03-22 16:00:05

Re: Custom field fixes

#4 2010-03-23 06:41:12

Re: Custom field fixes

#5 2010-03-23 08:44:44

Re: Custom field fixes

Board footer