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

Treat undef like blessed objects for the purpose of looking for special handling #106

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

jonathancast
Copy link

Suppose foo is an inflated timestamp column and $row->foo is set to a true DateTime (representing a date, say, 2016-06-23T16:42:34). In the present code, these two code snippets:

$row->set_inflated_column(foo => undef);
say 'The value is: '.$row->get_column('foo');
say 'The value is: '.$row->foo;

$row->set_inflated_columns({ foo => undef, });
say 'The value is: '.$row->get_column('foo');
say 'The value is: '.$row->foo;

would do different things. The first prints

The value is: 
The value is: 

the second prints

The value is: 
The value is: 2016-06-23T16:42:34

Because update uses set_inflated_columns, the same behavior is visible there.

This change makes set_inflated_columns call set_inflated_column when the column is inflated and new value is undef (which set_inflated_column handles correctly), which causes the two functions to behave identically in this case, which seems like the correct behavior.

@ribasushi
Copy link
Collaborator

Hi, and thank you for the report/patch!

A similar problem was reported couple weeks earlier, I haven't gotten around to looking at the other report yet: I am hoping to be able to do so in 2~3 days from now.

Your fix seems incorrect: it kinda sorta feels like you should be doing the cache invalidation at the right spot, instead of giving undef inflated status (which is likely to mess with other logic N steps removed). I can not put my finger on it yet, but then I haven't dug too deep.

Unrelated and regardless of the above: DBIC is at a stage of its life where single line fixes like the proposed can no longer be applied without an accompanying fails before / doesn't fail after test. While I am busy elsewhere, could you please put an addition to a file of your choice in t/icdt/* that will exercise the changeset in question?

Thank you!

@ribasushi
Copy link
Collaborator

@jonathancast yup, this is roughly what I was looking for, thanks! (there are a few snags wrt extra dependencies, but I will fix these myself, no need to worry about them).

I will get back to you on the fix really soon.

@jonathancast
Copy link
Author

Added test that fails in the old version and passes with the changes

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