[Dbix-class] Class::C3::Componentised bug (was deployment bug with DBIC running from PAR)

[Peter Rabbitson]

Toby Corkindale wrote:
> Peter Rabbitson wrote:
>> Toby Corkindale wrote:
>>> Peter Rabbitson wrote:
>>>> Toby Corkindale wrote:
>>>>> I need to stop replying to myself :(
>>>>> I'd say the problem lies this block of code:
>>>>>
>>>>> # Look through the @INC path to find the file
>>>>>     foreach ( @try_first, @INC ) {
>>>>>         my $full = "$_/$filename";
>>>>>         next unless -e $full;
>>>>>         return $UNIX ? $full : $class->_inc_to_local($full);
>>>>> }
>>>>>
>>>>> since when using PAR, the first entry in @INC is a coderef, and thus
>>>>> this routine tests for '-e "CODE(0xd34db33f)/Foo/Bar.pm"' which, not
>>>>> unsurprisingly, fails.
>>>>>
>>>>> D'Oh.
>>>>>
>>>>> I'll raise some bugs on RT. Not sure where the blame lies now..
>>>> Hi,
>>>> The issue you describe was worked around in this commit.
>>>> http://dev.catalyst.perl.org/svnweb/bast/revision/?rev=5355
>>>>
>>>> I would recommend against raising a bug against C3:: as it behaves
>>>> correctly
>>>> wrt availability of a module. Since there is no mechanism for a
>>>> coderef in
>>>> @INC to say "I didn't find" vs "I didn't load", ensure_class_found can
>>>> not
>>>> be expected to work correctly. A POD patch indicating the above
>>>> would be
>>>> most welcome.
>>> I'm not entirely sure what that POD should say though..
>>> "Warning - this feature will not work when used in one of the two ways
>>> you can use PAR, and thus should not be used at all if you intend your
>>> application, or any of its descendants, to work properly" ?
>>>
>>> Perhaps It would be better to submitt a patch against
>>> DBIx::Class::Schema::Storage::DBI to remove the uses of
>>> ensure_class_found instead?
>>
>> You didn't check the link I sent did you.
> 
> Hi Peter,
> I did check that link; however it didn't change my feeling on the
> matter, ie. that the mentioned function should always be avoided if it
> is (apparently) well known to cause issues downstream.

Hm... Maybe I am overlooking something. In the patch I explicitly remove
the call to ensure_class_found. In fact if one greps for ensure_class_found
it is nowhere to be found in current trunk, except for the test suite.

> The thing is, a doc patch to C3::Componentised can warn developers who
> are using it immediately in their apps.. but it doesn't help developers
> who are working one step removed. Eg. I use DBIx::Class, but I haven't
> thoroughly read the documentation for every single one of its dependency
> tree.
> I just think it's good practice to avoid using functions that have
> potential bad surprises..

So question remains - what is confusing you at this point - that the
function exists to begin with?

> But hey, look, I don't know the bigger picture, and don't want to start
> a storm in a teacup over this; so if the problem is known about and
> workarounds already being put in place, I'm happy that the right thing
> is being done.

Neither do I, but then I suspect that I am missing something from the
discussion.

>>> I freely admit I don't understand why one would use the
>>> "check-but-don't-load" functionality often, especially since the case in
>>> point that breaks the deployment will always attempt to load the class
>>> immediately if it is found:
>>>
>>
>> If someone put it there, someone does use it.
> 
> Although the existence, or popularity, of something doesn't make it
> correct.. Remember Matt's Script Archive? :)
> 
>> In short:
>>
>> 1) DBIC was fixed last week - applied patch is here:
>> http://dev.catalyst.perl.org/svnweb/bast/revision/?rev=5355
>>
>> 2) Raising bugs against C3 is not productive. Much better to supply a POD
>> patch against Class::C3::Componentised, explaining that
>> ensure_class_found()
>> will not work with PAR environments and anything that stuffs coderefs
>> in @INC.
> 
> As above, I disagree that the bug is not productive. It's a valid issue,
> and you must agree that the module would be better if it did not have
> the issue?

I disagree this is an issue... It is a feature. A feature that is broken
by a deficiency of coderef in @INC handling.

> However I'm writing up the POD patch as we speak, and will submit that
> as well. Thanks for the suggestion.

Thank you.

Cheers