IP Address should not be listed as an option.

IP Address ascending/descending ideally should be listed above ID ascending/descending.

I have tested this item ✅ successfully on 42d4938

Works fine. The IP Address column is not displayed anymore if IP Logging options is set up on NO.

I don't know how it is done, but in another XML there is a requires attribute that maybe implemented here.

			<option value="rating_count ASC" requires="vote">JGLOBAL_VOTES_ASC</option>
			<option value="rating_count DESC" requires="vote">JGLOBAL_VOTES_DESC</option>
			<option value="rating ASC" requires="vote">JGLOBAL_RATINGS_ASC</option>
			<option value="rating DESC" requires="vote">JGLOBAL_RATINGS_DESC</option>

@Quy

1. IP Address was there because of a typo in the code. It's fixed now
2. Currently, our Form API is not flexible enough to allow adding new options to any position we want, it can only be added at the end, so we can only have it as how it's for now
3. The requires is a special handle(kind of hack) for List custom field type. requires="vote" mean the option will only available if vote plugin is enabled, see https://github.com/joomla/joomla-cms/blob/staging/libraries/joomla/form/fields/list.php#L137 . It should be used here

@Sandra97 Could you please help testing it again? Making sure the issue which @Quy mentioned is sorted ( IP Address column should not be displayed at all)

It works fine, when the IP Address column is not displayed, the IP filter is now not displayed either.

Great. Thanks for confirming @Sandra97

Hi, @joomdonation although this works, I think that the proposed direction/solution of @Quy is better?

A quick (working) mockup:
in file: ./libraries/joomla/form/fields/list.php

                                // Requires IP Logging enabled
                                if (in_array('iplogging', $requires) && !ComponentHelper::getParams('com_actionlogs')->get('ip_logging', 0))
                                {
                                        continue;
                                }

and in file: ./administrator/components/com_actionlogs/models/forms/filter_actionlogs.xml

                        <option value="a.user_id DESC">COM_ACTIONLOGS_NAME_DESC</option>
                        <option value="a.ip_address ASC" requires="iplogging">COM_ACTIONLOGS_IP_ADDRESS_ASC</option>
                        <option value="a.ip_address DESC" requires="iplogging">COM_ACTIONLOGS_IP_ADDRESS_DESC</option>
                        <option value="a.id ASC">JGRID_HEADING_ID_ASC</option>

I can do a PR for this (have it working on my test server), let me know what you think

Personal, I don't like this. It's not nice when a generic form field class (library) depends on a setting from a component. Not sure how others think about it @mbabker @laoneo

The form field already has depenendies on: multiplanguage (plugin), Association (plugin), adminlanguage (module) and vote (content plugin).
All core plugins, modules, components just like com_actionlogs.

That dependency is there for some reasons I don't know. I think it's not needed here (even if we want the added string in correct order, we have different way to handle in the code of this component if it's really needed)

That solution is technically wrong in many ways and should have never been accepted in the first place. The right solution would be a subclass of JFormFieldList with the extra logic specific to that use case.

@mbabker Do you want to add a new form field type here? Or the current code in this PR is fine?

IMO current code is fine if you're OK with the limitation of option ordering. Otherwise, a custom form field is needed to get things in the right order.

If it's really needed, we can still get it in right order by removing the other two options from xml file and add it dynamic here (like how we are adding ip address sort options).

If anyone really concerns with the ordering of these options, let me know and I will adjust the code. Otherwise, I will leave it as how it's.

I have tested this item ✅ successfully on 880cf40

I have tested this item ✅ successfully on 880cf40

I have tested this item ✅ successfully on 880cf40

RTC