Field calendar plugin #13638

Bakual · 2017-01-18T15:18:41Z

Pull Request for Issue #13606 .

Currently, when you have a calendar field the timezone with each save the timezone is added/substracted from the date, resulting in a shift of the date/time with each save. This is especially bad if you only want to store the date plus have a negative timezone (eg New York), then the date shifts one day back with each save.
Also when the date is displayed in frontend, it is off by the timezone as well.

Main issue is that the calendar formfield expects the value stored as UTC and applies the timezone when shown in the form. But currently, the value isn't converted to UTC during save which results in that shift.
Also the timezone isn't applied when the value is shown in frontend.

Summary of Changes

Using JHtml::date to show the date in frontend. This automatically applies the user timezone to the value.
Using JText::_() to show the date in a localised format. With this PR this format is hardcoded to DATE_FORMAT_LC4 which will only show the date. That likely needs to be improved but I'm not yet sure how. It's not directly in the scope of this PR and can be done later fine. Also overrides are possible to change that format easily.
The format parameter of the field is replaced with a showtime parameter. It will control if the time is displayed or not both in forms and when the item is shown in frontend. It takes translated format strings.
Introduces a new plugin event "onAfterDataValidation". Also adds a new "onBeforeDataValidation" event and deprecates the existing "onUserBeforeDataValidation". Here I need input from @wilsonge as he initially created this event with New Plugin Event #10751. Imho the name of the event is misleading as it has nothing to do with com_users (it's a general event).
During that new event, the fields plugin will take the validated fields data, store it in a private property and use it in the onContentAfterSave event to store it into database. The "onContentBeforeSave" method isn't needed anymore in the fields plugin.
Expanded the current onContentBeforeSave and onContentAfterSave events so they not only pass the table object but also the validated data. The fields data can then be taken simply from that $data array. I'm sure other extensions will find that useful as well.

Testing Instructions

Make sure editing field values works as expected.
Make especially sure calendar formfields don't change the value on repeated saves and that the displayed value is same in frontend and backend. Especially also check with negative timezones.

Documentation Changes Required

The ~~new~~ adjusted event needs to be documented on https://docs.joomla.org/Plugin/Events.

wilsonge · 2017-01-18T15:22:13Z

Good by me. Remember to update the onUserBeforeDataValidation event in com_users to note the deprecation - the idea was that event was a generic version of what @rdeutz did #10351 and I just forgot to change the name to be generic too

ghost · 2017-01-19T16:30:49Z

I have tested this item ✅ successfully on c02720d

Re-Saved Article in Front- and Backend with "Universal Time Zone", got same Date. Changing Time Zone to "Chicago" changed Date one Day back (from 2017-01-12 to 2017-01-11), re-saving in Front- and Backend dosn't change Date.

_{This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/13638.}

ghost · 2017-01-19T16:33:25Z

@Bakual wasn't able to get Time in Calendar-Plugin, default Value is %Y-%m-%d, there should also a Time Value possible.

Bakual · 2017-01-19T19:02:41Z

wasn't able to get Time in Calendar-Plugin, default Value is %Y-%m-%d, there should also a Time Value possible.

Yep, that's the part with the formatting in frontend. I need a solution for that (probably a new parameter).

@laoneo also raised a concern that we store data in a plugin property. I'm not so sure if it's an issue as we do a similar thing in the debug plugin.
The other approach would be to expand the onContentAfterSave event with the validated data. So instead of doing "onContentAfterSave($context, $item, $isNew)" we would do "onContentAfterSave($context, $item, $isNew, $data = array())". That would then allow to fetch the validated data directly in that event ($item only contains data matched to the database table).

joomdonation · 2017-01-20T15:33:34Z

The other approach would be to expand the onContentAfterSave event with the validated data. So instead of doing "onContentAfterSave($context, $item, $isNew)" we would do "onContentAfterSave($context, $item, $isNew, $data = array())".

To me, that would be better solution compare to the current one. However, there are two things;

If you pass validated data ($data variable) to the event trigger (this line https://github.com/joomla/joomla-cms/blob/staging/libraries/legacy/model/admin.php#L1192, correct ?) , for existing plugins which listen to that event, I guess there would be warning because it doesn't have $data variable in the method?
Since $this->event_after_save could be a custom one (for example onSubscriptionPlanAfterSave, onEventAfterSave...), if third party extension want to use this custom fields feature, they would have to write a new plugin to just save custom fields data?

So if we have to introduce a new event like in this PR, maybe we could trigger new event after this line https://github.com/joomla/joomla-cms/blob/staging/libraries/legacy/model/admin.php#L1192

$dispatcher->trigger('onDataAfterSave', array($context, $table, $isNew, $data));

Or

$dispatcher->trigger('onItemAfterSave', array($context, $table, $isNew, $data));

How do you think about it?

Bakual · 2017-01-20T17:00:27Z

If you pass validated data ($data variable) to the event trigger (this line https://github.com/joomla/joomla-cms/blob/staging/libraries/legacy/model/admin.php#L1192, correct ?) , for existing plugins which listen to that event, I guess there would be warning because it doesn't have $data variable in the method?

Yes, that line. And no, there is no warning. PHP doesn't generate a warning if you call a method with to many arguments.

Since $this->event_after_save could be a custom one (for example onSubscriptionPlanAfterSave, onEventAfterSave...), if third party extension want to use this custom fields feature, they would have to write a new plugin to just save custom fields data?

3rd parties need to trigger the onContent.... events anyway. Otherwise the fields plugin will not work. The plugin is already listening on several such events (including the onContentAfterSave one). So there will be no difference here. Only thing would be if the extension triggers the event in its own code, it will need to add $data to it as well. But they need to adapt their code to support fields anyway.

The only way this can be an issue imho is when a 3rd party extension already passed additional arguments to that event. I don't know how probable that is but imho it's the risk of the 3rd party if he does that.

I wouldn't add a new event right after the existing one. Either we add one in a place where there is currently none, or we adjust the existing one.

joomdonation · 2017-01-21T07:25:58Z

Yes, that line. And no, there is no warning. PHP doesn't generate a warning if you call a method with to many arguments.

Thanks, I didn't know that. I thought the number arguments must be equal.

I wouldn't add a new event right after the existing one. Either we add one in a place where there is currently none, or we adjust the existing one.

The reason I suggested to add a new event is because I thought that add new parameter to event trigger will causes backward compatible issues with existing plugins. Now I know that It is OK to add new parameters to existing event trigger, I will be happy if you add $data to event_after_save at this line https://github.com/joomla/joomla-cms/blob/staging/libraries/legacy/model/admin.php#L1192

Maybe add $data to event_before_save trigger at this line as well https://github.com/joomla/joomla-cms/blob/staging/libraries/legacy/model/admin.php#L1171

Bakual · 2017-01-21T08:44:57Z

Thanks, I didn't know that. I thought the number arguments must be equal.

I wasn't sure myself, had to try it and verify with StackOverflow 😄

Maybe add $data to event_before_save trigger at this line as well

Yeah, that was my thinking as well. Both events should have similar signatures.
I'll update this PR when I get some time (hopefully this night)

joomdonation · 2017-01-21T08:49:33Z

Great. I will test it when it is ready :)

Bakual · 2017-01-22T19:19:25Z

@joomdonation Removed the new events (will probably do a separate PR for those) and adjusted the existing save events to pass also the validated data.

joomdonation · 2017-01-23T07:43:12Z

plugins/system/fields/fields.php

-		$parts = FieldsHelper::extract($context);
-
-		if (!$parts)
+		if (!$data || !is_array($data) || empty($data['params']))


Maybe the code in this line could be simplified a bit to

if (!is_array($data) || empty($data['params']))

Otherwise, it looks good to me. I will test the behavior tonight my time and update the test result in the issue tracker

True, will adjust

laoneo · 2017-01-23T09:57:12Z

plugins/fields/calendar/tmpl/calendar.php

@@ -21,4 +21,4 @@
 	$value = implode(', ', $value);
 }

-echo htmlentities($value);
+echo htmlentities(JHtml::_('date', $value, JText::_('DATE_FORMAT_LC4')));


IMO the date format needs to be the same as for the calendar form field (where it is in strftotime) otherwise it will be too confusing for the end user.

That's not something new with custom fields. That's how we deal with dates all over Joomla.
Only the formfield itself uses strtotime because that is what JavaScript understands. When we display the date in an item, we always use the language strings together with JHtml::_('date'). See for example the "Created" date:
<?php echo JText::sprintf('COM_CONTENT_CREATED_DATE_ON', JHtml::_('date', $displayData['item']->created, JText::_('DATE_FORMAT_LC3'))); ?>
(https://github.com/joomla/joomla-cms/blob/staging/layouts/joomla/content/info_block/create_date.php#L16)

laoneo · 2017-01-23T09:59:04Z

I very like the approach. If there is a way to handle the formats consistently then the issue is solved properly.

Bakual · 2017-01-23T10:10:56Z

If there is a way to handle the formats consistently then the issue is solved properly.

Not in 3.x for sure. Maybe in 4.0 if we get a way that the JavaScript code understands PHP date formats and we can break B/C with all existing forms. 😄
The new translateFormat attribute may help with a transition since developers no longer need to explicitely specify the format in most cases.

laoneo · 2017-01-23T10:18:12Z

But then I would suggest that we offer in the custom field only the formats which are available for the front for the datepicker and and not a free text field.

Bakual · 2017-01-23T11:01:48Z

But then I would suggest that we offer in the custom field only the formats which are available for the front for the datepicker and and not a free text field.

Yes, that needs improvement anyway.
I think we could also use an approach where we make use of the new attributes of that field: "translateFormat" and "showTime". If we add a setting "Show Time" we could get rid of any format setting. The formfield would use those two strings:

DATE_FORMAT_CALENDAR_DATE="%Y-%m-%d"
DATE_FORMAT_CALENDAR_DATETIME="%Y-%m-%d %H:%M:%S"

And for the display part we can toggle between

DATE_FORMAT_LC4="Y-m-d"
DATE_FORMAT_LC5="Y-m-d H:i"

I think that would actually solve most use cases without becoming overly complex.

laoneo · 2017-01-23T11:04:00Z

Absolutely, one with time and one without.

Bakual · 2017-01-23T11:56:34Z

@laoneo I have now replaced the format parameter with a showtime parameter.
By default it will only show the date and the calendar popup doesn't have a time selector.
If enabled, the time selector will appear and the time will be shown both in forms and items.

What do you think?

laoneo · 2017-01-23T12:39:28Z

Why not just making a drop down with available the formats, for me there is no need to have a text field here as we are not supporting it anyway for the front end display.

laoneo · 2017-01-27T16:01:12Z

Can you please fix the conflicts, would love to see it in the first beta 😉

…lidation to more generic onBeforeDataValidation and deprecate old name.

…e need for onContentBeforeSave method

…lidation to more generic onBeforeDataValidation and deprecate old name. (reverted from commit 8552ab7)

…rted from commit 1ceeddf)

Will use translated formats for all cases (form and item display)

Bakual · 2017-01-27T17:24:29Z

Rebased and fixed conflict.

yvesh · 2017-01-28T19:54:12Z

plugins/fields/calendar/calendar.php

+ * @return  DOMElement
+ *
+ * @since   __DEPLOY_VERSION__
+ */


Indentation is wrong :-)

huh? Codestyle didn't complain and it's inline with other places afaik. I likely copied it anyway.

@Bakual Same in raw view. GH bug?

Ah now I see what you mean. I thought you meant something with that specific line is wrong but it's the whole docblock. Fixed niw.

laoneo · 2017-01-30T09:35:32Z

I have tested this item ✅ successfully on f3682a0

_{This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/13638.}

laoneo · 2017-01-30T09:37:03Z

@franz-wohlkoenig can you please test it again, as there have been many changes since your latest test. Thanks!

ghost · 2017-01-30T09:52:09Z

I have tested this item ✅ successfully on f3682a0

_{This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/13638.}

wilsonge · 2017-01-30T09:53:45Z

Well done everyone!

joomla-cms-bot added the PR-staging label Jan 18, 2017

Bakual mentioned this pull request Jan 18, 2017

[com_fields] Use timezone and format of the calendar field #13606

Closed

Bakual force-pushed the FieldCalendarPlugin branch from c02720d to 55d40ac Compare January 22, 2017 19:16

joomdonation reviewed Jan 23, 2017

View reviewed changes

laoneo reviewed Jan 23, 2017

View reviewed changes

joomla-cms-bot added the Language Change This is for Translators label Jan 23, 2017

Bakual force-pushed the FieldCalendarPlugin branch from 10235c1 to 703aabd Compare January 23, 2017 11:43

Thomas Hunziker added 5 commits January 27, 2017 18:21

Display localised dateformat and apply user timezone.

96d63b3

Set filter type for calendar fields plugin

c92de05

Add a "onAfterDataValidation" event to JModelLegacy::validate()

e93e925

Add new plugin event onAfterDataValidation. Rename onUserBeforeDataVa…

e66527f

…lidation to more generic onBeforeDataValidation and deprecate old name.

Store validated fields data in plugin for use during save. Removes th…

3250d52

…e need for onContentBeforeSave method

Thomas Hunziker added 9 commits January 27, 2017 18:21

Codestyle

0ececc4

Add new plugin event onAfterDataValidation. Rename onUserBeforeDataVa…

b3e58ab

…lidation to more generic onBeforeDataValidation and deprecate old name. (reverted from commit 8552ab7)

Add a "onAfterDataValidation" event to JModelLegacy::validate() (reve…

2c03e33

…rted from commit 1ceeddf)

Adding the validated $data to onContentBeforeSave and AfterSave event

a01f3f8

Use the validated data from the onContentAfterSave event

fd5d027

Pass $userData to $this->onContentAfterSave method

b67811a

Codestyle

5880235

Simplify check

9d229aa

Adding an option to show/hide the time in the calendar field.

5e7e0bd

Will use translated formats for all cases (form and item display)

Bakual force-pushed the FieldCalendarPlugin branch from 703aabd to 5e7e0bd Compare January 27, 2017 17:24

yvesh reviewed Jan 28, 2017

View reviewed changes

Codestyle

f3682a0

wilsonge merged commit 0fa990a into joomla:staging Jan 30, 2017

wilsonge added this to the Joomla 3.7.0 milestone Jan 30, 2017

Bakual deleted the FieldCalendarPlugin branch January 30, 2017 13:07

This was referenced Mar 4, 2017

[com_fields] Calendar Field Plugin options, Language string is missing #14343

Closed

Remove useless format parameter from custom calendar field plugin #14344

Merged

infograf768 mentioned this pull request Mar 10, 2017

Rewrite fields description in English #14447

Merged

wilsonge mentioned this pull request Dec 26, 2018

J4: Fixing bugs in menu manager #23293

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Field calendar plugin #13638

Field calendar plugin #13638

Bakual commented Jan 18, 2017 •

edited

Loading

wilsonge commented Jan 18, 2017

ghost commented Jan 19, 2017

ghost commented Jan 19, 2017

Bakual commented Jan 19, 2017

joomdonation commented Jan 20, 2017

Bakual commented Jan 20, 2017

joomdonation commented Jan 21, 2017

Bakual commented Jan 21, 2017

joomdonation commented Jan 21, 2017

Bakual commented Jan 22, 2017

joomdonation Jan 23, 2017

Bakual Jan 23, 2017

laoneo Jan 23, 2017

Bakual Jan 23, 2017

laoneo commented Jan 23, 2017

Bakual commented Jan 23, 2017

laoneo commented Jan 23, 2017

Bakual commented Jan 23, 2017

laoneo commented Jan 23, 2017

Bakual commented Jan 23, 2017

laoneo commented Jan 23, 2017

laoneo commented Jan 27, 2017

Bakual commented Jan 27, 2017

yvesh Jan 28, 2017

Bakual Jan 28, 2017

yvesh Jan 29, 2017

Bakual Jan 29, 2017

laoneo commented Jan 30, 2017

laoneo commented Jan 30, 2017

ghost commented Jan 30, 2017

wilsonge commented Jan 30, 2017

Field calendar plugin #13638

Field calendar plugin #13638

Conversation

Bakual commented Jan 18, 2017 • edited Loading

Summary of Changes

Testing Instructions

Documentation Changes Required

wilsonge commented Jan 18, 2017

ghost commented Jan 19, 2017

ghost commented Jan 19, 2017

Bakual commented Jan 19, 2017

joomdonation commented Jan 20, 2017

Bakual commented Jan 20, 2017

joomdonation commented Jan 21, 2017

Bakual commented Jan 21, 2017

joomdonation commented Jan 21, 2017

Bakual commented Jan 22, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

laoneo commented Jan 23, 2017

Bakual commented Jan 23, 2017

laoneo commented Jan 23, 2017

Bakual commented Jan 23, 2017

laoneo commented Jan 23, 2017

Bakual commented Jan 23, 2017

laoneo commented Jan 23, 2017

laoneo commented Jan 27, 2017

Bakual commented Jan 27, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

laoneo commented Jan 30, 2017

laoneo commented Jan 30, 2017

ghost commented Jan 30, 2017

wilsonge commented Jan 30, 2017

Bakual commented Jan 18, 2017 •

edited

Loading