Undefined index in ctools_argument_entity_id_context()

With assumption that all keys are numeric that's fine. At least for d7

Why not just use trim() ? I know for now they're numeric, that won't always be the case. It'll make porting easier

I'm seeing this error everywhere since upgrading to 1.6-rc1.
Node edit pages, views admin, panels admin and most other admin pages.

Error message

Notice: Undefined variable: id in ctools_argument_entity_id_context() (line 73 of sites/all/modules/contrib/ctools/plugins/arguments/entity_id.inc).

the patch above doesn't resolve this notice.

How about this two birds patch?

Whoops, that is the wrong one... this one is the ticket.

OMG, sorry for the noise. The right one is the 666 byte patch;)

@maximpodorov it may not be good to fix more than one problem in this issue's patch as it makes it harder to review and slower to get in. Though most of those fixes are whitespace fixes and better naming which I agree should be fixed, this issue may not be the place for it and a follow up to the other issue may be better strategy.

Saying that... the maintainers of this module may have a different approach than how core patches get in.

It would be better to create a third linked one OR we need to be really clear on their connection and how the changes help the original reported issue.

I'm okay with linking the two here. Thanks for catching the RC1 issue Joel, .. I don't like all the entity_loads going on in the patch, but I cannot think of a better way to do it at the moment since ctools does not depend on Entity API.

Did the last patch fix the issues you were seeing?

1. +++ b/plugins/arguments/entity_id.inc
   @@ -60,28 +61,29 @@ function ctools_argument_entity_id_context($arg = NULL, $conf = NULL, $empty = F
        if ($match) {
          $id = $preg_matches[1];
   +      if (is_numeric($id)) {
   +        return ctools_context_create('entity:' . $entity_type, $id);
   +      }
        }
   -    if (is_numeric($id)) {
   -      return ctools_context_create('entity:' . $entity_type, $id);
   -    }
   -    return FALSE;
   

   This should fix the issue as well because the $id is defined right above it. So +1 to this.

2. +++ b/plugins/arguments/entity_id.inc
   @@ -60,28 +61,29 @@ function ctools_argument_entity_id_context($arg = NULL, $conf = NULL, $empty = F
   +  $arg = trim($arg);
   +
   

   And the arg is still trimmed just earlier so that is good as well and probably better to trim it earlier than later.
   +1 to this too.

3. +++ b/plugins/arguments/entity_id.inc
   @@ -60,28 +61,29 @@ function ctools_argument_entity_id_context($arg = NULL, $conf = NULL, $empty = F
   -  return ctools_context_create('entity:' . $entity_type, $entity[$arg]);
   +  return ctools_context_create('entity:' . $entity_type, reset($entities));
   

   This looks like this change could cause issues as it was using the $arg as the key (not knowing what $arg is referring too in this context) and now it's grabbing the first entity... which may be correct but just looks suspicious so I bring it up.

4. And the big hunk on the settings form changes look a bit tricky for me to review because I don't know what it's trying to do. But the first and last part of it look fairly straight forward and a nice cleanup.

I just applied the last patch (#9) and to CTools 1.6 RC1 and then ran a Selenium IDE test suite that has a couple hundred tests, many of which depend on Views, Panels, CTools modals, etc and everything passed fine and the PHP Notice is gone.

In short, this patch works for me.

Based on Maxim's comment that the two issues are linked, I didn't test the more modest patch and since all my tests passed, I'm happy with the more extensive patch :-)

3. Ok, just wanted to make sure, kinda looked like that was the case but the way it was used before indicated it was trying to get a specific index by $arg, but if it's just one yours makes that much more clear of it's intent.

4.

+++ b/plugins/arguments/entity_id.inc
@@ -96,30 +98,36 @@ function ctools_argument_entity_id_settings_form(&$form, &$form_state, $conf) {
-        $link = l(t("'%title' [%type id %id]", array('%title' => $info->{$entity['entity keys']['label']}, '%type' => $plugin['keyword'], '%id' => $conf['entity_id'])), file_create_url($uri), array('attributes' => array('target' => '_blank', 'title' => t('Open in new window')), 'html' => TRUE));
...
+      $link = is_array($uri) ? l($title, $uri['path'], $options) : (!empty($uri) ? l($title, file_create_url($uri), $options) : $title);

Before file_create_url() was only called if there was a label. Now it's happeing if the $uri isn't empty.

A bit unsure why the asset url creation is used instead of a url() function... is it an actual file being linked?

The double ternary isn't all that easy to read.

Well without the double ternary that's nicer, thanks @maximpodorov. Glad to see that file_create_url() is gone, it was quite precarious there to start.

One problem with this... I said it passed my test suite, but actually it didn't. Both patch #9 and #18 have the same problem

If I'm logged in and go to example.com/user, with 1.4, 1.5, 1.6 RC1, the H1 tag is the username. When I apply either of these patches, however, the H1 becomes "Log in"

I can use the Page Title module with tokens to make the meta title be the username and the content of the page is the profile content, but the H1 becomes "Log in"

I just applied the patch in #8 and it solves the notice and I have not yet seen any side effects.

Despite Maxim's observations that the two issues are related, it looks like Joel's more modest patch might more effectively close this issue and the related issue could be solved with a separate issue and separate patch.

The patch in #8 fixes the undefined index for me on 7.x-1.6

@ergophobe Can you add steps to reproduce your issue mentioned in #20 with the latest dev version and #18 applied?

I am running Drupal 7.20 on my system.

Yesterday, when I attempted to update Ctools to the latest version 7.x-1.6, I received an error similar to this....

"Class CToolsCssCache does not implement abstract method __construct......"

The error was pointing to code in the "......./sites/all/modules/ctools/includes/css-cache.inc" file. This file doesn't seem to be present in CTools version 7.x-1.5.

I am not sure where I should post this error. Any feedback will be very much appreciated.

@fuzzbuzz first, upgrade Drupal! There was a huge security hole discovered last year, previous to Drupal 7.32. Or at very least patch it. @see https://www.drupal.org/SA-CORE-2014-005

Second that error is not related to this issue. So first search for that error message in the issue queue here and if you don't find it, open up a new issue with the details.
Also it looks like you found the issue already ;) #2408217: Class CToolsCssCache does not implement abstract method __construct

Tried patch #8 but still getting a blank page on a page with the panelizer tab after upgrading to panels 7.x-3.5 and ctools 7.x-1.6.
There is no error being displayed when the page is loaded. The only error was after the upgrade and was Notice: Undefined variable: id in ctools_argument_entity_id_context() (line 73

We're going to use the patch from #8 in Panopoly. I also agree that we should go with the more modest fix in #8 here first, and then fix the bigger issues that @maximpodorov is working on in a separate issue. Let's get the quick fix in first because it's quick! :-)

@maximpodorov you're patch is about half the size and easier to review without the doc standards cleanup. I'm thinking of punting that to another issue, so it's easier to review your patch. As you can see people gravitate to the quickfix even though yours may be more on target.

@joelpittet - the steps are pretty much as described.

If I'm logged in and go to example.com/user, with 1.4, 1.5, 1.6 RC1, the H1 tag is the username. When I apply either of these patches (9 or 18), however, the H1 becomes "Log in"

If I apply patch 8, the H1 is the username, as in the distro.

Patch #8 resolves the undefined index issue for me on 7.x-1.6 as well.

Hmm ok @ergophobe and @maximpodorov Maybe we should just do #8 for now until the title issue can be resolved?

That would make my life simpler. Otherwise I'm going to have freeze at 1.6 and I don't want to do that, because I think there will be a number of solid patches that come out to fix minor issues that come to light now that 1.6 is official (I've seen a number in the Panopoly queue). Having a commit with potential side effects will make it harder to bring out 1.7.

Here is #8 but with all the cosmetic changes from #18. That should make for a closer aim for what @maximpodorov had.

Those other changes @maximpodorov can make in a follow-up that is a bit more to the point.

Trying this again...

#36 did it for me

Thanks a bunch!

@ergophobe would you do the RTBC honors regarding your issue?

#36 works! Thanks.

I was seeing the error displayed consistently on the "/node/add" page prior to applying #36. The patch has taken care of the issue, thank you!

I'll try to do that ASAP - needs to be tested by starting from the distro and adding any other patches so I'm sure I have the same conditions, but should be able to do it today or tomorrow. If you don't see it by Tuesday, feel free to prompt me.

@joelpittet - RTBC regarding my issue.

I also did a file compare on the renamed patch and it is binary same, so either #36 or #42 are the same except for the patch name.

@maximpodorov - as near as I can tell, the code in your interdiff looks good, but everything I spot-checked looked the same as the code from lines 99+ of #2319515: Entity id context placeholder form (for use with panels preview)

If you create an issue and patch, though, I'd be happy to test it for the page title issue that I had with your previous patch. Just drop me a msg if you open an issue and want me to follow so I can verify that the H1 issue is OK.

Great. If you add a post here and link it, I'll test ASAP

can confirm error goes away on applying patch from #42

+++ b/plugins/arguments/entity_id.inc
@@ -70,18 +74,18 @@ function ctools_argument_entity_id_context($arg = NULL, $conf = NULL, $empty = F
+  return ctools_context_create('entity:' . $entity_type, reset($entities));

Why not use entity_load_single()?

Guys, you are really using focus here. While you are discussing and cleaning up dirty that includes a feature that has been faster merged than the solution to this bug, the watchdogs all over the world are spammed by this problem.

I created a patch without all the coding standard stuff, the file_create_uri() and the empty check in the setting dialog. That is all just not part of the solution.
Simple patch with 6 changes rows, no interdiff needed.
Thanks to maximpodorov for his pre-work there.

Just apply my patch or any patch what so ever to the module and create an update release.

Thanks.

@func0der, I do agree to some extent, that is my patch in #8 but a project maintainer in #13 mentioned they would welcome those changes as well. That is why we are down that road now.

I'm ok with #42 going in since it is the answer to the universe and all.

Okay #42 looks good to me. Not sure about the latest patch from func0der, so I'll hold off on that for now. Fixed!

@joelpittet: Maintainers can be wrong, too.
And the fact that from comment #13 until now 3 months have past (almost) is reason enough to believe that was the case this time.

@japerry:
What are you not sure about? My patch is just doing what needs to be done to solve THIS issues problem.

The patch in #42 is maybe cleaning up coding standards, but is also introducing an unnecessary conditional (isset($id)) instead of moving the whole conditional two lines up like maximpodorov did it. Second has much more logic behind it, because the $id variable does not exist at all outside of the conditional in which it is created.
Either way there is simply too much noise in there and it may also introduce new bugs that are harder to find, if you would apply a big patch to the module.

I really do not care if you take my patch or not. And it is maybe much too late to discuss this now, but maybe consider it next time, if you want.

And please, release this fix fast.

@func0der, I agree they could be wrong, though I just want a fix in as do you... so I'm siding with "work with others" to make that happen. I spoke my peace, but someone has to make the call and in this case it's the maintainers... so roll with it.

#42 is committed now, could you provide a patch that would resolve the issues brought up in #53 against #42?

Should have been "re-roll with it", for a better pun.