? Pending

User tests: Successful: Unsuccessful:

avatar Bakual
Bakual
13 Dec 2016

Currently, com_fields uses an alias field as a name for the field in forms. But that alias really isn't used for anything else and can as well be substituted with the id, making the code actually simpler.

Summary of Changes

Removes the alias from fields and changes its use to use the ID instead.
To avoid conflicts in general, fields are now stored in an own group called "com_fields" instead of the "params" that is used already by most extensions.

Testing Instructions

Test creating, editing, deleting fields both in backend and frontend.
Test for fields in articles, users and contacts.

Documentation Changes Required

None.

avatar Bakual Bakual - open - 13 Dec 2016
avatar Bakual Bakual - change - 13 Dec 2016
Status New Pending
avatar joomla-cms-bot joomla-cms-bot - change - 13 Dec 2016
Category SQL Administration com_admin Postgresql com_fields Front End com_contact com_content com_users Installation Layout Libraries Plugins
avatar Bakual
Bakual - comment - 13 Dec 2016

@laoneo This should also fix #13179 because it just removes the "generateNewTitle" method completely. Imho the fields don't need to automatically change the title with a batch copy and thus I removed it.

avatar ggppdk
ggppdk - comment - 13 Dec 2016

Yes indeed it is simpler, but there are several advantages when using aliases

Most notable are:

  • custom layouts referencing the fields and the field values via aliases, are easier to read
  • you can later assign the alias to a field with a different ID, and anything referencing the field via the alias, will not need to be updated (i mean configuration referencing the field via alias instead of ID)
avatar Bakual
Bakual - comment - 13 Dec 2016

The alias is only used in the form, not when the field is displayed in a regular view. So it would only be useful for customising the edit views.
There is also already a class that can be set per field. Both independent for the form and for the regular view. I'd think that class is much more useful for customising.

avatar ggppdk
ggppdk - comment - 13 Dec 2016

The alias is only used in the form, not when the field is displayed in a regular view. So it would only be useful for customising the edit views.

right, i see

so either it should be used more or be removed as this PR does

avatar laoneo
laoneo - comment - 14 Dec 2016

I don't see the benefit really. The name of the field in the outputed HTML is now jform_2, which is hard for the site admin to imagine.

I like the change to move the fields away from the params fields. I would put that into a separate PR to not mix stuff and keep this one smaller.

avatar Bakual
Bakual - comment - 14 Dec 2016

I don't see the benefit really.

Main benefit is to remove a field that isn't needed at all. And to remove confusion as to what an "alias" does for fields. Because an alias is used for URL routing in all other forms, which makes no sense at all when it comes to fields (we don't have URLs for fields). So its named alias but does something completely different.
So it either should be removed or renamed to "field-id/name in forms". But the latter doesn't make sense to me as well. What is the benefit here when we already can add custom classes for styling?
Also, you would have to make sure each field name is unique in a form, or you get invalid HTML markup. That's simple with this PR since each ID is unqiue by its definition.

The name of the field in the outputed HTML is now jform_2, which is hard for the site admin to imagine.

It is jform_com_fields_2 for the ID and jform[com_fields][2] for the name.
And really, why does a site admin even care about the id/name of a field in an edit form?

I like the change to move the fields away from the params fields. I would put that into a separate PR to not mix stuff and keep this one smaller.

The move away from the params was a requirement for removing the alias. I saw that during writing the PR. That is the sole reason it is in this PR and not separate. We can split it of course to make reviewing a bit simpler, but it's the same thing to test in the end.

avatar laoneo
laoneo - comment - 14 Dec 2016

Some users of DPFields were working with the alias in template overrides, eg. @marcodings https://www.youtube.com/watch?v=fgDoZsD71H8&t=1652s

When we rewind the content parser feature joomla-projects/custom-fields#135 . Then it would mean that {{fields alias=avatar}} will not work anymore as well.

There was a reason that we introduced it in DPFields, so taking away all the stuff which is technically not obvious on the first sight makes for me no sense. You are right, the naming is wrong, but I would not remove it.

avatar Bakual
Bakual - comment - 14 Dec 2016

The thing is, if we now introduce stuff which isn't needed currently, we add technical debt that can't be changed later due to backward compatibility. It is simple to add a field (alias, name, whatever) again later if we ever introduce such a content parser feature. However that content parser would obivously work fine also with the ID of the field (as well as template overrides).
Same for the other fields that are of no use currently. We should not add stuff now just because it may or may not be supported later.

Imho we should keep it to the minimum we currently need and support and can be tested, and expand it later if we add the corresponding feature.

avatar laoneo
laoneo - comment - 14 Dec 2016

I think for a custom field which represents a JFormField it makes sense to have a form name (alias) attribute. It is in use right now as form name, just has the wrong label.

avatar Bakual
Bakual - comment - 14 Dec 2016

So we would have to rename it so it's clear what it does. Because currently it sure isn't clear and confusing.
However I still don't see the usecase where you would want to use that name/id for something. And especially not why that couldn't be done with the ID as well. Imho we're creating a lot of UI and code overhead for no gain.

Much more important than the formfield name would imho be the ID of the field when shown in frontend. Currently it shows there with an ID eg "field-entry-2". But if you think about this, this is already highly problematic. The field should have no ID at all. Because as soon as you want to show the fields on a page where multiple items are shown (eg one in a module and in component, or in a list), it will produce duplicate IDs on the page and thus rendering the HTML invalid. That doesn't matter if it's an alias or id that is used here. But that certainly is a different PR to remove that.

avatar marcodings
marcodings - comment - 14 Dec 2016

Thomas the definition of what is not used or is used is very subjective. We use (dp-)fields extensively and we use for example overrides and tags to assign fields to content.
Whilst i do agree we should be carefull about running up technical debt we should be equally reluctant in stripping out stuff just because as an individual we don't see the use case.
The stripping of tags will make it impossible for my to migrate from dp-fields to fields. Removing the ability to override fields, which as i understand it to be a consequence of this pr would be equally detremental.
Sure anything can be added later as bespoke code, however the whole point, to me was to have decent support from core

avatar Bakual
Bakual - comment - 14 Dec 2016

Thomas the definition of what is not used or is used is very subjective.

Of course I understand that. If someone can explain me why the need for an alias is there, then I'm fine with that.

I'm not sure I understand what you mean with "tags" and "overrides". This PR doesn't touch anything like that.
What it does is changing the name of the field in the item (eg article) form from using the alias to use directly the ID. So making the formfield name jform[com_fields][1] instead of jform[com_fields][alias].
As you may noticed it also changes the field group from jform[params][1] to jform[com_fields][1].

I don't see how that would change the ability to override the field or assign it to content.

Keep also in mind that this only changes something for the edit forms (eg article edit). For regular views like a single article page, the fields will behave exactly as before as the alias isn't used at all there.

avatar Bakual Bakual - change - 14 Dec 2016
Labels Added: ?
avatar anibalsanchez
anibalsanchez - comment - 19 Dec 2016

I have tested this item successfully on 18b9509

Test OK


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/13182.

avatar anibalsanchez anibalsanchez - test_item - 19 Dec 2016 - Tested successfully
avatar ggppdk
ggppdk - comment - 20 Dec 2016

Using ids as form field names, and when creating custom layouts is a little weird and problematic for portatibilty, imagine i want to copy i custom layout to another web-site i will have to remap the ids

about form field name, to give a comparison imagine:

article (/record) 'title' was field '1' and
article (/record) 'description' was was field '2'

I agree 'alias' term here is not proper, just use another term like 'name'

avatar laoneo
laoneo - comment - 20 Dec 2016

I agree on @ggppdk' proposal. I would also rename it to name and it will become more clear then.

avatar Bakual
Bakual - comment - 20 Dec 2016

I obviously disagree ?

Using ids as form field names, and when creating custom layouts is a little weird and problematic for portatibilty, imagine i want to copy i custom layout to another web-site i will have to remap the ids

First: We're only speaking about forms, not where the field is displayed in eg an article view. In that view it already uses the ID and not the alias. The alias is only ever used in a form generated by JForm. Are you really going to customise that one somehow? And why are you needing the id/name then and not take the class?

Second: You're exchanging one possible issue with another one. If the user is going to adjust the alias, it would break the custom layout as well. Personally I'd rather do the layout on something I'm sure the user doesn't accidentally change.

about form field name, to give a comparison imagine:
article (/record) 'title' was field '1' and
article (/record) 'description' was was field '2'

Imho it's not the same case as in a record the developer knows for sure how each field is named and the name has to relate to table fields, In our case, they are user generated and it doesn't really matter at all how they are named. Actually using the ID does again map to the database entry and makes the code much simpler.

avatar laoneo
laoneo - comment - 20 Dec 2016

A use case to use names in template overrides can be found here https://www.youtube.com/watch?v=fgDoZsD71H8&t=1652s. As I said already quite a bit of DPField users, did use the fields based on their alias, like

foreach ($item->fields as $field) {
    if ($field->alias == 'avatar') // do the work
}

They used then that layout among different sites and didn't had to mess with ID's. I understand you, that it technically makes sense to remove the alias. But I think we need to hear here the end user as well.

avatar Bakual
Bakual - comment - 20 Dec 2016

Please don't link to the video if you don't give the exact poisition where he explains the use case. I find it very hard to follow Q/A type of records to find the answer you're referring

As I said already quite a bit of DPField users, did use the fields based on their alias

That code wouldn't work because the alias isn't available in the formfield. They need to use $form->id (eg jform_params_alias) or $form->name (eg jform[params][alias]).
DPField users will likely need to adjust their overrides anyway (since we're likely changing the params part to com_fields).
Still I don't see why those overrides couldn't be achieved as well using the edit class property. Or if it for some reason has to be something unique per field and you don't want to add an arbitary unique class there, we could talk about adding the title property to the formfield so that one could be read for such code.

avatar laoneo
laoneo - comment - 20 Dec 2016

Sorry tought the t parameter was correct, but it always started from where I left. Here is the correct link https://www.youtube.com/watch?v=fgDoZsD71H8&t=26m5s.

avatar Bakual
Bakual - comment - 20 Dec 2016

Ah I see it now, but that is about frontend views, not form. The alias isn't used here for anything at all, it would just serve as a way to distinguish fields in custom overrides, but there are several other properties that can be used to achieve the same.
In eg an article view you could as well just use the title instead of the alias to achieve the exact same thing. Or use the render class.

avatar Bakual Bakual - change - 22 Dec 2016
Title
Remove alias from fields
[com_fields] Remove alias from fields
avatar Bakual Bakual - change - 22 Dec 2016
Title
Remove alias from fields
[com_fields] Remove alias from fields
avatar Bakual Bakual - edited - 22 Dec 2016
avatar ggppdk
ggppdk - comment - 23 Dec 2016

In eg an article view you could as well just use the title instead of the alias to achieve the exact same thing. Or use the render class.

Use a CSS class to reference a specific field ? that sounds a little strange

Drupal instead of "alias" or "name" calls it "Machine name", and looks like:
field_tags
field_color

and i understand why the "Machine", it is because this is not directly visible by site visitors / users

  • the "machine name" is used as the form field name:
<select name="field_color" ...

Then besides using it layouts / custom layouts it is also used to create a field specific class (the "render" class we already have can be used by many fields)

So in to total the "alias" or "name" or "field name" or "machine name" is useful as:

  • form field name
  • human readable and site-portable reference to a field (copy my layout file to other website)
  • create a friendly HTML TAG ID and / or friendly field specific CSS class (the "render" class would usually be not specific to a field)

About being uniqueness it should be on:
(context, alias)

avatar Bakual
Bakual - comment - 23 Dec 2016

Use a CSS class to reference a specific field ? that sounds a little strange

What is strange here? We do that all the time. Actually we use IDs very rarely in core.
The only time you should use an ID is when you're absolutely sure that ID is only shown once on a page. Or you have invalid HTML.
In a CMS you can only be sure of that when it comes to the template file (index.php) itself. Each content may be present multiple times on a page.

human readable and site-portable reference to a field (copy my layout file to other website)

You're aware we're still only talking about forms here. And I honestly doubt a lot of adjustments happen for those forms.
How the field is rendered in an article is a completely different thing and depends on the various layouts. Imho, it shouldn't use an ID at all here as this will break if you have the same field twice on the screen. Eg if someone wants to add fields to a list view.

create a friendly HTML TAG ID and / or friendly field specific CSS class (the "render" class would usually be not specific to a field)

As explained already, it's a bad idea to use the ID in most cases. For forms it may work because we usually don't have multiple instances of the same form open on the same page, but for rendered fields eg in an article it's a very bad idea to use IDs.

About being uniqueness it should be on:
(context, alias)

Alias has to be unique sidewide. Because even if you don't have the same form open twice, you may have different forms open on the same page (eg a login form in the module and a contact form in component). If both forms allow for custom fields and one field shares the same alias, then you already got a possible problem.

avatar Bakual
Bakual - comment - 23 Dec 2016

For reference: #13335 describes one of the current issues with using that alias (and without making sure it's unique).

avatar ggppdk
ggppdk - comment - 23 Dec 2016

Use a CSS class to reference a specific field ? that sounds a little strange
What is strange here? We do that all the time.

We speak of the 'render' class parameter, right ?
or i understood you wrongly ?
-- because it is a parameter, thus DB queries cannot reference it,
-- because it is not unique,
-- because it is not required parameter


it's a bad idea to use the ID in most cases. For forms it may work

No using IDs does not work well, it makes working with forms unfriendly

  • imagine that i write some JS code to manipulate the field it will be less readable using only the field ID, similar reason we do not usually use 1 letter names for variables, not readable
  • and if a graphical UI is created to manipulate the form layout, then i will have to use the field title which can be long and non unique (and the user should be able to switch to 'names' that are shorter and unique), and if use IDs it will be totally unreadable

You're aware we're still only talking about forms here

This is something that should change / can change with little effort,

-- It should also be usuable in viewing ('value rendering') layouts
(should be easy to index the fields of every record via their alias)

-- About CSS class that uses the alias (=field name),
this is really trivial to do, it can be added at the same place where 'render' class is added


About uniqueness, i see, maybe unique site wide, ok,
still i think it could do unique like this:
(context, alias)

And in places that it is needed,
e.g. to make HTML tag ID unique it can then be:

id="jform_custom_color_45" name="jform[custom][color]"

where 45 is the id of the field,

name=".....", only needs to be unique inside the form, and not in side the page or inside the iframe,
right ?


Whatever is decided, i explained some points in my above answers, about:
-- the usefulnes of keeping the alias and calling it "Field name"

avatar laoneo
laoneo - comment - 23 Dec 2016

A think we all agree, that alias is the wrong label. But I would not remove it.

avatar Bakual
Bakual - comment - 23 Dec 2016

We speak of the 'render' class parameter, right ?

No, the "edit class" (the one used in the form).
Yes using the table ID as ID attribute makes the formfields ID and name a bit less easy to identify when you want to write custom JS code (or CSS that relies on IDs). On the other hand the admin is not able to break that code by accidentally changing the alias.
Also, if that code is part of a custom formfield, then it's no issue at all. It's only an issue if you write that code somewhere in a template override. Chances are you're using development tools anyway and then it's just a copy-pasing of the ID. And honestly, are you really going to write JS in a layout override to adjust the behavior of a formfield? Shouldn't you better create your own custom formfield (which will be easy) and add the JS there? That sounds much more reusable to me than relying on the correct value of an alias property.

This is something that should change / can change with little effort,
-- It should also be usuable in viewing ('value rendering') layouts
(should be easy to index the fields of every record via their alias)

NO. When rendering (outside forms), the fields should not use any ID attribute at all because it will produce issues on a page as I explained multiple times already. So we actually need to remove the existing ids from the current output. But I'd like to keep this PR on the forms.

-- About CSS class that uses the alias (=field name),
this is really trivial to do, it can be added at the same place where 'render' class is added

What's the point in having both a "render class" plus an "alias" that is added to the class as well? Makes no sense to me.

name=".....", only needs to be unique inside the form, and not in side the page or inside the iframe,
right ?

Yes, but the way JForm works the ID and name is tied together. I don't think you can have different names there.

avatar Bakual
Bakual - comment - 23 Dec 2016

A think we all agree, that alias is the wrong label. But I would not remove it.

Yes, I think we all agree "alias" is a wrong label. I don't agree on the second part ?

avatar Bakual
Bakual - comment - 23 Dec 2016

Please see #13353 for one part of this PR that seems to be gone mostly unnoticed and probably we can all agree on that part.

avatar astridx
astridx - comment - 10 Jan 2017

I want to test this patch, but after applying it I get a failure if I want to open the fields.

Call to undefined method FieldsHelper::addSubmenu()
I am testing with the Joomla! version Joomla! 3.7.0-alpha2 dev. Is this the right version?

bildschirmfoto vom 2017-01-10 13-10-33

avatar laoneo
laoneo - comment - 10 Jan 2017

Yo need to use the latest staging code as many things have changed since alpha1.

avatar joomla-cms-bot joomla-cms-bot - change - 10 Jan 2017
Category SQL Administration com_admin Postgresql com_fields Front End com_contact com_content com_users Installation Layout Libraries Plugins SQL Administration com_admin Postgresql com_fields Front End com_contact com_content com_users Installation Layout Plugins
avatar Bakual
Bakual - comment - 10 Jan 2017

I've updated the branch and solved the conflicts. Hopefully I didn't miss something.

avatar astridx
astridx - comment - 11 Jan 2017

I tested this patch today.

I created a lot of field and applied this to categories and field groups. In the backend everything works fine. It was possible for me to create articles. All fields were showing in the correct groups and categories. I could edit this fields in the backend. And - if I forgot to fill a required field - I get a message.

But it was not possible for me to save changes in the frondend. I could edit the fields and when I save I get the message that everything is OK. But the changes have not been saved. I saw the unchanged field values. I made this changes with the super admin.

avatar Bakual
Bakual - comment - 11 Jan 2017

But it was not possible for me to save changes in the frondend. I could edit the fields and when I save I get the message that everything is OK. But the changes have not been saved. I saw the unchanged field values. I made this changes with the super admin.

@astridx It should be fixed with #13539. Can you make sure you have the latest staging in your installation? It was fixed only yesterday.

avatar astridx
astridx - comment - 11 Jan 2017

I have tested this patch successfully.

Sorry, you're right. I'm sure that I used the command
"Git pull upstream staging"
this morning.
Probably I have made a typo.
Now the saving in the frondend is OK too

avatar Bakual
Bakual - comment - 11 Jan 2017

I also updated the branch of this PR today. Maybe that fixed it as well. Patchtester only copies the files, it doesn't really merge them. So there may be some issues like that due to its workings.

Can you log a successfull test then in JIssues?

avatar astridx
astridx - comment - 11 Jan 2017

I have tested this item successfully on a9e3ad3


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/13182.

avatar astridx astridx - test_item - 11 Jan 2017 - Tested successfully
avatar franz-wohlkoenig
franz-wohlkoenig - comment - 14 Jan 2017

I have tested this item ? unsuccessfully on a9e3ad3

In "User":

  1. Link (works), not required,
  2. Calendar1, not required, had a Date, is erased und now empty,
  3. Calendar2, required. Date of this Field is shown above at "Calendar1".
    2

After changing Field-Position in Backend Position of Values don't move with Fieldname:

2

Test with Joomla! 3.7.0-alpha2-nightly, macOS Sierra, 10.12.2, Firefox 50.1.0, PHP 7.0.4, MySQLi 5.5.53-0


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/13182.

avatar franz-wohlkoenig franz-wohlkoenig - test_item - 14 Jan 2017 - Tested unsuccessfully
avatar Bakual
Bakual - comment - 14 Jan 2017

@franz-wohlkoenig That happens with and without this PR, right? At least I can reproduce it in clean staging as well. So it would be unrelated to this PR and a different issue needs to be opened.
I'd say the reason is because the label is shown even if there is no value saved for the field. In other places, we just don't show the whole field in that case.

avatar franz-wohlkoenig
franz-wohlkoenig - comment - 14 Jan 2017

@Bakual Tested on Article: New "Editor"-Field in Backend, filled with Value, saved. Open in Frontend, add Numbers in Field, saved > Value isn't saved. In Short: Modify and saving works in back-, not in frontend.

avatar astridx
astridx - comment - 14 Jan 2017

@franz-wohlkoenig
Do you have the latest version of staging?
As you can read in this conversaiton I had the same problem earlier.

avatar franz-wohlkoenig
franz-wohlkoenig - comment - 14 Jan 2017

Testing on Joomla! 3.7.0-alpha2-nightly.

avatar Bakual
Bakual - comment - 14 Jan 2017

The fix for the frontend issue has been merged 3 days ago. If your nightly is older, then it explains that behavior.

avatar Bakual
Bakual - comment - 14 Jan 2017

@franz-wohlkoenig See #13589 for the misaligned values.

avatar anibalsanchez
anibalsanchez - comment - 14 Jan 2017

I have tested this item successfully on a9e3ad3

Test OK


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/13182.

avatar anibalsanchez anibalsanchez - test_item - 14 Jan 2017 - Tested successfully
avatar franz-wohlkoenig
franz-wohlkoenig - comment - 14 Jan 2017

@Bakual update nighlty every morning.

avatar Bakual
Bakual - comment - 14 Jan 2017

@franz-wohlkoenig You were right, something was wrong there again. I have no rebased the branch with current staging and now it should work again.

avatar franz-wohlkoenig
franz-wohlkoenig - comment - 15 Jan 2017

I have tested this item ? unsuccessfully on eb8664f

Works with "User" and "Article".

In "Contact"

avatar franz-wohlkoenig franz-wohlkoenig - test_item - 15 Jan 2017 - Tested unsuccessfully
avatar Bakual
Bakual - comment - 15 Jan 2017

Please always test if the issues found are because of this PR or also in current staging.

avatar franz-wohlkoenig
franz-wohlkoenig - comment - 15 Jan 2017

I have tested this item successfully on eb8664f


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/13182.

avatar franz-wohlkoenig franz-wohlkoenig - test_item - 15 Jan 2017 - Tested successfully
avatar laoneo
laoneo - comment - 16 Jan 2017

Can we close this one in favor of #13576? There are some people who do need the alias/name field. So it should not be removed, but the name of the field alias field should be changed instead.

avatar Bakual
Bakual - comment - 16 Jan 2017

I still don't understand the need for it and I think it will produce more issues than advantages.
I'll leave that to the team to decide.
I agree that if this PR doesn't get accepted, the other one is the right approach and needs to go in.

avatar laoneo
laoneo - comment - 16 Jan 2017

There are three people just here who say, they need that field. So why then remove something which is needed by some?

avatar Bakual
Bakual - comment - 16 Jan 2017

So why then remove something which is needed by some?

Because they can do the same with IDs, or they didn't understand what this PR is about (this is about forms, not item display). And we otherwise make the code more complex for no gain.
I still don't believe anyone will do any template overrides for eg an article form specific to some custom field and then reuse that across other sites. And even if they do, it's still dead simple to adjust the number in the layout to the ned ID of the field.
On the other hand those same layouts suddenly all break as soon as an admin changes the name of the field, which I try to avoid here.

avatar Bakual
Bakual - comment - 2 Feb 2017

Closing as there doesn't seem to be much support ?

Please instead test

  • #13576 (Rename Alias and make sure it is unique)
  • #13353 (Don't store in params to avoid conflicts)
avatar Bakual Bakual - change - 2 Feb 2017
Status Pending Closed
Closed_Date 0000-00-00 00:00:00 2017-02-02 20:38:48
Closed_By Bakual
avatar Bakual Bakual - close - 2 Feb 2017

Add a Comment

Login with GitHub to post a comment