Postpone 'create_related' rows until main row is created

[KES777]

We can add related (children) row *ONLY AFTER* main (parent) row is created!!!
So we *MUST* postpone creation.

https://rt.cpan.org/Public/Bug/Display.html?id=104375

[KES777]

What does red cross mean and how kill it? )

[ribasushi]

There is an automated system that merges your contributions and tests the result under all supported linux configurations. If you click on the "details" to the right of the "red cross", you will be taken to a list of failed builds. Looking at the log of each will give you a hint why everything broke.

In your particular case you used Data::Dump which is not available on a "virgin" perl, so everything fails. You can fix this and re-submit the PR - the build job will start again.

[KES777]

Why I get different results here and on travis?
$ prove t/52leaks.t
t/52leaks.t .. Requirement group 'ARRAY(0x25d3230)' does not exist at t/52leaks.t line 102.
t/52leaks.t .. Dubious, test returned 255 (wstat 65280, 0xff00)
No subtests run

Test Summary Report
t/52leaks.t (Wstat: 65280 Tests: 0 Failed: 0)
Non-zero exit status: 255
Parse errors: No plan found in TAP output
Files=1, Tests=0, 0 wallclock secs ( 0.02 usr 0.00 sys + 0.07 cusr 0.01 csys = 0.10 CPU)
Result: FAIL

And here:
===( 416;4 0/? 2/? 7/? 16/? 0/? 0/2 0/1 0/? 0/? )=========={UNKNOWN}: Not an ARRAY reference at /home/travis/build/dbsrgits/dbix-class/blib/lib/DBIx/Class/Row.pm line 181, <> line 1. at t/52leaks.t line 144
Tests were run but no plan was declared and done_testing() was not seen.
Looks like your test exited with 255 just after 2.
[16:11:02] t/52leaks.t ......................................... Dubious, test returned 255 (wstat 65280, 0xff00)
All 2 subtests passed

What I miss that error 'Not an ARRAY reference at /home/travis/build/dbsrgits/dbix-class/blib/lib/DBIx/Class/Row.pm line 181' not printed locally?

[KES777]

And why when I run prove it uses mine system DBIx ("/home/kes/perl5/lib/perl5") and not that which is located in current directory (t/lib)?
@inc
[
"t/lib",
"/home/kes/perl5/lib/perl5/x86_64-linux-gnu-thread-multi",
"/home/kes/perl5/lib/perl5",
"/etc/perl",
"/usr/local/lib/perl/5.18.2",
"/usr/local/share/perl/5.18.2",
"/usr/lib/perl5",
"/usr/share/perl5",
"/usr/lib/perl/5.18",
"/usr/share/perl/5.18",
"/usr/local/lib/site_perl",
".",
]

[KES777]

Hmmm.. can not create new issue, that is because I am contributor now?? ))

The problem is the inc::Module::Installer says about it install all required modules, but it do not install:
DBIx::Class::Storage::DBI::create_ddl_dir(): Can't create a ddl file without SQL::Translator >= 0.11018 (see DBIx::Class::Optional::Dependencies for details) at t/94versioning.t line 50
Actually do not upgrade existing module.
So. you do not supply required version of SQL::Translator to inc::Module::Installer or that is bug of inc::Module::Installer

[KES777]

t/99dbic_sqlt_parser.t ...........

Can't locate object method "new" via package "SQL::Translator" (perhaps you forgot to load "SQL::Translator"?) at t/99dbic_sqlt_parser.t line 294.

[ribasushi]

Why I get different results here and on travis?
...
And why when I run prove it uses mine system DBIx ("/home/kes/perl5/lib/perl5") and not that which is located in current directory (t/lib)?

Check the 2nd option here https://metacpan.org/pod/distribution/Test-Harness/bin/prove#OPTIONS

[ribasushi]

The problem is the inc::Module::Installer says about it install all required modules, but it do not install

It marks them for installation. You need to make installdeps to actually get them.

[ribasushi]

Hmmm.. can not create new issue, that is because I am contributor now?? ))

I have explicitly disabled the Issues button on this github mirror. Issues are to be reported via RT.

[KES777]

I did it =)

[ribasushi]

KES777++

This is a lot of code to review. I am especially wary of the removal of 'filter'. Likely won't get back to you in several days. If you do not hear anything until Monday - give me a ping.

[ribasushi]

@KES777 Actually, looking at the overall diff there seems to be no test case for the actual problem you encountered. At this point in the project's lifetime there must be an integration (not unit) test for almost every behavioral change. Please, if time permits adapt the test from https://rt.cpan.org/Ticket/Display.html?id=104375#txn-1495758 as a proper .t based on DBICTest->init_schema. For the sub new ... part use an early load as seen here: https://github.com/dbsrgits/dbix-class/blob/current/blead/t/73oracle_blob.t#L13-L27

You asked in https://rt.cpan.org/Ticket/Display.html?id=104375#txn-1495758 whether the current test schema/fixture are documented. They unfortunately aren't, several people attempted to and ended up with nothing so far. The main set of relations is

Artist => CDs => Tracks => cd_single (back to CDs)

There are other tables as well, but the main part of tests run against these. The preloaded data can be found here: https://github.com/dbsrgits/dbix-class/blob/current/blead/t/lib/DBICTest.pm#L428. Alternatively you can elect to populate the schema yourself: https://github.com/dbsrgits/dbix-class/blob/current/blead/t/prefetch/manual.t#L13-L48

Cheers!

[KES777]

I am especially wary of the removal of 'filter'

it is not removed. It were merged as noted here (see -247): ## 'filter' should disappear and get merged in with 'single' above!

Actually, in code, I have changed, there are few point we must pay attention:

1. Create related/child row only after parent (see +255, +272-280)
2. Filter obj is placed into $inflated hash (-258). In new code that done here: +274, +278
3. create_related_rows for 'multi', 'filter', 'single' is same. Except:
   a) 'multi' can not be 'in_storage' otherwise exception: +197
   b) +200-206 are same for 'single' and 'filter' except 'set_from_related' are executed only for 'single' (see +201)
   c) Now create_related_rows return one object or array of objects. I repeat that behaviour, BUT for future clean code you must not destinguish one or many in both cases must be arrayref (see PS)

MY RESOLUTION: code is safe and you must not worry about it ;-) Merge or not it is your decision. Look at test - all passed.

test for almost every behavioral change.

This is not behavioral change. It is leaved same. It is better to say this patch is BUGFIX for not noted yet bug: You can not add slave rows without master row not exist!!! But code, I have fixed, violate this DB integrity rule.

And actually this patch does not resolve my issue, but this one step to fixit. As I noted here:
https://rt.cpan.org/Public/Bug/Display.html?id=104375#txn-1495569 I loose clue. When I try to get access to parent/master row from children the code at ResutlSource:sub _resolve_condition:1773
return wrong condition UNRESOLVABLE_CONDITION, maybe because it do not know that row in memory yet. And I do not understand the terrible code there.

Please, if time permits adapt the test from

Yes, I will do that. But in other pull request. As I mentioned this patch does not resolve my issue.
Thank you for your examples and links. They help me. ;-)

PS. I had desire to report about some method renaming and removing, about removing of some part of documentation and add doc for well formed methods (that were renamed). But I guess you will reject that as mine other proposal. So I threw out this idea. It's a pity.

[ribasushi]

MY RESOLUTION: code is safe and you must not worry about it ;-)

Um... the entire point of review/maintainership is to worry about things ;) This PR will end up with some code being merged. Without me actually reading through it I can't yet tell you how much will be taken as-is, and how much will be changed.

I understand the eagerness of seeing your code go up on CPAN. But one needs to take into consideration the extraordinarily large footprint of this project. Due to this DBIC moves much slower than many other perl projects. All in all - patience please ;)

I had desire to report about some method renaming and removing, about removing of some part of documentation and add doc for well formed methods (that were renamed). But I guess you will reject that as mine other proposal.

It is hard for me to definitively tell whether I would have accepted or rejected a particular change. The current philosophy is roughly

• If something can be done outside of DBIC - it is always preferred to do so in a separate module
• Clarification changes to some gnarly parts of the API are fine, given the changes are 200% backwards compatible, and the old way of doing things continues working indefinitely

With the above in mind I urge you to open extra issues on RT (or alternatively discuss proposals on the IRC channel). What I definitely do not want to see is you sitting on a good idea because of a fear it will be rejected.

IOW: It's a pity ;)

[KES777]

I just do not want to troll with mine great ideas )))

[ribasushi]

I just do not want to troll with mine great ideas )))

Shrug... All I can say is that without the trolling you'll never find out if the ideas were truly great or not ;)
Anyway - either file extra RTs or keep it to the issue you are currently grappling with: I won't mind either way.

Great to have a new pair of eyes going through the gnarly code in either case ;)
@KES777++

[KES777]

what is RTs???

I will post all PRs I have. If I have time, I will continue to dig deeper (it will be usefull for description of internals). Lets continue our conversation after you return from YAPC ;-) Good Luck!

PS. and your promise to give answer how to combine job and OpenSource. Do you remember my letter? )

[ribasushi]

what is RTs???

RT stands for "Request Tracker" - the default (and often preferred bugtracker) used for CPAN projects. You have already opened several issues there, e.g. https://rt.cpan.org/Ticket/Display.html?id=104375. Each ssue is usually simply called RT for short.

They are also usually referred as RT#<num> in changelogs: https://metacpan.org/release/RIBASUSHI/DBIx-Class-0.082820#whatsnew

[KES777]

ping

[ribasushi]

@KES777 Haven't had a chance yet, I am sorry. Next day or so.

[KES777]

no problem. Just the ping as you ask =)

[ribasushi]

Ok, so now that there is a test, I understand what you are after. Do I understand correctly that the code in this PR so far is just a cleanup, and does not in fact address the bug in question? Just want to make sure we are on the same page here...

[KES777]

No. It is better to say:
I need to fix also:

DBIx::Class::Relationship::Accessor; at line 57:
my $val = $self->search_related( %1$s )->single;
or
package DBIx::Class::Relationship::Base; line 531
$rsrc->_resolve_condition( $rel_info->{cond}, $rel, $self, $rel )

To not return UNRESOLVABLE_CONDITION

See detailed info here: https://rt.cpan.org/Public/Bug/Display.html?id=104375#txn-1495569

[KES777]

that have been fixed by me already if I understand the pointed crazy code

[KES777]

ping

[ribasushi]

@KES777 Pong. I have not forgotten this, but real-life intervened. It may be that I will not finish working on this until after YAPC::NA (2 weeks from now). Sorry for the delay, as I noted earlier this project moves slowly by design.

[KES777]

No problem. I have saw that design so speed not surprise =)
Do you think to start DBIxx based on DBIx with new design, new features, approved API?

[ribasushi]

Do you think to start DBIxx based on DBIx with new design, new features, approved API?

I personally have zero interest in a "radical perl6-like reimplementation". The current API (with all its deficiencies) is in wide use, as is perl itself. Thus nothing beyond incremental improvements is practical in my personal PoV.

This doesn't mean I wouldn't like to see a different group attempt to do implement something different, and ultimately succeed at supplanting DBIC's use. It just definitively isn't something I am personally interested in pursuing.