Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Avoid creating invalid custom_field names #456

Closed
vanmelick opened this issue Dec 7, 2014 · 7 comments
Closed

Avoid creating invalid custom_field names #456

vanmelick opened this issue Dec 7, 2014 · 7 comments

Comments

@vanmelick
Copy link
Contributor

See:
http://forum.textpattern.com/viewtopic.php?id=43726
http://www.textpattern.net/wiki/index.php?title=Advanced_Preferences#Custom_Fields

Instead of having a list of "do not use" names for the custom_fields in the Wiki, it would be more userfriendly if TXP rejected those names when a user tries to create a custom_field with such a name. Not doing so results in problems that the average user cannot diagnose himself.

The way custom_field names are handled right now when parsing attribute values is really just an ugly hack. An alternative, which would break some of the current installs, is to no longer support the use of user-supplied custom_field names as attribute names.

@Bloke
Copy link
Member

Bloke commented Dec 7, 2014

I'm hoping the new custom field implementation will largely sidestep this. The rough plan is:

  • To (internally) prefix anything related to custom fields with a fixed string (e.g. txp_cf_). In other words, very crude namespacing. This includes txp_lang strings that are used to display localized versions of CF names.
  • Custom fields have Names and Titles. Names are dumbed-down ascii representations of Title (internally prefixed, as mentioned above).
  • When referring to custom fields you can do so by (dumbed-down) Name, prefixed name or by its ID number if you prefer. It is expected that Names will be preferred in most cases, as this will aid readability and portability.

The fact that the names are wholly represented without spaces or special characters means that <txp:article_custom /> can take attribute names without the current issue of having to use non-space characters to permit article filtering. So any of the following would be valid:

  • <txp:article_custom price="50" />
  • <txp:article_custom txp_cf_price="50" />
  • <txp:article_custom custom_12="50" />

That unfortunately still doesn't completely bypass the issue of what happens if you give a custom field exactly the same name as an attribute. For example, naming a field sort, which often catches people out:

  • <txp:article_custom sort="50" /> : still likely to fall over
  • <txp:article_custom txp_cf_sort="50" /> : fine
  • <txp:article_custom custom_11="50" /> : fine

I'm unsure if catching every possible attribute or database field name and rejecting it at creation time is particularly practical, long-term (I would be willing to be proved wrong if this is doable). There may be a way to be clever about it and determine that '50' isn't a column name so assume a CF was intended, but that's a little risky (and probably slower).

A way round it is to simply forbid direct CF names as attributes in <txp:article_custom />. Perhaps only permitting prefixed names (or custom_N) declares intent and means there's no possibility of clashes, at the expense of a little extra typing. Prefixed names would still be more portable than numbers if moving from one installation to another, so a design goal is to only have numbers as a fallback.

That leads to one other thing I'd like to do: make it possible to use <txp:article_custom sort="price asc" /> and have it figure out that you mean the field custom_11. That should be eminently doable with the new table structure and classes, as the internal data structures expose both name and number, so its easy to map between the two as needed.

In all cases, the Title is used for display purposes only, subject to the currently in-force language.

Any further thoughts on this, please chime in here. There may be a better way to handle this.

@bloatware
Copy link
Member

I would

  • allow for custom_n attributes as alternative to cfname. These are unambiguous (unlike dumbed-down names) and users are already using them in sort.
  • still allow for cfname attributes, but ignore them as cf values when they clash with tag's attributes like sort etc.
  • internally store, say, custom_11 as $thisarticle[11]. This avoid eventual variables pollution on extract($thisarticle), which we should avoid but still have in few places.

I'm currently testing the first two points and it looks doable.

bloatware added a commit that referenced this issue Jan 4, 2023
bloatware added a commit that referenced this issue Dec 23, 2023
@bloatware
Copy link
Member

Not sure we can do more in pre-CF branch. As long as it is not intended to be used as article filter, sort is a valid cf name. Close after warning strings addition/translation?

@Bloke
Copy link
Member

Bloke commented Dec 23, 2023

I'll give this a whirl. Looks good from a cursory glance at the code. Nice work! I'll sort the strings out.

@Bloke
Copy link
Member

Bloke commented Dec 23, 2023

Firefox doesn't give the most useful default experience if you try and make a custom field with a reserved word. "Please match the requested format" is a bit obtuse, haha. But there's not much we can do about it when using the pattern attribute.

CF-FormatMatch

Better than letting it through and then it causing issues later.

@Bloke
Copy link
Member

Bloke commented Dec 23, 2023

How do we feel about the string Reserved word in custom field name(s): <strong>{list}</strong>?

Can anyone come up with anything better? Bearing in mind this string can appear in the announcement box when saving Prefs, and the pre-flight check on Diagnostics, so it needs to supply context.

Is Reserved word as custom field name(s): <strong>{list}</strong> better? Or something else?

@bloatware
Copy link
Member

Firefox doesn't give the most useful default experience if you try and make a custom field with a reserved word. "Please match the requested format" is a bit obtuse, haha. But there's not much we can do about it when using the pattern attribute.

A bit of JS could probably fix it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants