Support from Acquia helps fund testing for Drupal Acquia logo

Comments

pcambra’s picture

Title: User test vars declared as \Drupal\user\Plugin\Core\Entity\User where it should be \Drupal\user\Plugin\Core\Entity\UserInterface » User test vars declared as \Drupal\user\Plugin\Core\Entity\User where it should be \Drupal\user\UserInterface
FileSize
5.86 KB

It should be actually \Drupal\user\UserInterface

jhodgdon’s picture

Component: documentation » user.module

This seems reasonable to me, and the documentation syntax is correct. Our documentation/coding standards do say to use interfaces in favor of classes whenever possible... But maybe one of the maintainers of the user module can comment on the accuracy of doing that here?

Berdir’s picture

Looks good to me.

There are a lot more $account and $user variables, arguments which aren't documented at all yet or possibly still as Drupal\user\User and all kinds of broken things. I'm not sure if we should fix that here too, that might end up being a bigger patch or if we should simply make incremental improvements (fine with me, also less conflicts with other, more important patches)

pcambra’s picture

Issue tags: +Novice

Tagging

jhodgdon’s picture

Component: user.module » documentation
Assigned: Unassigned » jhodgdon
Status: Needs review » Reviewed & tested by the community

Let's leave this issue as it is now. Thanks for the review, Berdir -- I'll get this one committed soon.

Regarding other spots... If someone wants to file a "meta" issue with a title something like:

[meta] User variables should be documented as UserInterface not Plugin\Core\User

and reference this as a sub-issue, that would not be a bad idea. As a bonus, a grep through core for references to Drupal\user\Plugin\Core\Entity\User in documentation would help us figure out the scope.

pcambra’s picture

jhodgdon’s picture

Assigned: jhodgdon » Unassigned
Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs reroll

The patch no longer applies in
core/modules/node/lib/Drupal/node/Tests/NodeAccessLanguageAwareCombinationTest.php

pcambra’s picture

Status: Needs work » Needs review
Issue tags: -Needs reroll
FileSize
5.23 KB

Here we go

jhodgdon’s picture

Status: Needs review » Fixed

Thanks! Committed to 8.x.

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