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

[com_fields] Remove alias from fields #13182

Closed
wants to merge 7 commits into from

Conversation

Bakual
Copy link
Contributor

@Bakual Bakual commented Dec 13, 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.

@Bakual
Copy link
Contributor Author

Bakual commented Dec 13, 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.

@ggppdk
Copy link
Contributor

ggppdk commented Dec 13, 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)

@Bakual
Copy link
Contributor Author

Bakual commented Dec 13, 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.

@ggppdk
Copy link
Contributor

ggppdk commented Dec 13, 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

@laoneo
Copy link
Member

laoneo commented Dec 14, 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.

@Bakual
Copy link
Contributor Author

Bakual commented Dec 14, 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.

@laoneo
Copy link
Member

laoneo commented Dec 14, 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.

@Bakual
Copy link
Contributor Author

Bakual commented Dec 14, 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.

@laoneo
Copy link
Member

laoneo commented Dec 14, 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.

@Bakual
Copy link
Contributor Author

Bakual commented Dec 14, 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.

@marcodings
Copy link
Contributor

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

@Bakual
Copy link
Contributor Author

Bakual commented Dec 14, 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.

@Bakual Bakual force-pushed the RemoveAliasFromFields branch 2 times, most recently from 20bd4c5 to ce89009 Compare December 14, 2016 20:30
@anibalsanchez
Copy link
Contributor

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.

@ggppdk
Copy link
Contributor

ggppdk commented Dec 20, 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'

@laoneo
Copy link
Member

laoneo commented Dec 20, 2016

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

@Bakual
Copy link
Contributor Author

Bakual commented Dec 20, 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.

@laoneo
Copy link
Member

laoneo commented Dec 20, 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.

@Bakual
Copy link
Contributor Author

Bakual commented Dec 20, 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.

@laoneo
Copy link
Member

laoneo commented Dec 20, 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.

@Bakual
Copy link
Contributor Author

Bakual commented Dec 20, 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.

@Bakual Bakual force-pushed the RemoveAliasFromFields branch from 18b9509 to 1a7be2d Compare December 21, 2016 11:16
@Bakual Bakual changed the title Remove alias from fields [com_fields] Remove alias from fields Dec 22, 2016
@ggppdk
Copy link
Contributor

ggppdk commented Dec 23, 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)

@Bakual
Copy link
Contributor Author

Bakual commented Dec 23, 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.

@Bakual
Copy link
Contributor Author

Bakual commented Dec 23, 2016

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

@ggppdk
Copy link
Contributor

ggppdk commented Dec 23, 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"

@ghost
Copy link

ghost commented Jan 14, 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".
    1

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.

@Bakual
Copy link
Contributor Author

Bakual commented Jan 14, 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.

@ghost
Copy link

ghost commented Jan 14, 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.

@astridx
Copy link
Contributor

astridx commented Jan 14, 2017

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

@ghost
Copy link

ghost commented Jan 14, 2017

Testing on Joomla! 3.7.0-alpha2-nightly.

@Bakual
Copy link
Contributor Author

Bakual commented Jan 14, 2017

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

@Bakual
Copy link
Contributor Author

Bakual commented Jan 14, 2017

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

@anibalsanchez
Copy link
Contributor

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.

@ghost
Copy link

ghost commented Jan 14, 2017

@Bakual update nighlty every morning.

@Bakual Bakual force-pushed the RemoveAliasFromFields branch from a9e3ad3 to eb8664f Compare January 14, 2017 21:23
@Bakual
Copy link
Contributor Author

Bakual commented Jan 14, 2017

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

@ghost
Copy link

ghost commented Jan 15, 2017

I have tested this item 🔴 unsuccessfully on eb8664f

Works with "User" and "Article".

In "Contact"

@Bakual
Copy link
Contributor Author

Bakual commented Jan 15, 2017

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

@ghost
Copy link

ghost commented Jan 15, 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.

@laoneo
Copy link
Member

laoneo commented Jan 16, 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.

@Bakual
Copy link
Contributor Author

Bakual commented Jan 16, 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.

@laoneo
Copy link
Member

laoneo commented Jan 16, 2017

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

@Bakual
Copy link
Contributor Author

Bakual commented Jan 16, 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.

@Bakual
Copy link
Contributor Author

Bakual commented Feb 2, 2017

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

Please instead test

@Bakual Bakual closed this Feb 2, 2017
@Bakual Bakual deleted the RemoveAliasFromFields branch February 2, 2017 20:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants