Bug #104541 for Catalyst-Runtime: Deep recursion on subroutine MyApp::setup_component

I can verify that you get deep recursion when trying to install a previously working CPAN module.  I'll look into it. I do want to point out some of these features are now cored, although the overall approach is different.  I wonder if the efforts could be combined in some manner?  If you could look at the new component injection stuff and two plugins I did around it that I am hoping to see in core someday as well: Catalyst-Plugin-InjectionHelpers-0.007  and Catalyst-Plugin-Ma…Dependencies-0.005 this takes a more moose role based approach rather than subclasses but I bet that could be combined.  In any case 5.9009x was labeled a 'shouldn't break stuff' release and although your code here is definitely flogging catalyst it is hitting public APIs.  Let me think about this a bit. On Tuesday, May 19, 2015 11:10 AM, Peter Flanigan via RT <bug-Catalyst-Runtime@rt.cpan.org> wrote:       Queue: Catalyst-Runtime Ticket <URL: https://rt.cpan.org/Ticket/Display.html?id=104541 > On Tue May 19 10:04:22 2015, ilmari@ilmari.org wrote: Show quoted text
Sure the code is here https://metacpan.org/source/PJFL/Catalyst-Plugin-ConfigComponents-0.9.1/lib/Catalyst/Plugin/ConfigComponents.pm

I can verify that moving the bit where we expand inner packages on a component from setup_components to setup_compont is the cause if this recursion.  However the case:  { no strict 'refs'; @{"$class\::Sub::ISA"} = @{"$class\::ISA"} } seems pathological. give me a day or two to work this out On Tuesday, May 19, 2015 9:11 PM, John Napiorkowski <jjn1056@yahoo.com> wrote: I can verify that you get deep recursion when trying to install a previously working CPAN module.  I'll look into it. I do want to point out some of these features are now cored, although the overall approach is different.  I wonder if the efforts could be combined in some manner?  If you could look at the new component injection stuff and two plugins I did around it that I am hoping to see in core someday as well: Catalyst-Plugin-InjectionHelpers-0.007  and Catalyst-Plugin-Ma…Dependencies-0.005 this takes a more moose role based approach rather than subclasses but I bet that could be combined.  In any case 5.9009x was labeled a 'shouldn't break stuff' release and although your code here is definitely flogging catalyst it is hitting public APIs.  Let me think about this a bit. On Tuesday, May 19, 2015 11:10 AM, Peter Flanigan via RT <bug-Catalyst-Runtime@rt.cpan.org> wrote:       Queue: Catalyst-Runtime Ticket <URL: https://rt.cpan.org/Ticket/Display.html?id=104541 > On Tue May 19 10:04:22 2015, ilmari@ilmari.org wrote: Show quoted text
Sure the code is here https://metacpan.org/source/PJFL/Catalyst-Plugin-ConfigComponents-0.9.1/lib/Catalyst/Plugin/ConfigComponents.pm

Ok so I thought about it and reviewed the code.  Please following and try to help me see if I got it right. I think Catayst is now correct but was broken previous, and that your test case that is now recursive is not a good test case for a problem catalyst previously had. So you have a model that inherited from this component: https://metacpan.org/source/PJFL/Catalyst-Plugin-ConfigComponents-0.9.1/t/lib/Catalyst/Component/Implicit.pm right so when that component is setup in injects a new componet "$Comp::Sub" AND it sets the base case for this new injected component to the component we just setup. That means we have a second component with the same COMPONENT method that does the same thing... So in older catalyst we only expanded inner components ONCE.   inner components could not expand.  Now we expand recursively.  Your test case recurses indefinitely now, which I think it should always have done except for this bug in Catalyst. I think the new behavior is correct and it moves us in the direction of making Catalyst component setup more flexible and closer to the 'inversion of control container' long term goal we have. I think its totally safe to wrap setup_components, that is not causing this issue. Although I think (but please correct me if you see a flaw in my thinking) that Catalyst is now correct and that your test case is now properly recursive I can see this could cause people trouble.  So I could add a configuration flag to Catalyst like 'dont_expand_inner_components' that if true fails to recurse.  That would solve your test case and if you think people hit this issue in real life non pathological code I can add that. What do you think? jnap On Tuesday, May 19, 2015 9:11 PM, John Napiorkowski <jjn1056@yahoo.com> wrote: I can verify that you get deep recursion when trying to install a previously working CPAN module.  I'll look into it. I do want to point out some of these features are now cored, although the overall approach is different.  I wonder if the efforts could be combined in some manner?  If you could look at the new component injection stuff and two plugins I did around it that I am hoping to see in core someday as well: Catalyst-Plugin-InjectionHelpers-0.007  and Catalyst-Plugin-Ma…Dependencies-0.005 this takes a more moose role based approach rather than subclasses but I bet that could be combined.  In any case 5.9009x was labeled a 'shouldn't break stuff' release and although your code here is definitely flogging catalyst it is hitting public APIs.  Let me think about this a bit. On Tuesday, May 19, 2015 11:10 AM, Peter Flanigan via RT <bug-Catalyst-Runtime@rt.cpan.org> wrote:       Queue: Catalyst-Runtime Ticket <URL: https://rt.cpan.org/Ticket/Display.html?id=104541 > On Tue May 19 10:04:22 2015, ilmari@ilmari.org wrote: Show quoted text
Sure the code is here https://metacpan.org/source/PJFL/Catalyst-Plugin-ConfigComponents-0.9.1/lib/Catalyst/Plugin/ConfigComponents.pm