Hi there,
it seems that the patch #606966: 'load arguments' of parent path are not inherited has broken the detection of load_functions
for menu items in some cases.
I recognized this issue while working with the commerce module. Affected hook_menu implementation: commerce_product_ui_menu
If I debug the behaviour in _menu_router_build
the load_functions
(in this case commerce_product_type_load
) works, but it's overwritten by this code:
if (!isset($item['load arguments']) && !empty($parent['load arguments'])) {
$item['_load_functions'] = $parent['_load_functions'];
}
If I disable the this code, everything works as before.
Btw: I know at least someone else that has a similar problem (affected module media_browser_plus) but I'm still not sure if this is a code or a configuration issue.
Cheers,
Peter
Comment | File | Size | Author |
---|---|---|---|
#31 | 979958-fix-inherit-load-arguments-cleanup.patch | 1.13 KB | carlos8f |
#16 | 979958-fix-inherit-load-arguments.patch | 8.23 KB | Damien Tournoud |
#15 | 979958-fix-inherit-load-arguments.patch | 7.62 KB | Damien Tournoud |
#11 | drupal.menu-load-functions.11.patch | 3.77 KB | sun |
#9 | menu-router-tests-979958-9.patch | 2.7 KB | das-peter |
Comments
Comment #1
sunOdd - I don't see any load arguments defined in there...?
Comment #2
das-peter CreditAttribution: das-peter commentedchx told me the same thing. That's one reason why I'm in doubt whether this is a configuration or code issue :|
If I understand the code right then every
%my_param
is used to check if there's a function likemy_param_load()
. If the function exists it's stored in$item['_load_functions']
and it doesn't matter ifload arguments
is present.Relevant code lines:
The inheritance code doesn't respect the case that there could already exist such
_load_functions
.Maybe an
array_merge
or an extended check for defined_load_functions
could do the trick - use the parent only if there are no_load_functions
present.Comment #3
sunwell...
1) Did you already confirm that reverting the menu.inc changes from the already committed patch makes your code work again?
2) A test proving that something is wrong would be helpful (and would be required for any patch/fix anyway).
Comment #4
das-peter CreditAttribution: das-peter commentedwell...
1) I confirmed it that way, that I commented out the following, newly introduced, code and it worked again. And I analysed the code as described in #979958-2: load_functions detection for menu items broken by patch #606966.
2) Sure, as promised in IRC here's the stuff:
admin/issue_demo/sub
. There you should see a guidance how to reproduce the issue.a) confirm that the load functions are found under normal conditions,
b) load functions are found if the parent declares
load arguments
We could add a whole bunch of other tests / asserts into these two functions but for this issue this should be sufficient.
Hope this helps identify the root cause. I'm still not sure how to proceed here.
Comment #5
das-peter CreditAttribution: das-peter commentedgrml, looks like I've a git patch issue with line endings :|
Comment #6
sunPlease move those test menu items into the menu_test_menu() implementation of menu_test.module. Requires to change the paths to slightly follow the existing pattern of router items defined there already.
Afterwards, we can remove all of the menu rebuilding helper functions contained in this patch.
The test merely invokes a drupalGet('the/deeper/of/both/paths') and the page callback (also in menu_test.module) should do
(or similar)
The test then goes ahead and asserts that we see the expected load functions on the page; i.e.,
assertText('foobar_load')
and
assertNoText('ParentPage')
Powered by Dreditor.
Comment #7
das-peter CreditAttribution: das-peter commentedNext try, hope I've got everything right :)
Comment #8
sun1) The second test is not needed and can be appended to the first. Note that for every single test, an entirely new Drupal site is installed in the internal browser; not required for this test.
2) OT: @link should have been @see, but in general, we only ever link to d.o issues in case an issue's problem space cannot be described in a couple of words, which happens very rarely.
3) The phpDoc summary of the first test is sufficient, I think. Might be worth to append "...and properly inherited." to it.
Shouldn't all three paths be in the same hierarchy? Or why did you separate out the first?
load_functions is always set. Simply use debug() to output them on the page, and return an empty string for this page callback.
Afterwards, @return can be removed from the phpDoc, as the function is simple enough then.
Can be removed.
Let's return any kind of unique string here.
Powered by Dreditor.
Comment #9
das-peter CreditAttribution: das-peter commentedDone.
Intention of having separate hierarchy is, to "isolate" the default from the inheritance case. Not sure if this has really a technical impact, I just followed my gut feeling ;)
Comment #11
sunI'm not able to cleanly replicate this bug. Don't really understand what the exact problem scenario/condition is.
Please base further patches on attached patch.
Comment #13
Damien Tournoud CreditAttribution: Damien Tournoud commentedThe patch committed in #606966: 'load arguments' of parent path are not inherited is just totally brain-dead.
That just makes no sense at all: it means that (1) the load arguments will be inherited to the child even if the load callback function is different; (2) children will get load functions even if they don't need any. *sigh*
Working on a patch.
Comment #14
webchickComment #15
Damien Tournoud CreditAttribution: Damien Tournoud commentedImplemented a more decent logic: we only inherit arguments from parents using the same loader at the same position.
Extended the test, and made it a saner unit test. Current HEAD fails 4 out of the 7 tests!
Comment #16
Damien Tournoud CreditAttribution: Damien Tournoud commentedWe should not override explicit
'load arguments'
even if empty. Restored the check for that and extended the test.Comment #17
pwolanin CreditAttribution: pwolanin commentedpatch looks reasonable.
Comment #18
pwolanin CreditAttribution: pwolanin commentedComment #19
rszrama CreditAttribution: rszrama commentedBeautiful. I was wondering why I was suddenly getting errors on my product type overview / edit pages. : )
Tests pass, Peter thinks it's reasonable, and the pertinent errors I was getting in the Commerce UI have disappeared. Seems ready, no?
Comment #20
tstoecklerI'm not that into menu.inc, I just noticed from reading the patch, that HEAD currently checks
!empty($parent['load arguments'])
while this patch doesn't. Is that intended?
Powered by Dreditor.
Comment #21
das-peter CreditAttribution: das-peter commented@rszrama I was indeed working on the commerce module when I found this ;)
@all thanks for pushing this :)
Comment #22
Damien Tournoud CreditAttribution: Damien Tournoud commented@tstoeckler: yes,
!empty($parent['load arguments'])
is the part that makes absolutely no sense and is replaced by the big condition below (ie. "the parent has a loader at the same position that uses the same function name and provides arguments").Comment #23
Cyberwolf CreditAttribution: Cyberwolf commentedSubscribing. Looks like a quite serious regression from beta3. Encountered it today after upgrading to rc1. Patch from #16 seems to fix the problems on our installation.
Comment #24
webchickUgh. :\ Committed to HEAD. Thank you.
That probably means we need a new RC very shortly, though I'd like to wait a couple more days for a few other patches. What do you guys think?
Comment #25
carlos8f CreditAttribution: carlos8f commentedThis causes only the first argument listed in $parent's 'load arguments' to be inherited. The rest would be thrown away. Why?
Comment #26
Damien Tournoud CreditAttribution: Damien Tournoud commented@carlos8f, no you are reading this wrong.
$item['_load_functions'] structure is:
(with only one $function, and $arguments being an array)
We are taking $function with
key($item['_load_functions'][$k])
and $arguments withreset($item['_load_functions'][$k])
.Comment #27
carlos8f CreditAttribution: carlos8f commentedAhh, I was reading that wrong. So wouldn't this be a more readable version of the same assignment?
Comment #28
int CreditAttribution: int commented@webchick, but if Drupal 7 RC1 wasn't official released.. Where is the changelog and the announcement? or you want to only announcement the RC2?
Comment #29
Damien Tournoud CreditAttribution: Damien Tournoud commented@carlos8f: ew. yes, that would work too :)
Comment #30
webchick@int: I rolled RC1 at about 3am, and then had meetings all day yesterday, so the annonoucement/changelog are taking awhile. :( I'm hoping to have some time at the airport tonight, but if someone wanted to become my new best friend and beat me to it, definitely no objections.
Comment #31
carlos8f CreditAttribution: carlos8f commentedLet's also not forget that PHP is braindead and
0 == 'any string'
. So we should get into the habit of using === for comparisons like this.Attached some trivial cleanup to make the inheritance more readable.
Comment #32
Damien Tournoud CreditAttribution: Damien Tournoud commentedThanks for the clean-up.
Comment #33
webchickCommitted that too. Thanks!
Comment #34
carlos8f CreditAttribution: carlos8f commentedThanks, much more readable now :)
Comment #35
Dave ReidYikes this one bit us hard yesterday since enabling View's default taxonomy term view in D7 RC1 will also cause any taxonomy term editing and deleting pages to WSOD. Thanks everyone who helped get the fix in!
Comment #36
webchickYeah, I'd like to roll a new RC, ideally yesterday, but we still have two outstanding criticals. :\ Help on squashing those would be greatly appreciated.