Notice: Undefined index: #item in user_user_view_alter()

Call to a member function get() on null in user_user_view_alter() (line 410 of /app/www/cmd/web/core/modules/user/user.module)
My error is on line 410, not 409 .

if (!$item->get('alt')->getValue()) {

BTW @jungle thanks for writting the module [avatar_field_formatter](https://www.drupal.org/project/avatar_field_formatter), very useful.

But

A known core issue: Notice: Undefined index: #item in user_user_view_alter(), so you have to apply this patch to make this module works.

This description on [avatar_field_formatter](https://www.drupal.org/project/avatar_field_formatter) is not accurate.

Because "If we want display user picture on user entity view page" is the only condition that we should apply this patch.

Even if we use views to display user's picture and use module
[avatar_field_formatter](https://www.drupal.org/project/avatar_field_formatter)'s widget, we don't have need to apply the patch either.

It's true, children 'alt' property must be checked before getting the value.

#2907329: Add test that user_user_view_alter works with different formatters seems to be the same issue and has a similar fix but simpler test.

Gave that a quick once over. It's neat and clean and straight forward.

Only some small things to consider.

1. +++ b/core/modules/user/tests/src/Functional/UserPictureTest.php
   @@ -159,4 +168,18 @@ public function saveUserPicture($image) {
   +  /**
   +   * Tests dummy User picture formatter.
   +   */
   +  public function testDummyUserPictureFormatter() {
   

   Possible to refactor this into a kernel test?

   If your test implements formInterface, you can generate inputs the the build() method and submit it, in a kernel test

2. +++ b/core/modules/user/user.module
   @@ -319,9 +320,14 @@ function user_user_view(array &$build, UserInterface $account, EntityViewDisplay
   +      if ($item instanceof ImageItem) {
   

   Relexively I bork at the need for instanceOf, we are in a procdural function though. I imagine the refactoring necessary to avoid it would be heavy

Except nitpick looks good to go

+++ b/core/modules/image/tests/modules/image_module_test/src/Plugin/Field/FieldFormatter/DummyImageFormatter.php
@@ -0,0 +1,33 @@
+  public function viewElements(FieldItemListInterface $items, $langcode) {
+    $elements = [];
+    $elements[] = ['#markup' => 'Dummy'];
+    return $elements;

It's more readable when

return [
  ['#markup' => 'Dummy'],
];

+++ b/core/modules/user/user.module
@@ -319,9 +320,14 @@ function user_user_view(array &$build, UserInterface $account, EntityViewDisplay
-      if (!$item->get('alt')->getValue()) {
-        $item->get('alt')->setValue(\Drupal::translation()->translate('Profile picture for user @username', ['@username' => $account->getAccountName()]));
+      if ($item instanceof ImageItem) {

we can do this as

if ($item && array_key_exists('alt', $item->getProperties)) {

and avoid instanceof, which is generally a code-smell that indicates a missing abstraction.

But in this case, we have that abstraction, its getProperties

Which is the same thing @thursday_bw flagged too

The vulnerable aspect of testing against instanceof is it prevents imageItem from being extended or replaced by a new class. eg

You do have access to all the methods of the fieldItemInterface. That opens up some possibilities.
You can also easily add a new interface.

class MyFancyImageItem extends ImageItem {}
hook_form_alter('user_profile_form' ...) {
  $form['myOldImageField']['#type'] = 'my_fancy_image_item';
}

Should someone do something like this, then test against instanceof 'ImageItem' will fail, and produce an error.

Recommendation 1

If you instead test against the object's capabilities then the person extending imageField has more control over how your
code deals with. But it wont' break it purely by overriding the class.

You'll need to dig into what data you have available for you to most flexibly test against.
Non functional, non tested example:

      if ($item->getFieldDefinition()['somekey'] == 'someidentifyingvalue') {
        if (!$item->get('alt')->getValue()) {
          $item->get('alt')->setValue(\Drupal::translation()
            ->translate('Profile picture for user @username', ['@username' => $account->getAccountName()]));
        }
      }


      // or
      if ($item->fieldSettingsForm()->somePropertyOrMethodOrwhatever() == 'someidentifyingvalue') {
        if (!$item->get('alt')->getValue()) {
          $item->get('alt')->setValue(\Drupal::translation()
            ->translate('Profile picture for user @username', ['@username' => $account->getAccountName()]));
        }
      }

Recommendation 2

Another option, that may have similar drawbacks is the following:

Add an interface for imageItem that extends fieldItemInterface

Create the interface:

+ interface ImageItemInterface extends FieldItemInterface {
+ ...
+ }

Implement the interface:

- class ImageItem extends FileItem {
- ...
- }
+ class ImageItem extends FileItem implements ImageItemInterface {
+ ...
+ }

Then you could test that the object implements the ImageItemInterface. like this:

      $item = $build['user_picture'][$key]['#item'];
      $interfaces = class_implements();
      if (isset($interfaces['ImageItemInterface'])) {
        if (!$item->get('alt')->getValue()) {
          $item->get('alt')->setValue(\Drupal::translation()
            ->translate('Profile picture for user @username', ['@username' => $account->getAccountName()]));
        }
      }

Now ImageInterface can still be happily extended, and you're not writing ugly code that analyses the field settings.

And @larowlan has honed in on exactly what we needed to know. With much less words too ;)

Looking nice.

1. +++ b/core/modules/user/tests/src/Functional/UserPictureTest.php
   @@ -159,4 +168,18 @@ public function saveUserPicture($image) {
   +    $this->drupalLogin($this->webUser);
   +    // Set dummy_image_formatter to the default view mode of user entity.
   +    EntityViewDisplay::load('user.user.default')->setComponent('user_picture', [
   +      'region' => 'content',
   +      'type' => 'dummy_image_formatter',
   +    ])->save();
   +    $this->drupalGet($this->webUser->toUrl());
   

   micro-optimisation, but we're making two requests to the same URL here because $this->drupalLogin submits to user/login, which is UserLoginForm, which in term does this

   $form_state->setRedirect(
           'entity.user.canonical',
           ['user' => $account->id()]
         );
   

   i.e. goes to the $this->webUser->toUrl(), which we then do again.

   So if we setup the view display first, then did the login, and then asserted we were on the user page (to ensure we are on the correct page) we'd avoid one extra http request.

2. +++ b/core/modules/user/user.module
   @@ -317,10 +317,14 @@ function user_user_view(array &$build, UserInterface $account, EntityViewDisplay
   -      if (!$item->get('alt')->getValue()) {
   +      if ($item->get('alt')) {
   

   this is a change in behaviour now.

   We need $item->get('alt') && !$item->get('alt')->getValue()

   the original code only runs if there is no value, now we're running it in all cases.

+++ b/core/modules/image/tests/modules/image_module_test/src/Plugin/Field/FieldFormatter/DummyImageFormatter.php
@@ -0,0 +1,33 @@
+      ['#markup' => 'Dummy'],

+++ b/core/modules/user/tests/src/Functional/UserPictureTest.php
@@ -159,4 +168,17 @@ public function saveUserPicture($image) {
+    $this->drupalLogin($this->webUser);
+    $this->assertSession()->statusCodeEquals(200);

I'd extended a test to make sure that "Dummy" is displayed

Thanks, test patch just should not be last

1. +++ b/core/modules/user/tests/src/Functional/UserPictureTest.php
   @@ -42,6 +43,14 @@ class UserPictureTest extends BrowserTestBase {
   +   * {@inheritDoc}
   

   Nit: inheritDoc -> inheritdoc

2. +++ b/core/modules/user/tests/src/Functional/UserPictureTest.php
   @@ -159,4 +168,18 @@ public function saveUserPicture($image) {
   +   * Tests dummy User picture formatter.
   

   This isn't a great description, should we explain why we actually need a dummy formatter test?

To address #32.2 - no reason to change it if we sure that element instanceof ImageItem which is only defined by standard profile.

Maybe be can move it there or refactor as user_picture_enabled() not used anymore, looks needs follow-up

Patch installs the testing module for only this case (to not affect existing)

wrong patch and interdiff (other test cases were disabled for debug)

1. +++ b/core/modules/user/tests/src/Functional/UserPictureTest.php
   @@ -169,9 +161,12 @@ public function saveUserPicture($image) {
   -   * Tests user_user_view_alter() with a dummy image formatter for user picture.
   +   * Tests user picture field with a dummy image formatter for user picture.
   

   still needs better wording(

2. There's only 4 usage found in contrib for http://grep.xnddx.ru/search?text=user_picture_enabled

Filed another task follow-up #3151555: Deprecate user_picture_enabled() reconsider user picture field

Also I see similar changes discussed in #2844302-19: Move Field Layout data model and API directly into \Drupal\Core\Entity\EntityDisplayBase (last 11 point)

+++ b/core/modules/user/tests/src/Functional/UserPictureTest.php
@@ -159,4 +160,21 @@ public function saveUserPicture($image) {
+   * Tests user picture field with a dummy image formatter for user picture.

"Tests user picture field with a non-standard field formatter"?

Looks great!

+++ b/core/modules/user/user.module
@@ -317,8 +318,14 @@ function user_user_view(array &$build, UserInterface $account, EntityViewDisplay
+        // User picture field is provided by standard profile install, skip
+        // if the render using non-image field item which may miss alt property.

nit: 'User picture field is provided by standard profile install, skip if the render using non-image field item which may miss alt property' is not grammatically correct.

Suggest 'User picture field is provided by standard profile install. If the display is configured to use a different formatter, the #item render key may not exist, or may not be an image field.'

Or similar

Feel free to re-RTBC if the patch only changes the comment.

Saving review credits

Committed aece631 and pushed to 9.1.x. Thanks!

As this is a bugfix, and there is no disruption or string changes and we have green runs on 9.0 and 8.9, cherry-picked to those branches too.

Thanks everyone