Steps to reproduce

Steps to duplicate (tested on my local system and simplytest.me 8.x):

  1. Enable content translation module
  2. Add a few languages
  3. Enable translation for articles (on content language settings page)
  4. Create Article
  5. Give article a title and body text
  6. Add a translation to article, change body text.
  7. Save and keep published.
  8. We are looking at the translated version of the article
  9. Click on Edit
  10. 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

For background see #29?, #51, #57, #181.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Gábor Hojtsy’s picture

Issue tags: +Needs tests

Also reproducible with editing taxonomy terms. Essentially the wrong $arg array is passed to hook_help(), the language prefix is not removed, seems like.

Gábor Hojtsy’s picture

[8:29pm] volkman: GaborHojtsy: okay, this is going to sound extremely noobish, but where is the route determined?
[8:29pm] volkman: GaborHojtsy: core/lib/Drupal/Core/Routing/RouteCompiler.php?
GaborHojtsy: volkman: so, I don't know either
[8:46pm] GaborHojtsy: volkman: I'd go look where hook_help() is invoked
[8:46pm] GaborHojtsy: and how it gets the args
[8:46pm] volkman: yeah, i started xdebugging it, but then i got pulled back to client work
[8:47pm] GaborHojtsy: volkman: oh
[8:47pm] volkman: i'll work more on it this evening
[8:47pm] GaborHojtsy: volkman: http://api.drupal.org/api/drupal/core%21modules%21help%21help.api.php/fu...
[8:47pm] GaborHojtsy: volkman: probably http://api.drupal.org/api/drupal/core%21includes%21menu.inc/function/men... and onwards
[8:49pm] GaborHojtsy: volkman: then http://api.drupal.org/api/drupal/core%21includes%21bootstrap.inc/functio... and http://api.drupal.org/api/drupal/core%21includes%21bootstrap.inc/functio... from there
[8:49pm] GaborHojtsy: volkman: which leads to http://api.drupal.org/api/drupal/core%21includes%21bootstrap.inc/functio...
[8:50pm] GaborHojtsy: volkman: and http://api.drupal.org/api/drupal/core%21includes%21bootstrap.inc/functio... and seem to be no language removal applied on the way
[8:50pm] GaborHojtsy: volkman: I wonder why other things don't break
[8:50pm] volkman: GaborHojtsy: yeah, that is curious
volkman: GaborHojtsy: yeah, that is curious
[8:52pm] neclimdul: good news everyone! "how did things ever work" is late in the solving the problem process

Gábor Hojtsy’s picture

http://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:

      list($language, $path) = language_url_split_prefix($current_path, $languages);
      // Store the correct system path, i.e. minus the path prefix, in the
      // request.
      $request->attributes->set('system_path', $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...).

RobLoach’s picture

Status: Active » Needs review
FileSize
1.06 KB

Maybe something like this?

Gábor Hojtsy’s picture

Status: Needs review » Needs work

NoNoNoNoNo. 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.

Gábor Hojtsy’s picture

So once again, $arg in hook_help() on node/%node/edit is:

array ( 0 => 'en', 1 => 'node', 2 => '1', 3 => 'edit', 4 => '', 5 => '', 6 => '', 7 => '', 8 => '', 9 => '', 10 => '', 11 => '', )

It should not have 'en' at the start in any case.

Gábor Hojtsy’s picture

Status: Needs work » Postponed

Talked 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.

katbailey’s picture

For 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 ;-)

Gábor Hojtsy’s picture

Status: Postponed » Active
Issue tags: -Needs tests

That 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

Gábor Hojtsy’s picture

Issue tags: +Needs tests
moe4715’s picture

Status: Active » Needs review
FileSize
4.96 KB

Here is the test case. We added the block module to the test dependencies and the url prefix 'en' for the english language.

Status: Needs review » Needs work

The last submitted patch, 1831846-extended-node-translation-ui-test.patch, failed testing.

attiks’s picture

Status: Needs work » Reviewed & tested by the community

Test if failing for the right reason.

@core comitters: Do NOT commit this, we're still waiting for the fix

Berdir’s picture

Status: Reviewed & tested by the community » Needs work

The test now confirms the problem, this means that this issue is needs work until someone writes a fix.

Berdir’s picture

Issue summary: View changes

Updated issue summary.

katbailey’s picture

Status: Needs work » Needs review
FileSize
7.8 KB

I 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 ;-)

Status: Needs review » Needs work

The last submitted patch, 1831846.current_path.15.patch, failed testing.

katbailey’s picture

Status: Needs work » Needs review
FileSize
542 bytes
7.89 KB

Bah! How about this?

katbailey’s picture

FileSize
907 bytes
7.99 KB

Removed 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.

katbailey’s picture

Title: Hook help does not handle path segments correctly when URL language prefixes are enabled » arg() is using _current_path() instead of current_path()
Component: routing system » base system

The 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.

Crell’s picture

Why 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.

aspilicious’s picture

This should be fixed. throwing warnings all over the place. Feels major to me.

YesCT’s picture

Issue tags: -Needs tests

#18: 1831846.current_path.18.patch queued for re-testing.

Status: Needs review » Needs work
Issue tags: +Needs tests

The last submitted patch, 1831846.current_path.18.patch, failed testing.

Gábor Hojtsy’s picture

@Crell: are you advocating using current_path() directly instead? Any more concrete suggestions?

Crell’s picture

drupal_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.

maciej.zgadzaj’s picture

Status: Needs work » Needs review
FileSize
557 bytes

@Crell's suggestion rolled into a patch. Let's see what testbot is going to say about it.

Gábor Hojtsy’s picture

That 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.

Crell’s picture

Gabor: 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.

maciej.zgadzaj’s picture

Ok 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 to arg() suggested.

Damien Tournoud’s picture

What needs to happen is that the Request needs to be injected into Blocks, specifically inside SystemHelpBlock. I started talking about that we @EclipseGc yesterday.

Damien Tournoud’s picture

To say that another way: hook_help() gets injected the path and arguments, there is nothing wrong with that. The problem lies in what calls hook_help(), specifically SystemHelpBlock.

YesCT’s picture

what are the next steps here now?

Gábor Hojtsy’s picture

Priority: Normal » Critical

Due 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:

Steps to duplicate (tested on my local system and simplytest.me 8.x):

  1. Enable content translation under extend
  2. Add a few languages
  3. Enable translation for article and basic page (on content language settings page)
  4. Create Article or Basic page
  5. Can create new revision or not
  6. Give article or basic page a title and body text
  7. Add a translation to article or basic page, change body text.
  8. Save and keep published.
  9. We are looking at the translated version of the article or basic page
  10. Click on Edit
  11. Get Fatal error: Call to a member function bundle() on a non-object in \core\modules\node\node.module on line 136
Gábor Hojtsy’s picture

Issue summary: View changes

Updated issue summary history copied over from et ui part 2 issue summary

Gábor Hojtsy’s picture

Updated issue summary too.

dlu’s picture

Moved 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...).

dawehner’s picture

Component: base system » routing system

Can we make a test which just enables each core modules and visits each help page once to ensure there is nothing fundamental broken?

mradcliffe’s picture

I 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.

Hydra’s picture

Status: Needs review » Needs work

The 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.

    case 'node/%/edit':
      $nid = is_numeric($arg[1]) ? $arg[1] : $arg[2];
      $node = node_load($nid);

This solves the fatal error, but this is not a good solution.

dawehner’s picture

Status: Needs work » Needs review
FileSize
3.5 KB

So what about something like this? This basically implements the idea of damien.

Hydra’s picture

FileSize
3.49 KB

Oh I love this! Works perfectly fine. Just a little novice I found. Thanks dawehener!

+++ b/core/modules/system/lib/Drupal/system/Plugin/Block/SystemHelpBlock.php
@@ -29,14 +33,83 @@ class SystemHelpBlock extends BlockBase {
+      $configuration,$plugin_id, $plugin_definition, $container->get('request'), $container->get('module_handler'));

Missing whitespace

dawehner’s picture

#40: help-1831846-40.patch queued for re-testing.

catch’s picture

Title: arg() is using _current_path() instead of current_path() » Help block is broken with language path prefixes

Re-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.

dawehner’s picture

It 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.

Status: Needs review » Needs work

The last submitted patch, help-1831846-FAIL.patch, failed testing.

Gábor Hojtsy’s picture

Unfortunatelt the PASS patch did not pass either :/ Do we want to remove menu_get_active_help() too then?

dawehner’s picture

Ha yeah I probably missed up the tests rerole ... :(

larowlan’s picture

Status: Needs work » Needs review
FileSize
4.41 KB
1.06 KB
andypost’s picture

Status: Needs review » Reviewed & tested by the community

Let's get this in asap

catch’s picture

Status: Reviewed & tested by the community » Needs work

menu_get_active_help() is dead code now, let's remove that here.

andypost’s picture

Status: Needs work » Needs review
Issue tags: -Needs tests
FileSize
5.28 KB
897 bytes

So removed

catch’s picture

Status: Needs review » Reviewed & tested by the community

Thanks!

Gábor Hojtsy’s picture

Issue tags: +D8MI, +sprint, +language-base

Let's get this in :)

catch’s picture

Status: Reviewed & tested by the community » Fixed

Committed/pushed to 8.x, thanks!

Gábor Hojtsy’s picture

Issue tags: -sprint

Woot, amazing!

Automatically closed -- issue fixed for 2 weeks with no activity.

Anonymous’s picture

Issue summary: View changes

Update with new STR, now a fatal error