What can be discussed is to merge the *HelperFields classes in the basic *Helper classes as they mostly exist already.

I think that's a good idea. Especially since we have some fields related stuff there already anyway (the sidebar links).

What do you think about making sure we always return a valid context? Currently we return the passed section if we're in backend without looking at it. May be worth to sanitise that and return a default one if the passed-in section isn't know.
Like if (!in_array($section, array('article', 'category', 'form')) {return 'article';}?

What do you think about making sure we always return a valid context

I would not as there will be a reason why another context is returned.

I would not as there will be a reason why another context is returned.

It is basically a user input (from the URL) you take there. It could be anything.
Shouldn't the component know itself which contexts are valid? At least for my extension and for core I can't think of a case where there would be another context passed which should be allowed to be proceed.

But then should null being returned and not the default context?

But then should null being returned and not the default context?

Depends what your code does with it. Ideally, you would return false if the context isn't valid and the whole process gets cancelled with an error message.
But we can also take a route where we just assume the user wanted to do the default action instead and work with a default context. I don't care much here.

Moved the functions to the component helper classes and returning null when the section is not known. Any more concerns ;)

Looks good to me. Thanks 👍

Could you test it?

I have tested this item ✅ successfully on 699974f

Can we PLEASE name the methods in a sane way? getRealSection() has no meaning for me at all. The comments are equally unhelpfull. We need method names (and attribute names) that are self-explanatory.

@Hackwar Any idea for a better name? The issue comes from the fact that frontend uses a different context for its form than the backend. So we need a way to map that context to the one used for fields (the backend one usually). Section means the second part of that context (taken from categories where it is component.section).

To begin with, the helper of the administration application should never be called in the frontend, so I don't see the reason for the check for the application in there. If you need to check both in front- and backend, create a helper in front- and backend. And then I would expect the forms to have the same context in front- and backend. Now that isn't the case right now, so we need a solution at least until we are at 4.0. I don't know the whole workflow of fields, but I would expect there to be a place somewhere in the model, where we can do a preprocess/postprocess step and where this can be implemented, instead of having such an extremely specific method. If you disagree completely here, at least name it something like "translateContext()" and hand in the full context. But as I said, rather drop the whole method and translate this somewhere in the respective model.

The way com_fields work, you can't use the model. It works using the various existing plugin events. The context of those events differ between front- and backend. If they should be the same or not isn't relevant here, fact is they are different and we need to map them.

One could argue indeed that this should be a frontend helper method, however there may also be cases where the context needs to be mapped in backend as well. Plus the plugin itself is the same for front- and backend. So the application check is either done in the plugin and the appropriate helper loaded or we do that check in the helper if it's needed. I think the check isn't really needed (but imho doesn't hurt) except for com_contact.
Personally as an extension developer I prefer having only one helper instead of doing duplicate work. It also keeps all fields specific code in said helper.

If you disagree completely here, at least name it something like "translateContext()" and hand in the full context.

What would be the purpose of the full context? The helper is called based on the first part of that context, so it will always be the helpers component. There is no point comparing the full context here, we only need the second part (section).
"translateContext" isn't appropriate either. At least myself I would expect it to translate something into another language. Imho, that method also could be useful for other extensions in future if they need to map the section between front and backend. So a name relating to com_fields would be wrong as well.

I'm still looking through the code of the fields feature, but right now I don't really understand why we are having these methods in there in the first place. To me it looks as if this could be done with a simple mapping array in the helper class of each component (where necessary) and one central method in the helper classes base class. All of that under the assumption that this is even a good way to solve the issue that you are trying to solve.

A method in the base class would likely work for most extensions I think. But especially com_contact is a beast as it has two different contexts (the contact and the mailform) and to make things interesting the frontendcontext "contact" relates to the mailform and in the backend it relates to the contact. That's why we need the check for the application in that specific case. I know, stupid context naming but we can't change that currently.

Again: Provide a class for the backend and one for the frontend and you don't have to do any checks here. It can simply be a mapping array instead.

What do you gain by having two helpermethods doing the same instead of one? Keep in mind that this check comes from a plugin, which means you just move the (optional) check for the application to the plugin to determine the correct helper. In the plugin it would be a required check.
For the developers it means creating a helper class with a (more or less) duplicated method.

The helper class itself is being used in frontend already anyway (eg for ACL stuff). So I don't see why you have an issue with it. And we several other instances where we load something from backend in frontend, that is not unusual.

It can simply be a mapping array instead.

What I do for my extension is like this:

$mapping = array(
	'sermon'         => 'sermon',
	'frontendupload' => 'sermon',
	'speaker'        => 'speaker',
	'speakerform'    => 'speaker',
	'serie'          => 'serie',
	'serieform'      => 'serie',
);
return isset($mapping[$section]) ? $mapping[$section] : null;

But it depends on the extension if it can be done that simple or not.

The code how it is now makes sense and is needed. What we can discus is the name and the documentation which should be improved. I'm up here for suggestions.

Any suggestions?

What about verifySection as function name instead of getRealSection?

What about:
getCustomFieldsSection()
or
getFieldsSection()

It may be useful for other cases as well in future. So I wouldn't tie it directly to com_fields.

The code how it is now makes sense and is needed.

yes it is needed , and i am happy to see it

It may be useful for other cases as well in future. So I wouldn't tie it directly to com_fields.

I was not aware of this, do you have an example ?

I was not aware of this, do you have an example ?

Each time you need to map the context used by frontend models to those used in backend you could need this method. I don't have a specific example in mind currently.

• When I see verifySection, then I expect to get a boolean back.
• getFieldsSection says for me not more than getRealSection.

Going a step back, what does the function do? It returns a section where fields are created for already. getMappedSection perhaps?

When I see verifySection, then I expect to get a boolean back.

Hmm, it's an argument.
JModelForm has a method validate() which returns the sanitised data. maybe we can use that term as it is already used for a smilar behavior? So validateSection?

getMappedSection could work as well.

validateSection sound's not too bad either.

Changed function name to validateSection.

I have tested this item 🔴 unsuccessfully on 5117825

When I try to add a field:

Call to undefined method FieldsHelper::addSubmenu()

@anibalsanchez Can you try again? It may have been an outdated branch here. I've updated it now with staging.

I guess @anibalsanchez needs to update his Joomla installation to the latest staging.

I have tested this item ✅ successfully on 11f3770

It seems to be working OK.

But, it is showing a notice. It could be related to a different issue:

Notice: Undefined property: stdClass::$assigned_cat_ids in ..../administrator/components/com_fields/helpers/fields.php on line 471

@anibalsanchez this should be solved with pr #13334.

This one can be put on RTC as only function names have changed, nothing else. For that we have two successful tests.