Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
Steps to reproduce
Steps to duplicate (tested on my local system and simplytest.me 8.x):
- Enable content translation module
- Add a few languages
- Enable translation for articles (on content language settings page)
- Create Article
- Give article a title and body text
- Add a translation to article, change body text.
- Save and keep published.
- We are looking at the translated version of the article
- Click on Edit
- Get Fatal error: Call to a member function bundle() on a non-object in \core\modules\node\node.module on line 136
Expected behavior
The help text is displayed.
Actual behavior
Fatal error: Call to a member function bundle() on a non-object in \core\modules\node\node.module on line 136
Background
Comment | File | Size | Author |
---|---|---|---|
#50 | interdiff.txt | 897 bytes | andypost |
#50 | drupal8.routing-system.1831846-50.patch | 5.28 KB | andypost |
#47 | help-stuff.fail_.patch | 1.06 KB | larowlan |
#47 | help-stuff.pass_.patch | 4.41 KB | larowlan |
#43 | help-1831846-PASS.patch | 9.63 KB | dawehner |
Comments
Comment #1
Gábor HojtsyAlso reproducible with editing taxonomy terms. Essentially the wrong $arg array is passed to hook_help(), the language prefix is not removed, seems like.
Comment #2
Gábor HojtsyComment #3
Gábor Hojtsyhttp://api.drupal.org/api/drupal/core%21lib%21Drupal%21Core%21EventSubsc... is where the URL prefix is removed in the routing and it leads to http://api.drupal.org/api/drupal/core%21modules%21language%21language.ne... where this code sets back the path:
It is most probably not taken all the way back to _current_path() which arg() uses to end up within menu_get_active_help() improperly (http://api.drupal.org/api/drupal/core%21includes%21bootstrap.inc/functio...).
Comment #4
RobLoachMaybe something like this?
Comment #5
Gábor HojtsyNoNoNoNoNo. hook_help() gets the $arg and $path values, so it can return help for other paths which are different from the current path. It is told what path to return help for. It should base data based on the $path and $arg it got. There is also the same in taxonomy_help() and possible other issues. The problem is that the $arg it gets still includes the language prefix but it should not. We should solve that instead of trying to work around it.
Comment #6
Gábor HojtsySo once again, $arg in hook_help() on node/%node/edit is:
It should not have 'en' at the start in any case.
Comment #7
Gábor HojtsyTalked about this with @katbailey as well. Turns out the problem is arg() can only use _current_path() which will not take the request changes into account. There is current_path() to wrap it with proper lookup BUT that is not useful in early bootstrap. That in itself will not be a problem for the hook_help() use case, but arg() can and is used in other scenarios.
So I'm hearing to make this work @katbailey's and @chx's rework at #1831350: Break the circular dependency between bootstrap container and kernel need to be committed first :/ Hum.
Comment #8
katbailey CreditAttribution: katbailey commentedFor now though, it might make sense to submit a patch here with a failing test? If someone does that then I will personally promise to fix this once the other issue is sorted ;-)
Comment #9
Gábor HojtsyThat is a great suggestion. Anybody wanna do that? I just wanted to help track this down. The test would:
- enable language module
- add one more language
- set path prefix for English
- create a node
- invoke the node edit page => FAIL with a notice
All of the above can be found in other tests, so this new one is just a copy-paste from those :D
Comment #10
Gábor HojtsyComment #11
moe4715 CreditAttribution: moe4715 commentedHere is the test case. We added the block module to the test dependencies and the url prefix 'en' for the english language.
Comment #13
attiks CreditAttribution: attiks commentedTest if failing for the right reason.
@core comitters: Do NOT commit this, we're still waiting for the fix
Comment #14
BerdirThe test now confirms the problem, this means that this issue is needs work until someone writes a fix.
Comment #14.0
BerdirUpdated issue summary.
Comment #15
katbailey CreditAttribution: katbailey commentedI think this is probably far too ambitious but humour me please, testbot... I will do a less ambitious version after I've seen what explodes ;-)
Comment #17
katbailey CreditAttribution: katbailey commentedBah! How about this?
Comment #18
katbailey CreditAttribution: katbailey commentedRemoved the reference to request_path() as it is no longer relevant. Also added a comment about why we still have a fallback to _current_path().
And to explain why it should be perfectly ok to just move the current_path() function into bootstrap.inc, this function used to depend on various other things in path.inc, e.g. drupal_path_initialize() and drupal_get_normal_path(). It no longer does and would just be a wrapper around drupal_container()->get('request')->attributes->get('system_path'); were it not for the fact that we have to support run-tests.sh.
Comment #19
katbailey CreditAttribution: katbailey commentedThe problem is more general than anything to do with hook_help() so changing the title to reflect that. Also, it has nothing to do with the routing system.
Comment #20
Crell CreditAttribution: Crell commentedWhy exactly are we making arg() work, rather than exterminating arg() with extreme prejudice? If arg() no longer works in a particular context, that's an excellent reason to remove it from that context.
Comment #21
aspilicious CreditAttribution: aspilicious commentedThis should be fixed. throwing warnings all over the place. Feels major to me.
Comment #22
YesCT CreditAttribution: YesCT commented#18: 1831846.current_path.18.patch queued for re-testing.
Comment #24
Gábor Hojtsy@Crell: are you advocating using current_path() directly instead? Any more concrete suggestions?
Comment #25
Crell CreditAttribution: Crell commenteddrupal_container()->get('request')->attributes->get('system_path');
And if that's too ugly for you, then FFS stop relying on the path directly, because that's a global dependency.
Really, arg() needs to not be in Drupal 8. It is evil in numerous ways.
Comment #26
maciej.zgadzaj CreditAttribution: maciej.zgadzaj commented@Crell's suggestion rolled into a patch. Let's see what testbot is going to say about it.
Comment #27
Gábor HojtsyThat sounds strange to me, because that is mostly what http://api.drupal.org/api/drupal/core%21includes%21path.inc/function/cur... would do if we'd call that but as per the comment, we should not.
Comment #28
Crell CreditAttribution: Crell commentedGabor: The fewer layers of procedural wrappers we have to go through, the better. arg() and current_path() both need to die. Path-sensitive code is a code smell.
maciej: I wasn't suggesting we tweak how arg() works. I want to see arg() go away. Whatever the problematic code is in the help system that was originally mentioned that is using arg() should be refactored to... not use arg(). Then the bug goes away.
Comment #29
maciej.zgadzaj CreditAttribution: maciej.zgadzaj commentedOk Larry. Let's then start getting rid of arg() completely. Very small first step attached, just to make sure I'm going the right direction - using
menu_get_object()
as doc toarg()
suggested.Comment #30
Damien Tournoud CreditAttribution: Damien Tournoud commentedWhat needs to happen is that the Request needs to be injected into Blocks, specifically inside SystemHelpBlock. I started talking about that we @EclipseGc yesterday.
Comment #31
Damien Tournoud CreditAttribution: Damien Tournoud commentedTo say that another way:
hook_help()
gets injected the path and arguments, there is nothing wrong with that. The problem lies in what callshook_help()
, specificallySystemHelpBlock
.Comment #32
YesCT CreditAttribution: YesCT commentedwhat are the next steps here now?
Comment #33
Gábor HojtsyDue to changes to how node objects work, the notice reported in the summary is now a fatal error. Same problem though. Elevating to critical since you can whitescreen your site with trying to edit a translation due to this. Copied steps to reproduce from #1834014-5: Delete confirmation messages for comments in original language should include warning for translations via @JayMN:
Comment #33.0
Gábor HojtsyUpdated issue summary history copied over from et ui part 2 issue summary
Comment #34
Gábor HojtsyUpdated issue summary too.
Comment #35
dlu CreditAttribution: dlu commentedMoved to routing system per #2050763-16: Refine "base system" component (notes on refactoring of "base system" category here: https://docs.google.com/a/acquia.com/spreadsheet/ccc?key=0AusehVccVSq2dF...).
Comment #36
dawehnerCan we make a test which just enables each core modules and visits each help page once to ensure there is nothing fundamental broken?
Comment #37
mradcliffeI was able to solve this by passing in the second argument to arg(), which is the current_path() in #1937852: menu_get_active_help() does not support language arguments for node_edit router path. This worked for the immediate issue with nodes and systemblockhelp.
Also related is #244090: Tie help into menu router, which is an ancient issue that now has its goal of using menu router path in help instead of the current hook_help implementation.
It might be faster to use the second argument in arg() instead of fixing hook_help() implementations at this time.
Comment #38
Hydra CreditAttribution: Hydra commentedThe patch in #29 works for the node/add/% part so far, but the $arg ist still used in other cases. Case node/%/edit is causing the error described by Gábor. The cause is, that when we are editing an other language, the path is something like de/1/edit, what ends up in an $args array which is has the language parameter in $arg[1] instead of the node id. Therefore node_load isn't loading a node object.
This solves the fatal error, but this is not a good solution.
Comment #39
dawehnerSo what about something like this? This basically implements the idea of damien.
Comment #40
Hydra CreditAttribution: Hydra commentedOh I love this! Works perfectly fine. Just a little novice I found. Thanks dawehener!
Missing whitespace
Comment #41
dawehner#40: help-1831846-40.patch queued for re-testing.
Comment #42
catchRe-titling for the functional bug here. Patch looks good to me.
Looks like there's tests in #11 which could be combined with the patch.
Comment #43
dawehnerIt would be really cool if some folks could check whether the rerole of the test was fine, as they still fail with the fix applied for me.
Comment #45
Gábor HojtsyUnfortunatelt the PASS patch did not pass either :/ Do we want to remove menu_get_active_help() too then?
Comment #46
dawehnerHa yeah I probably missed up the tests rerole ... :(
Comment #47
larowlanFix from 40 with tests from #2092641: node_help() does not allows to edit node translations
Comment #48
andypostLet's get this in asap
Comment #49
catchmenu_get_active_help() is dead code now, let's remove that here.
Comment #50
andypostSo removed
Comment #51
catchThanks!
Comment #52
Gábor HojtsyLet's get this in :)
Comment #53
catchCommitted/pushed to 8.x, thanks!
Comment #54
Gábor HojtsyWoot, amazing!
Comment #55.0
(not verified) CreditAttribution: commentedUpdate with new STR, now a fatal error