[com_fields] Use timezone and format of the calendar field

[laoneo]

Summary of Changes

When the Joomla timezone is set to none UTC, eg. Chicago, and a calendar form field is created for an article, then the time is moving backwards on every save of the article.

This is an upstream fix, reported by a DPFields user Digital-Peak/DPFields@375eee2.

Testing Instructions

• Set the global Joomla timezone to Chicage (or something else which is not UTC)
• Create a calendar form field for articles
• Set the format of the field to %Y-%m-%d %H:%M
• Edit an article
• Define a date in the custom field
• Save the article (not save and close)

Expected result

The date in the calendar custom field should stay as set. On the front end the same date with the same format of the plugin should be shown.

Actual result

The date is moving backwards. On the front, the date is shown with the database format.

[Bakual]

Imho, better would be to allow the filter to be set in the field. Default would be USER_UTC, the other option is SERVER_UTC and all other values basically disable the feature.
I think the issue is because the formfield defaults to USER_UTC if nothing is specified but there is no such default during the save process.

[laoneo]

I tried both filters and none of them work as expected. They are wrong IMO anyway as they are doing the opposite of what is expected.

[Bakual]

At least for core they do what is expected 😄
USER_UTC converts the datetime from the user timezone to UTC (saving) and back (display)
SERVER_UTC converts the datetime from the server timezone to UTC (saving) and back (display)

It is mostly useless when you want to store only a date. Eg a birthday shouldn't be changed based on timezones.
If you want to show the date and time of an event (eg article creation, television broadcast), then you need to adjust the time to the users setting or he will be off by some hours.

It depends on the usecase. That's why I think a parameter may be useful, depending on what the user wants to do here.

[laoneo]

IMO, it should be the other side, to assume the value comes in the user timezone and then convert it to UTC and not assume that the time which comes is in UTC and we need to convert it to the user timezone.

[Bakual]

IMO, it should be the other side, to assume the value comes in the user timezone and then convert it to UTC

That is what is done during save. The value is taken from the user, assumed it is in his timezone and then converted to UTC and saved as UTC in the database.
When displayed, it is converted back from UTC to the timezone of the user (which may be another timezone depending on user).

[laoneo]

It does the oposite, it takes the date and assumes it is in UTC and sets then the timezone and convertes it to the user tz on this line.

[Bakual]

Yep, that is during display. Because the data is coming from the database (UTC) and is shown to the user in his timezone.
The save part doesn't happen in the formfield, it happens in JForm: https://github.com/joomla/joomla-cms/blob/staging/libraries/joomla/form/form.php#L1310. That's where it gets converted from user timezone to UTC for saving into the database.

[laoneo]

Ok, now it starts to make sense. So why the time shift then as all should be ok, when no filter is set?

[joomdonation]

Actually, right now, during saving, no filter is applied for calendar custom fields. The value is saved as how it entered, then when it is displays, it is converted using USER_UTC

[joomdonation]

It seems there is no place for setting filter for calendar custom field at the moment. So during saving, no filter is being applied to the value

However, when the value is displayed again, USER_UTC is applied because no filter is set for calendar custom field https://github.com/joomla/joomla-cms/blob/staging/libraries/joomla/form/fields/calendar.php#L165

[laoneo]

But then it is a bug in Joomla as you can't do something on save and then do something different on load.

[joomdonation]

Yes. Seems to be a bug in JForm field calendar in this case

[Bakual]

There is a bug in behavior if the filter isn't set, yes. Not sure how to fix it without breaking anything, because the current default value USER_UTC goes back a long time and is even documented in https://docs.joomla.org/Calendar_form_field_type. Removing that default may have unexpected results.
However in JForm itself, we can't just add the same default because it is used in all fields.

So I think fixing this may have to wait until 4.0 where we can either make the filter a required attrbute or remove the default value for the filter.

[joomdonation]

How the custom fields values are being saved to database? Is it using the filtered/validated data from form object or using data directly from application input?

I ask the question because if we add the following command

$this->element['filter'] = $this->filter;

After this line

https://github.com/joomla/joomla-cms/blob/staging/libraries/joomla/form/fields/calendar.php#L165

Then the data is properly filtered using correct filter. However, I checked the value stored in jos_fields_values table and see that it is still storing unfiltered data, so maybe a bug in saving custom fields data, too?

[laoneo]

Good point, can be the problem that we are not sending the data trough the form validate here https://github.com/joomla/joomla-cms/blob/staging/plugins/system/fields/fields.php#L65 as I was pointing out here #13565 (comment)?

[joomdonation]

Yes, that's the reason of the problem. I don't know we support custom fields data filtering or not, but if you want to support filter like how it is supported in JForm, then we need to find a way to get filtered/validated data to store instead of getting data directly from input

[laoneo]

At the moment, the only way I see is to remove the onsave events in the system plugin and move the logic to the save function in the model. For example in DPCalendar, all the custom fields are available in the params field, so this point here is reached, which is the filtered/validated data.

[Bakual]

It would be great for sure to have the input data validated/sanitised. I'm not sure how easy that will be.

[joomdonation]

Not sure why Joomla doesn't pass the validated/sanitised data while triggering the event at https://github.com/joomla/joomla-cms/blob/staging/libraries/legacy/model/admin.php#L1192 , If it was, then the work would be much simpler

I don't know the custom fields code enough to see a solution :(. How about storing validated data back to input object before calling save method at https://github.com/joomla/joomla-cms/blob/staging/libraries/legacy/controller/form.php#L745

Something like

$this->input->post->set('jform_validated_data', $validData);

Then that validated data can be accessed via input object later?

[laoneo]

For example on articles is the problem that the params fieldsets are displayed on the edit form, but they are not sent with the object on the plugin save event because the field is attribs and not params as before. So for me would be the right approach to send also this data with the plugin event, otherwise all the params from other custom fields extensions don't get saved anyway.

[joomdonation]

Send that data via plugin event would be the correct way. However, do we have a way to code it so that it doesn't affect existing plugins process this event?

I think the other option would be use $app->setUserState like this https://github.com/joomla/joomla-cms/blob/staging/libraries/legacy/controller/form.php#L768

That would be safe option for Joomla 3? We pass data to plugin in the same way in com_users https://github.com/joomla/joomla-cms/blob/staging/components/com_users/controllers/user.php#L100

[laoneo]

I'm adding the params from the request as _params to the table when the params field doesn't exist on the table. Like that the data is filtered and validated, there is no need then to fetch it directly from the request anymore.

It is not possible to add it as $table->params, otherwise we get an error on save that the table doesn't contain a params field in the table.

This allows to set the filter to USER_UTC. Any feedback on my last change?

[joomdonation]

The changes should be OK. Haven't looked at the code carefully yet, but I have some questions:

1. In this line of code if (isset($data['params']) && !isset($table->params)), could you explain the reason of adding !isset($table->params) ?

2. In this line of code $table->_params = $data['params'];, is _params safe to use? I mean do we have to worry if existing code use that field before for whatever purpose ?

3. Seems in custom fields setup, we don't have place to setup the filter for field? If so, I think you will have to set it in the code to set filter for calendar custom field type to use USER_UTC by default?

[laoneo]

1. If $table->paramsexists, all is working as expected. No need to attach the params as it is done already on $table->bind.
2. Perhaps another check would make sense, see commit 6812621.
3. That's what I do.

[laoneo]

Please use my last commit as I the date is now also properly printed on the front. I'v updated the testing instructions as well.

@Bakual this would introduce a dependency in JModelAdmin to com_fields (there was once a discussion about that with com_categories 🙊), similar to tags. Honestly I would not like it. Params is used to add new fields so params should also being validated and filtered and made available on the item in some way.

[Bakual]

this would introduce a dependency in JModelAdmin to com_fields

You don't have to change anything in JModelAdmin. As you see in my other PR it works fine when using another property. All JModelAdmin does with "params" is in the getItem method it is converting it to a registry object if it exists, but that has nothing to do with validating a formfield.

[laoneo]

On both PR's we have the problem that the data is fetched from the request and not from the item in the plugin event as I'v pointed out here as well #13353 (review).

With my recent changes in this PR, the problem is eliminated with the _params property. In your PR, this must then be _com_fields on JModelAdmin (except when you give it a different name like _bakual).

[Bakual]

That's what I meant. We could just name it eg _com_fields instead of _params and don't have the reference to params anymore. They don't have to match any name, it's just the starting underscore which makes a different behavior if I'm right. No need to change anything in JModelAdmin.

[laoneo]

It doesn't get attached to the $item because there is no such field, that's the reason why we can't do it with _com_fields, see my change here https://github.com/joomla/joomla-cms/pull/13606/files#diff-39b4bc5cb02bbace2ee4febe8f54583eR1161.

[joomdonation]

OK. So I tested with two different formats (with and without parameters), two different timezones, the data is saved/displayed properly. So basically, this PR fixed the issue.

For the correct way of passing data from model to the plugin for processing, I will leave it to you two as you have been working on custom fields from beginning.

[Bakual]

Ah I missed that you have changed that in JModelAdmin. That doesn't look good.
Also you can forget the changeformat stuff, that's not going to pass. Why do you need that?
To me the proposed code looks like a big hack. There mus be a better way to fix this.

[laoneo]

Thanks for testing, please mark it as success here https://issues.joomla.org/tracker/joomla-cms/13606.

[laoneo]

That doesn't look good.

Eager to see a proper solution then. Hacks are in JModelAdmin all over.

Also you can forget the changeformat stuff, that's not going to pass. Why do you need that?

Because JDate needs a PHP date format and not this unreadable strftime format.

There mus be a better way to fix this.

Go for it.

For me getting the data unvalidated/filtered is also not an option. So this approach solves it at least in an understandable way.

[joomdonation]

How about my suggestions about passing data before:

I think the other option would be use $app->setUserState like this https://github.com/joomla/joomla-cms/blob/staging/libraries/legacy/controller/form.php#L768

That would be safe option for Joomla 3 and in Joomla 4, we can pass that data directly to event trigger? We pass data to plugin in the same way in com_users https://github.com/joomla/joomla-cms/blob/staging/components/com_users/controllers/user.php#L100

Use that approach will not only allow this plugin but also other plugins access to validated/sanitized data data, too.

[Bakual]

The display part shouldn't rely on the format set in the field. It should use one of the translated formats available in the language strings, see https://github.com/joomla/joomla-cms/blob/staging/language/en-GB/en-GB.ini#L303-L308.
That is how we display a date in each other location. Forms where an exception but with 3.7.0 there will be a solution here as well to use those translated formats.
We could add a parameter field where the user can select the display format. I do something similar in my extension using this code: https://github.com/Bakual/SermonSpeaker/blob/master/com_sermonspeaker/admin/models/fields/dateformat.php#L53-L72 to fetch the available formats.

Another and maybe better approach is to add the new parameters of the new calendar. There is a showtime toggle which enables/disables the time part of the calendar popup. It is also used in the translateFormat feature and would allow us to toggle between two translated formats (with and without time). That would likely solve most of the usecases for the display part and the other can easily do it in an override anyway.

So that would remove the need for the changeFormat stuff.

[laoneo]

Agree, an output format parameter would make more sense. What about adding the ones from the language strings and give the admin the ability to define a custom one?

The params issue is still not solved yet. @joomdonation, honestly I'm not a fan of getting the validated data from the user state. When check in failed, then it makes sense, but in our case, I'm not sure.

[Bakual]

What about adding the ones from the language strings and give the admin the ability to define a custom one?

We could allow a custom one, but imho it's useless in most cases and will not work for multilingual sites. I would just offer the existing ones and if he needs a different one he can either override the existing language strings or the layout. Both would be easy.

The params issue is still not solved yet.

I started to look into it but was stuck in the basement of a train station with only the laptop screen. So I was quite handycapped 😄

[dgrammatiko]

@laoneo @Bakual how about reverting to the old calendar and rename the new one as datetime and

• The js part will ALWAYS return the date as local (UTC + timezone)
• The field could use a param to either respect the timezone or just keep the UTC
  will this take us out of the current mess?

[laoneo]

No, datetime is already taken by DPCalendar 😁

The format is not a big deal, just the usual disagreement between @Bakual and me 🙊.

More importantly is how the custom field data get's all the way along to the onContentBeforeSave event that it gets filtered and validated.

[Bakual]

@dgt41 It's not an issue with the calendar, it's an issue within com_fields that just surfaces here. I'm sure we can solve it 😄

[dgrammatiko]

@Bakual @laoneo ok then, I'll mute myself here 😀

[Bakual]

I guess he thinks that we can pass a strftime time format to JDate::format.

See https://github.com/joomla/joomla-cms/compare/staging...Bakual:FieldCalendarPlugin?expand=1 for what I think :-p
This applies the user timezone plus applies a localised date format. The date format needs to be customised to a degree as I wrote above.

[Bakual]

See #13638 for my approach.

[laoneo]

Closing in favor of #13638.