[Regression] Fatal Error Call to a member function getDirectoryPath() on a non-object when Default picture is set

I can reproduce the same in drupal 7 beta 1 without Gravatar integration module.

To reproduce, set "Default picture" field in admin/config/people/accounts to http://images.adbard.net/sites/default/files/audience_1.png or similar and access node page. The site will be totally broken now.

Can confirm, somewhere in template_preprocess_user_picture() the $filepath isn't in the right format to make sure file API knows where to get the picture. On top of that, the comments above are pretty confusing also imho :).

Confirmed!

Ok, a workaround to make this work is 'simple'. If the avatar is in your public files directory (sites/default/files) and you enter

public://filename_of_default_picture.extension

everything will be fine. Maybe we should add a validate to this field to really find out if the URI entered is valid and give some guidance how to enter a value in this field, or on validate convert it to a URI which can be read by the stream wrapper.

image_style_url() assumes a local file. You can't put in anything that has http:// in that field. Is that a regression? I can't think of anywhere else that lets you enter in an external url like that.

This is a regression. It was perfectly valid to allow an external URL.

Added a check to url_is_external() so that image styles aren't loaded.

file_stream_wrapper_valid_scheme(file_uri_scheme($filepath)) feels more appropriate here.

Or better yet, file_valid_uri() which is basically a wrapper around this.

+1 for using the API properly.

Let's get a test for this.

Working on a comment and a test right now.

First attempt at writing tests. Fails without the change, passes with it. Sounds right to me?

Thanks chx and davereid.

#15: drupal-937562-15.patch queued for re-testing.

Uh, thanks for the retest?

Anyway, justinrandell raised concerns about using druplicon.png for the tests, since it could theoretically someday be used on /user. I used druplicon.png at chx's suggestion, and it's also used in other tests.

He suggested using $this->drupalGetTestFiles('image') and getLocalPath(), but that wouldn't work because that isn't an absolute or external URL. He also said "i'm not going to stop this patch if you leave it as is".

So where does that put us?

That druplicon image is there approx since Drupal 1 :) I am not worried. We do not use its dimensions either.

After further discussion let's fix it rather. Use a simpletest image and xpath to ensure it's on the page because it's a user picture.

assertTrue == should be assertEqual and simpletest attributes() method is not needed if you know the name of the attribute -- just use the array syntax, just dont forget to cast to string as well.

Thanks for fixing it while I slept. :)

Awesome! A few minor nitpicks and then this is good to go:

+      // If the image does not have a valid Drupal scheme, don't load image styles.
+      if (module_exists('image') && file_valid_uri($filepath) && $style = variable_get('user_picture_style', '')) {

Why would this happen? Could you add an example? It's not clear from the code.

+  /**
+   * Do the test:
+   *  Picture is valid URL
+   *
+   * results: The image should be displayed
+   */

This is non-standard PHPDoc. Should just be one short sentence description that is < 80 chars.

+    $images = $this->drupalGetTestFiles('image');
+    $image = $images[0];
+    $pic_path = file_create_url($image->uri);
+    variable_set('user_picture_default', $pic_path);

Let's get a comment summary at the top of this hunk, like you have down below, to explain what's happening.

Done all three despite the test class is already full of this "non-standard" PHPDoc.

Committed to CVS HEAD. Thanks.