As the title says, the class is extending from JFormField and should thus have a JFormField prefix, resulting in JFormFieldAbstractlist, if there is even a use for an Abstractlist class, when we already have JFormFieldList that up till now most fields extended from. Seems an unnecessary change for me.
This is part of a code review of the fields feature. See #13222, #13223, #13224, #13225, #13226 and other connected issues.
Labels |
Added:
?
|
please see this naming issue too:
Radios / Checkboxes / List (Select),
-- have in their configuration "options" that are comprosed:
Text+Value (and in future with: ...+property1+property2)
I commented that drupal and HTML calls these "options"
But the above was renamed to "values" by this PR
#13116 (comment)
Sorry, I don't get the comment from you @laoneo. You've taken JFormFieldList and moved its content to a new class that is one level below JFormFieldList in order to not force all fields that extend from JFormFieldList to be fields in com_fields. That might work for the core if you ignore that this means that suddenly existing classes extend from another baseclass, but this does not work for third party fields. This change is not backwards compatible. People are extending from JFormFieldList and the way the code is now, all third party fields that extend from that class, are forced into com_fields as field types. That change has to be reverted and a new field type that extends from JFormFieldList needs to be implemented that implements the interface and existing fields that extend from JFormFieldList need to have the "implements " added to them. The way this is now, this is a break in our B/C commitment.
@ggppdk I don't understand your comment.
@Hackwar
This issue that you opened, is about proper naming of the class that is extended by fields:
(select) list , radios, checkboxes
I speak of naming issue in the interface of the same fields,
they have a configuration parameter "Select values" which should be "Select options"
I should have posted a picture ..., here is the picture
I argue term "values" is improper / miss-leading, and argue that HTML and drupal and others call them "Options"
ok, now I understand. I have to admit that I don't really care a lot about the translated strings, because in the end those can be changed whenever you want. We wouldn't really be breaking backwards compatibility if we change this from values to options. Changing interfaces, classes and method names on the other hand can not be changed if we release it without another major release and that again would also bring bad press, because we would be breaking stuff, even though we already know now that we will have to change it.
Title |
|
Title |
|
@laoneo The thing is that if someone is doing a typecheck of the field during rendering, then this will break since some fields that used to be extending from JFormFieldList are no longer. Eg this code:
if ($field instanceof JFormFieldList)
{
// Do something specific for selects like adding a class or whatever
}
So this indeed is a valid B/C break and we should look for another way to determine if a formfield should show up as custom field or not.
That's an argument, but how likely is this the case? If we can come up with a better approach, then I'm fine changing it. This one was the only one I could find to put it into form fields as on DPFields I had my own classes.
It's not that adding stuff is a B/C break, but if you're moving API methods around in a way that breaks inheritance in the API as it was before the new features were added or you've changed the class inheritance chain then you have broken B/C. It's why #13163 can't be accepted into CMS 3.x or Framework 1.x; that change breaks the inheritance from a parent class and as pointed out breaks is_a()
or instanceof
checks.
but how likely is this the case
It doesn't really matter how likely it is, it's a B/C break and we are aware of it. We said to not do that. We made some exceptions for security reasons but this isn't justified here.
I haven't looked yet at how exactly the list of available formfields is generated. Would it be possible to have a property or method in the formfield which can be set to hide the field? Eg isCustomField
or so?
The problem is that you don't have an instance available. The check if the field is implementing the required interface is done here https://github.com/joomla/joomla-cms/blob/staging/administrator/components/com_fields/models/fields/type.php#L98.
As it breaks BC, we should label is as release blocker or?
I think that the way the supported fields are discovered is okay that way, but right now we can't take the Joomla core field classes and attach the interface to them directly. A solution could be to add a folder to com_fields that contains empty classes that extend the core field types and which implement the interface. That way we would have that cleanly seperated. For Joomla 4.0, we could remove those, make field types elligible for com_fields by default and turn this around, adding an interface that marks a field as not valid for com_fields.
Labels |
Added:
?
|
@laoneo Does this code also load 3rd party component formfields? I think only the ones added as plugins, right?
A solution could be to add a folder to com_fields that contains empty classes that extend the core field types and which implement the interface.
If you go that line, you could as well just have a hardcoded list of core formfields in the code. No need to have empty stubs for that.
Does this code also load 3rd party component formfields?
@Hackwar you are mainly suggesting the way I did it in DPFields https://github.com/Digital-Peak/DPFields/tree/master/com_dpfields/admin/models/types. I was requested in joomla-projects/custom-fields#104 to merge it into existing form fields.
@Bakual If you take the interface and stick it on any core field class, you are forcing all third party fields that extend from this class to "show up" as a field type. I know that they don't magically show up because we first have to include them in some form or way, but there are more than enough extensions out there that simply always include their own fields folder upon onAfterInitialise. B/C is a bitch, but if we mean it, we will have to go such a route in 3.x and will have to clean it up in 4.0. More reason to finally get people to work on that. I wouldn't know of a thing that you could do that marks a field class as eligible to be used in com_fields that is not automatically inherited by an extending class.
Hmm, I think that doesn't work as my custom formfields don't show up. Probably because it should take the context from userstate instead of request.
If you take the interface and stick it on any core field class, you are forcing all third party fields that extend from this class to "show up" as a field type.
I'm aware of that, and I don't see it as an issue by itself. It's not a B/C case. You can see it as a bug in a new feature but it doesn't break anything.
I start to think that deciding if the class is available for com_fields or not based on an interface is not the ideal approach, because either way we get an issue:
On the other hand with a class property it would be possible to change the behavior of childclasses. At the expense that we need to instantiate each FormFieldClass. Which may break things if that class requires something that isn't available.
Base class (eg JFormFieldList) implements an interface which enables com_fields. Now all fields extending that class are also NOT available for com_fields, without any way to opt IN.
This is not the case as they can just implement the interface and they will show up.
But I think I found a workaround for the 3.x series. We can put the abstract class code back to JFormFieldList and remove the interface from the list class. Then we create a new class JFormFieldCFList in administrator/components/com_fields/models/fields which implements the interface and we will have the list type then trough JFormFieldCFList. In J4 we can then revert back, when we think that this workaround is too ugly. I'm aware that it is not optimal but it should work.
Sorry, wrote it wrong. I meant in the second case the interface would disable the field, not enable.
Using interfaces only works if the base class doesn't implement any interface. As soon as it implements one, all children will inherit that behavior with no way out.
Then we create a new class JFormFieldCFList in administrator/components/com_fields/models/fields which implements the interface and we will have the list type then trough JFormFieldCFList.
That's quite ugly actually and imho doesn't solve the real issue. First it's not only the list formfield, there are many of them which you would have to create such workarounds.
But then what happens with J4? You face the exact same issue again. What happens if I extend JFormFieldList in my extension but don't want it to show up as custom field? If JFormFieldList implements the interface, I can't opt out anymore. You would have the exact same issue again.
Thus I don't think it works using Interfaces.
It may be an approach to have custom fields not automatically detected and instead use plugins to create all of them. You'd then need one plugin for core fields where you return the list of available fields and each extension would have to provide a plugin as well to enable their own fields (if there are any that make sense). The administrator could then even use the plugin parameter to enable/disable selected field types. Just a thought.
This approach was suggested by @bembelimen on Joomla day Austria to me. I like it very much and it would make things even more clear and configurable. The only question remains then, where we put the Dom logic? Should it remain in the form field or in a separate class as in DPFields?
See #13300 for the issue that currently component form fields aren't discovered at all.
Obviously if we change the way fields are loaded, that one isn't needed
I have no opinion on where that method should reside. Could it be used outside of com_fields (eg by extensions or JForm)? Then it makes sense to ahev it in the field. If it is only useful for com_fields, then it may as well be moved to com_fields.
It basically adds the possibility to create a Dom element for a stdclass. It has no specific connection to com_fields but on the other side, I don't see use cases where this code should be used other than in com_fields.
Then I'd move it to com_fields. We're more flexible in adjusting it if needed there :)
We can make it as a trait
Back to the roots then.
Milestone |
Added: |
@Hackwar please have a look on pr #13319. It will revert the JFormField to it's original state. All the custom field type specific code is moved to the plugin itself. Like that we can then eliminate the JFormAbstractlist and revert that to it's original state. All custom field types like text, etc. are then encapsulated into it's own plugin.
Yep
Status | New | ⇒ | Closed |
Closed_Date | 0000-00-00 00:00:00 | ⇒ | 2017-01-01 13:37:33 |
Closed_By | ⇒ | Bakual |
Labels |
Removed:
?
|
This change was necessary as only fields which do implement
JFormDomfieldinterface
are ready to support custom fields. As many field types need the list behavior but should not appear as custom field type, it was necessary to move the list logic out of the list.I can't relly remember, but there was a reason why I put it not into the fields folder, but can't remember.