When the menu rebuilds all access arguments get evaluated. That means commerce_order_new() with no parameters loads up commerce_order_state_load() and goes down a gnarly trail.
Sample stacktrace when running registry-rebuild through Drush, which luckily I discovered due to random error.
Call Stack:
0.0001 230072 1. {main}()
0.0009 393336 2. drush_main()
0.2296 10391080 3. Drush\Boot\BaseBoot->bootstrap_and_dispatch()
0.2330 10400888 4. drush_dispatch(, ???)
0.2345 10405600 5. call_user_func_array('drush_command', array ())
0.2345 10405920 6. drush_command()
0.2357 10455736 7. _drush_invoke_hooks(, array ())
0.2364 10498416 8. call_user_func_array('drush_registry_rebuild', array ())
0.2364 10499584 9. drush_registry_rebuild()
2.0957 61995320 10. drush_registry_rebuild_cc_all()
2.1074 61996832 11. drupal_flush_all_caches()
3.1784 71643528 12. menu_rebuild()
3.1810 71644264 13. menu_router_build()
3.3634 76698424 14. call_user_func('commerce_order_ui_menu')
3.3634 76698528 15. commerce_order_ui_menu()
3.3634 76699648 16. commerce_order_new('pending', ???, ???)
3.3634 76699960 17. commerce_order_state_load('pending')
3.3634 76700008 18. commerce_order_states()
3.3634 76700464 19. module_invoke_all('commerce_order_state_info')
3.3688 76704608 20. call_user_func_array('commerce_checkout_commerce_order_state_info', array ())
3.3688 76704816 21. commerce_checkout_commerce_order_state_info()
3.3688 76705856 22. commerce_checkout_first_checkout_page()
3.3688 76705936 23. commerce_checkout_pages()
3.3752 76724624 24. drupal_alter('commerce_checkout_page_info', array (
I don't even get how commerce_checkout_first_checkout_page() gets involved, but by providing a UID of 0 and "pending" it bypasses this madness in the hook_menu call.
| Comment | File | Size | Author |
|---|---|---|---|
| #19 | commerce_order_ui_menu-2632084-19.patch | 1.92 KB | smccabe |
Comments
Comment #2
mglamanPatch attached. Fixes this madness by returning an order entity for menu rebuild instead of going down commerce_order_state_load () rabbit hole.
Comment #3
joelpittetThis is fine as long as someone doesn't define a custom default status in the pending state. Though if they do it will be put into the wrong one.
I had to look that up BTW for the second or third time: https://drupalcommerce.org/faq/order-states
Is that acceptable or should we find another route to shrink that callstack?
Comment #4
joelpittetTo avoid any regression I discussed #3 with @mglaman and maybe we can store the one call to that function instead of the second call. It won't save a lot but at least there wouldn't be a regression for someones custom starting/default 'pending' status.
Maybe
commerce_order_states()should go into a permanent cache to avoid the rabbit hole in all cases?Comment #5
joelpittetSorry if I'm hijacking this:( I'll move this to a new issue if you prefer.
Comment #6
mglamanComment #7
joelpittetMoved the cache to it's own issue #2636642: Add permanent cache to store commerce_order_states()
Comment #8
joelpittetThis won't have a great performance improvement but it's quick. The problem is the depth of the stack and worried if default pending status is changed by a company this would break their site's workflow with #2
Comment #9
rszrama commentedAgreed on your reservations; I think Matt and I chatted about them but didn't end up rerolling the patch. I'm committing this one as is and will hit the cache one next.
Comment #11
mglamanThis didn't really I prove anything. We're still forcing the order state and statuses to be built, thus triggering checkout page info and panes to be run. By the second invocation, pre-patch, it was statically cached.
Edit: that's why my original patch provided arguments to the function.
Comment #12
joelpittetThe default args would have a workflow regression if people have changed the default status within the ode ting state.
Not sure how likely that is but trying to be safe than sorry considering how varied businesses can be.
Yeah this fix maybe saves a couple function calls, super micro optimization. Is there another way we could improve this without the regression?
Comment #13
torgospizza> Is there another way we could improve this without the regression?
Why not just remove the call to commerce_order_new() from the hook_menu and into the function itself? (See patch.)
Seems really weird to me that we would put commerce_order_new() into hook_menu (which gets called whenever there is a menu_rebuild()) for use as an argument, when the $order argument itself is never really checked for anything. The only condition in commerce_order_ui_order_form_wrapper($order, $account = NULL) is to check for is (empty($order->order_id)). If $order was set to NULL or something (and actually an argument passed in via the URL magic loading method) then this would make sense. But currently it just strikes me as unnecessary.
Comment #15
torgospizzaI'm dumb. I assume commerce_entity_access probably requires the new entity be passed to it... so this approach may not fly, unless we move the order creation call to within that function if one is not passed to it.
I'll leave it open for now in case we want to discuss it further, but I agree with @joelpittet - a better optimization would be preferred.
Comment #16
smccabe commentedWouldn't something like this work? It is just #13 patch updated to allow $order to be optional, so it can be passed in for editing but doesn't have to be provided when creating.
Comment #17
smccabe commentedWhoops, small mistake in that patch.
Comment #18
mglamanThis actually makes a lot more sense to me, instead of caching an empty order object in menu cache we just initiate one when the form is built.
Comment #19
smccabe commentedCorrected some minor formatting pointed out by @joelpittet
Comment #20
joelpittetBeauty, I like this approach much better and leaves the arguments for BC and the edit menu item
admin/commerce/orders/%commerce_order/editComment #21
rszrama commentedCommitted, sorry I missed this one in my last commit spree.