?

User tests: Successful: Unsuccessful:

avatar kunal-bajpai
kunal-bajpai
18 Jan 2014

ISSUE : JHtml::calendar ignores $format when readonly attrib set

avatar kunal-bajpai kunal-bajpai - open - 18 Jan 2014
avatar Bakual
Bakual - comment - 18 Jan 2014

Please have a look at what is done in #2531. As I understand it, it's the same issue but for Joomla 3.2. You likely need a similar approach.

Simply removing the % from the format will not work as the format is set to %Y-%m-%d %H:%M:%S and you'd end up with Y-m-d H:M:S instead of the correct Y-m-d H:i:s
Also depending on extension and language, you would face other inconsistencies between the two date formats as well.

avatar kunal-bajpai
kunal-bajpai - comment - 18 Jan 2014

Added a new function to convert the linux time codes to php time codes. Please check now @Bakual

avatar Bakual
Bakual - comment - 18 Jan 2014

Did you had a look at the other PR? I would prefer a consistent solution for Joomla 2.5 and 3.x.
Also this one would only solve it for readonly, but would still ignore the format for editable fields?

avatar kunal-bajpai
kunal-bajpai - comment - 19 Jan 2014

@Bakual Yes, I did have a look. The first issue that you have mentioned was due to the hard coded format 'Y-m-d H:i:s' for readonly fields and have fixed that by replacing it with a method call to linux_to_php_date($format).
I could not reproduce the second issue you had mentioned (readonly and disabled attributes ignored).

Format attribute is respected for non readonly attributes as that was already being passed in the js initialisation of the calendar. Please test with field

Since i am really a Joomla novice, I am unable to see exactly where this solution fails for Joomla 3.x. Can you please explain that part to me?

avatar kunal-bajpai
kunal-bajpai - comment - 19 Jan 2014

@Bakual Yes, I did have a look. The first issue that you have mentioned was due to the hard coded format 'Y-m-d H:i:s' for readonly fields and have fixed that by replacing it with a method call to linux_to_php_date($format). Format attribute is respected for non readonly attributes as that was already being passed in the js initialisation of the calendar. Please test with field &lt field type="calendar" format="%d-%m-%Y" / &gt

I successfully reproduced the second bug in 3.2.1. But this does not exist in 2.5 and hence is not included here.

avatar Bakual
Bakual - comment - 19 Jan 2014

Format attribute is respected for non readonly attributes as that was already being passed in the js initialisation of the calendar.

I can't confirm that. See screenshot, left part is the article form and right part is the modified article.xml form definition where you see the format is set to the date only (without time).
article

As you see, the editable Created Date field is not respecting the format neither. It's only respected in the JavaScript calendar itself. That is when you select a date from the popup window, it will show correct in the field. However if you load the article form the next time, it will show the time part again.

avatar kunal-bajpai
kunal-bajpai - comment - 19 Jan 2014

I see. Since the calendar respects the format after it has loaded, I believe the bug may not lie with the calendar field but in the way in which the stored date is first being put into the calendar field.
If we look into com_content, I think we should be able to find some line where the stored timestamp is being formatted using a hard coded format thus ignoring our format for that particular instance. Do you think you know where to look for that?

avatar kunal-bajpai
kunal-bajpai - comment - 19 Jan 2014

@Bakual Digging a little into com_content, I find that the calendar fields in question are being echoed on line #120 of /administrator/components/com_content/views/article/tmpl/edit.php which is
echo $this->form->getInput('created');
I am still unable to locate the definition of this function. I feel this is where the bug would be. Can you tell me where i can locate the definition?

avatar kunal-bajpai
kunal-bajpai - comment - 19 Jan 2014

Not the definition of the function actually, but where the initial values for this field are initialised. I think that is where calendar will be assigned the value stored in the db.

avatar kunal-bajpai
kunal-bajpai - comment - 19 Jan 2014

@Bakual I guess I was wrong about the bug being in com_content. Found where the value was being hard coded. Please test now.

avatar Bakual
Bakual - comment - 19 Jan 2014

It's all in those two files you already found: libraries/joomla/form/fields/calendar.php is the formfield which is called by JForm to display the field. The formfield then uses JHtml::_('calendar', ...) in libraries/joomla/html/html.php to actually show the calendar input.
The $value itself comes straight from the database, is adjusted to the timezone as needed by the formfield and then passed to JHtml. JHtml itself just outputs the given value without applying any formats for the editable field.

Again, it's the same issue I fixed in the other PR for 3.x. I don't think converting the $format back and forth is the solution for this issue, that's why I came up with the solution proposed in the PR where the format is simply applied using strftime.

avatar Bakual
Bakual - comment - 19 Jan 2014

Just looking at the code:
Now you're formatting the value twice for readonly fields, once in the formfield and once in JHtml.
And the editable fields still don't work if JHtml::calendar is used directly (not using the formfield).

That's why I used

$inputvalue = strftime($format, strtotime($value));

and use $inputvalue then for the <input> field.
There's a bit more code around it which deal with timezones to make sure the value isn't adjusted based on tz.

avatar kunal-bajpai
kunal-bajpai - comment - 19 Jan 2014

@Bakual This final fix has done it for me. Bug fixed. Please test.

avatar kunal-bajpai
kunal-bajpai - comment - 19 Jan 2014

I get what you are saying. Let me fix that.

avatar Bakual
Bakual - comment - 19 Jan 2014

Still the same issues present I wrote 20 minutes ago and still don't understand why you don't want to use strftime() directly.

avatar kunal-bajpai
kunal-bajpai - comment - 19 Jan 2014

@Bakual On line 982 of html.php, a format string is to be passed to date. Suppose we pass 'Y-m-d H:i:s' and it returns '2014-02-01 08:08:08'. To use strftime, you are first using strtotime on this value. if you pass this to strtotime, there is an ambiguity bet ween Y-m-d H:i:s and Y-d-m H:i:s as any one of 01 or 02 may be the month and then strtotime will apply its own method to resolve this. This may result in incorrect conversions.
Hence, i suggest that we translate the format string itself using our custom method.
Can you please confirm if your format string is not being ignored now so then we can move on to what you are saying about time zones.

avatar Bakual
Bakual - comment - 19 Jan 2014

Using formats like Y-d-m will likely fail anyway as soon as you save the value. Because during save PHP date will guess the format itself. If you want to solve that, you need a lot more code.
Also when you are using strftime, you don't need the self::_('date', ...) part anymore, because $inputvalue is already formatted correct.
As you can see in my PR, I don't use it there. I also don't need the if-else part to return different inputs when the field is readable or not. I just add the style and attributes as needed. Otherwise the output is the same.

avatar kunal-bajpai
kunal-bajpai - comment - 19 Jan 2014

@Bakual You can see this is one of my first pull requests so I tried not changing a lot of code to prevent unwarranted repercussions.
So I did it by adding a method and altering small chunks of code while your PR does it by alteration to a bigger chunk of code.
As you said, outputs are the same. So this does solve the issue for you, right?

avatar Bakual
Bakual - comment - 19 Jan 2014

You can see this is one of my first pull requests so I tried not changing a lot of code to prevent unwarranted repercussions.

I didn't want to discourage you. I very appreciate your help. I just wanted to point you to a better and already tested solution.

So I did it by adding a method and altering small chunks of code while your PR does it by alteration to a bigger chunk of code.

Actually, you ended up with 85 additions and 4 deletions, while mine has 49 additions and 39 deletions. I'd say the change is about the same size :smile:
Also my PR deals with an additional issue about readonly attributes which I believe isn't present in 2.5.

As you said, outputs are the same. So this does solve the issue for you, right?

I meant the output for readonly and editable fields is the same in my PR.

And no, the issue isn't solved

  • when you use JHtml::calendar directly (not using the formfield), the format is only applied to readonly fields. Editable fields are untouched.
  • On the other hand when using the formfield, the format is applied twice for readonly fields. This isn't really an issue, it's just unneeded.
avatar kunal-bajpai
kunal-bajpai - comment - 19 Jan 2014

@Bakual Right. I'll try and understand your code and get it in here then. Thanks for your support and prompt replies. You have been a great help. Hopefully, I'll fix this soon. :)

avatar kunal-bajpai
kunal-bajpai - comment - 19 Jan 2014

@Bakual Fixed the date to be formatted for readonly and editable even when calendar is directly used. However, I could not avoid formatting the date twice when a filter is set so I went through your code. If i am not wrong then there too the date is formatted twice in case a formfield is used and a filter is set (once while fixing the tz in calendar.php and then in html.php using strftime) so I let that remain.

I still stuck to my implementation because I feel I understand that better. Also, just to clarify, I've spent about 45 lines for a single array definition just to make it more readable so the additions are actually just half the number.
Still, please let me know if the PR is OK or if there is anything at all that I need to take care of further. I hope to take away from here more than just a bug fix. Thanks a lot for your patience.

avatar Bakual
Bakual - comment - 19 Jan 2014

If i am not wrong then there too the date is formatted twice in case a formfield is used and a filter is set (once while fixing the tz in calendar.php and then in html.php using strftime) so I let that remain.

The formfield only calculates the TZ difference based on the filter parameter (user or server timezone). It doesn't apply a format here.

Also, just to clarify, I've spent about 45 lines for a single array definition just to make it more readable so the additions are actually just half the number.

Don't want to be picky, but many of the changed lines in my PR are only because I removed an if-else. Most of the code is untouched elsewhere but shows up as changed because the whole block was moved around slightly. Plus there is an additional issue and related test. :smile:
It's really quite a small change once you understand what I did. It looks more complicate than it is.

Still, please let me know if the PR is OK or if there is anything at all that I need to take care of further.

We just need to make sure the solution is the same in the end in both PRs. I don't want a new JHtml method in J!2.5 which isn't present in J!3.x. The codebase should only be different when there are reasons for it. And for this I don't see any reason to have two different solutions. So either mine can be backported to 2.5 or yours will end up in 3.x as well. Otherwise it's just harder to fix something without actually gaining anything.
Personally I prefer my solution for obvious subjective reasons.

If you want, you can send me an email to bakual@bakual.ch so we could disucss it a bit better.

avatar kunal-bajpai
kunal-bajpai - comment - 20 Jan 2014

@Bakual Yes, thank you. I had not thought of the effect on the code base due to the addition of an unnecessary function.
Here goes, then. Adapted your code to fit 2.5. Hope this is satisfactory now.

avatar kunal-bajpai
kunal-bajpai - comment - 20 Jan 2014

@Bakual Fixed the spaces.

avatar kunal-bajpai
kunal-bajpai - comment - 20 Jan 2014

@Bakual Hope that works :D

avatar brianteeman
brianteeman - comment - 8 Aug 2014

Closed as per the comment on the tracker

avatar brianteeman brianteeman - change - 8 Aug 2014
Status New Closed
Closed_Date 0000-00-00 00:00:00 2014-08-08 14:20:36
avatar brianteeman brianteeman - close - 8 Aug 2014

Add a Comment

Login with GitHub to post a comment