Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Reverse conversion to DateTime object when Datetime as filter param #32

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

Vlczech
Copy link
Contributor

@Vlczech Vlczech commented Apr 5, 2017

Reverse conversion to DateTime object from array of datetime values (as filter param)

(in case of approval PR, please include it to 11.2.X release also - it is based on 11.X release)

@uestla
Copy link
Owner

uestla commented Apr 5, 2017

Sorry, but I don't like this. I see your point but what if I had this filter container:

$inner = $container->addContainer('inner');
$inner->addText('date');
$inner->addText('timezone_type');
$inner->addText('timezone');

Then it would return me \DateTime instances where I would expect string array.

@Vlczech
Copy link
Contributor Author

Vlczech commented Apr 8, 2017

@uestla: I don't like this scifi types of ifs, because lot of things would not be created. The same I can say: what if somebody has in custom template for example block column-label: 2156f01#diff-1f1cc339449554a5dc5f3ac6e0aa8d74R177

So, invent how Datagrid has to remember filters and we can check if appropriate filter has these three parts, which you mentioned. If yes, then no datetime conversion will be performed; if no, something like this datetime conversion will be perfomermed.

@uestla
Copy link
Owner

uestla commented Apr 8, 2017

what if somebody has in custom template for example block column-label

Then it would extend the default block, where's the problem here? Nothing unexpected for me.

What would be unexpected however, is a special combination of three values in filter array from which the grid would create an object.

I think this shouldn't be solved on the datagrid level since it's more about serializing the filter value in URL.

So, invent how Datagrid has to remember filters ...

No no no, please do not command me. This thread is for discussing/reviewing your pull request, not for creating a job nobody pays me for. Thank you.

@Vlczech
Copy link
Contributor Author

Vlczech commented Apr 15, 2017

Then it would extend the default block, where's the problem here? Nothing unexpected for me.

If somebody already had this block name for another block or if somebody uses this block for another block, then unexpected behavior of this block occurs also.

The same thing as $inner = $container->addContainer('inner'); $inner->addText('date'); $inner->addText('timezone_type'); $inner->addText('timezone');
In this case I can use another inner name to avoid datetime object.

I don't know, how other would you like to solve the serialization of params, cause the same situation occurs with additional parameter of serialization

Do not be so annoying - I didn't command you, I only proposed how to solve this problem.

@uestla
Copy link
Owner

uestla commented Apr 16, 2017

If somebody already had this block name for another block or if somebody uses this block for another block, then unexpected behavior of this block occurs also.

Yes, but this logic could be applied to 4dd6f21 which you had no problem with tagging as a new version and did not consider it as BC break...

But that is off topic, let's please return to the serialization.

In this case I can use another inner name to avoid datetime object.

But why would you want to? I would hate the datagrid with a passion if I would have to rename my form fields just because it converts my values to some object by default.

Solving this by changing filters serialization cannot be IMHO done without BC break. Why don't you just change your DateTime form control to return not \DateTime object but string value (even by extending it just for the purpose of using it in datagrid)?

Do not be so annoying...

I think "annoyed" is what you meant, otherwise the rudeness of your messages would be getting out of hand...

@Vlczech
Copy link
Contributor Author

Vlczech commented Apr 24, 2017

Yes, but this logic could be applied to 4dd6f21 which you had no problem with tagging as a new version and did not consider it as BC break...

I know, I just compare, that this is a similar situation of using "what if somebody ..."

Yes, you are right, that it can be done by returning string instead of DateTime, but my bet is that grid should be able to work with DateTime also.

It is possible - I meant "otrávený" in Czech ;)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants