glz_custom_fields

gerhard · 2010-03-21 15:48:15

Bloke wrote:

Anything I can do to make the ride easier?

I’m really glad you asked mate! I came with a solution quiet a while back, but Robert thought it would impact the performance. So this is what I came up with last year.

I don’t really know what implications will that change have, but that’s what makes most sense in my opinion. It makes all $prefs more organized and easier to traverse.

The reason why we’re having problems with custom fields is because currently there is $prefs['max_custom_fields'] which counts how many custom fields there are. So if I have 1-10 & 11, 13, 15 max_custom_fields will be 13 and will assume that 1-13 exist, hence the undefined error. I cannot work around this because this is one of TXP’s core functions, getCustomFields().

Bloke · 2010-03-22 09:55:20

gerhard wrote:

I don’t really know what implications will that change have, but that’s what makes most sense in my opinion. It makes all $prefs more organized and easier to traverse.

If that would help, that’s easy to fix; we could add an optional argument to get_prefs() to allow you to return the prefs list sub-categorized if you wish. No backwards-incompatibilities would be introduced and no animals would be harmed.

Then you could simply re-fetch the $prefs array manually yourself and use that version instead of the one that TXP publishes, thus ignoring the max_custom_fields pref and counting it yourself.

However, that still might not address the inherent problem of holes in the custom field array. Bear with me while I think out loud here. As you point out, getCustomFields() grabs the ‘max’ custom field count — say it’s 14, made up of 1-10, plus 11, 12, 15, and 16 because 13 & 14 have been deleted — and only iterates up to this number. The fact that it checks if the custom_n_set is empty before populating the array is only half the issue; worse, it will only count as far as — in this case — 14. So the CF array will only contain 12 items; and items 15 & 16 will never be ‘seen’.

Not only is the warning issued by TXP because it only checks if the custom_n_set is empty and doesn’t check if it exists first (it should probably do that), it misses anything with an n > $prefs['max_custom_fields']. Does that about sum it up?

If optionally organising the prefs array by event is what you want and will allow you to build a better plugin, I can do that for you no probs. We should probably fix the getCustomFields() function too so that it uses actual custom_n_set fields and not computed ones based on max_custom_fields, though as Robert rightly said, we’d need to do that really efficiently; hmmm, that’ll require some brain power (any cool ideas?). Under this scenario, of course, it begs the question over the efficacy of $prefs['max_custom_fields'] at all!

Last edited by Bloke (2010-03-22 09:56:10)

gerhard · 2010-03-22 10:32:13

That’s a brilliant solution for get_prefs()!

I wouldn’t need to count anything with the new approach, neither would getCustomFields(). Rather than using a for loop to iterate over custom fields, I would use a foreach with $prefs['custom'], like this. A for could still be used, but I think that foreach would be clearer. Checking if a custom fields exists wouldn’t be necessary because get_prefs() already handles that.

Yes, $prefs['max_custom_fields'] would become redundant with prefs organized by event.

Bloke · 2010-03-22 10:48:23

gerhard wrote:

That’s a brilliant solution for get_prefs()!

I have my moments :-)

I wouldn’t need to count anything with the new approach, neither would getCustomFields().

Hadn’t thought of that, thanks for the code. It does add a query to grab the prefs a second time so we’d have to check if that has an impact in terms of page performance; not sure yet. Thinking trousers are on.

A for could still be used, but I think that foreach would be clearer.

Agreed. For the ordered parameter I was actually thinking more along the lines of group_by. With very little overhead to the function it could be possible to offer you the choice of grouping the prefs by ‘event’, ‘type’, ~~or ‘user_name’~~. Not sure *shrug* might be useful for some people, and it’s not much hardship over only allowing ‘event’.

Not sure if ordering by ‘html’ or ‘position’ is of any value so I’d probably not permit those. Whaddya think?

Last edited by Bloke (2010-03-22 10:55:29)

gerhard · 2010-03-22 11:07:17

Yeah, there already are 2 queries, adding a 3rd one would make get_prefs too complicated.

What about introducing a new get_ordered_prefs()? Eventually, it makes sense to transition from a single big array to a more ordered one (ie namespacing). getCustomFields() could be the first function to use the new prefs structure, other parts of TXP could transition to it as and when.

Also, you suggestion about ordering is a very good one. Again, get_ordered_prefs('event') would make most sense. I would keep this as simple as possible though, adding more options at this point would be “premature optimization”.

Bloke · 2010-03-22 12:12:02

gerhard wrote:

What about introducing a new get_ordered_prefs()?

Could do. Not sure which is the best way to go actually; a dedicated function that allows migration as you suggest, or simply altering the existing get_prefs() like this.

I don’t think it slows things down significantly; get_prefs is called once on the public side and twice on the admin side (three times if you’re on the Diagnostics tab). Do you think a dedicated function is better to maintain current performance?

I do kinda wish the two queries inside txp_prefs could be rolled into one, but the requirement to allow global prefs to override like-named user prefs means this isn’t possible. Ummm, I don’t think. At least, if it can be done I don’t know how to do it.

Hope this isn’t going too OT in your support thread, sorry.

Last edited by Bloke (2010-03-22 12:13:22)

Bloke · 2010-03-22 12:44:23

On an actual support topic note, in this version 1.2.4 of glz_cf if I alter the order of the custom fields a few things that I consider unexpected¹ happen. Say I decide to move custom_11_set to position 1 and custom_12_set to position 2 by setting their Position values to 1 and 2, respectively:

custom_11_set appears in the 2nd custom field slot on the Write tab, after custom_1
custom_12_set appears in the 4th custom field slot on the Write tab, after custom_1, custom_11, and custom_2
I’m unable to delete custom_11 and custom_12 until I renumber them to something > 10
The Position column on the Extensions->Custom Fields tab shows, from top to bottom: 1, 1, 2, 2, 3, 4, 5, 6, 7, 8, 9, 10
I’m allowed to delete custom_9 and custom_10 because they’ve “popped out” of the top 10

Bug, feature or user error?

¹ Your expectations vs mine may differ :-)

mlarino · 2010-03-27 00:35:39

Not sure if this is a bug, but I just noticed a little problem with Checkboxes.

I have a custom field with 28 checkboxes, and I can only check 21 of them.
If I check more than 21, when I hit save in the write tab when it refreshes only 21 are checked, and the ones on the bottom get uncheck.
Wierd… not sure if its a user problem, can someone else test this?

this is the content in my check box custom field:

Acceso para minusválidos
Aire Acondicionado
Alarma
Amueblado
Animales permitidos
Ascensor
Aseos
Balcón
Calefacción
Chimenea
Conserje
Cocina amueblada
Doble ventana
Escaparate
Exterior
Garaje
Hilo musical
Jardín
Parcela cerrada
Piscina
Puerta de seguridad
Riego automático
Porche
Salida de gases
Terraza
Trastero
Video Portero
Vistas

uli · 2010-03-27 00:45:00

mlarino wrote:

If I check more than 21, when I hit save in the write tab when it refreshes only 21 are checked, and the ones on the bottom get uncheck.

You’re right.

datorhaexa · 2010-03-31 16:25:54

Went through the entire thread but I couldn’t seem to find any information on if there’s a limit to the number of chars in the textarea? Or, is it without a limit?

Pat64 · 2010-04-01 06:23:28

Hi Gerard ;)

I’m just reporting a bug:

I created 12 custom fields. Then, decided to remove someone. Action on “Delete” buttom was fine except on the 11Th custom field!
Go back to “article” tab, and I saw a PHP error reporting like this “variable for custom_11 not defined in /include/article.php”. Looking throught my data base, and I saw the field “max custom field” in prefs table was set with the number 11.

So it seems that when we remove all custom fields the “max custom field” value isn’t changed into the “prefs” table.

Cheers,

Edit: Oops, don’t read Stef post, sorry. That’s exactly the same problem man.

Last edited by Pat64 (2010-04-01 06:25:29)

Bloke · 2010-04-01 11:54:38

Pat64 wrote:

Oops, don’t read Stef post, sorry. That’s exactly the same problem man.

Since I’ve had no real resistance to my custom fields patch proposal I’ll go ahead and submit it sometime this weekend. That means, from the next TXP version onwards, gerhard no longer needs to rely on the max_custom_fields value and can instead call the native getCustomFields() function to get an array of the actual custom fields in use, contiguous or not. That should put an end to the errors on the admin side and should make custom fields more robust going forwards.

Textpattern CMS

Textpattern CMS support forum

#685 2010-03-21 15:48:15

Re: glz_custom_fields

#686 2010-03-22 09:55:20

Re: glz_custom_fields

#687 2010-03-22 10:32:13

Re: glz_custom_fields

#688 2010-03-22 10:48:23

Re: glz_custom_fields

#689 2010-03-22 11:07:17

Re: glz_custom_fields

#690 2010-03-22 12:12:02

Re: glz_custom_fields

#691 2010-03-22 12:44:23

Re: glz_custom_fields

#692 2010-03-27 00:35:39

Re: glz_custom_fields

#693 2010-03-27 00:45:00

Re: glz_custom_fields

#694 2010-03-31 16:25:54

Re: glz_custom_fields

#695 2010-04-01 06:23:28

Re: glz_custom_fields

#696 2010-04-01 11:54:38

Re: glz_custom_fields

Board footer