Follow up for #1361228: Make the user entity a classed object

We need to revise documentation so that we can properly apply type hinting for code that deals with user entities.

CommentFileSizeAuthor
#31 1537434_31.patch36.12 KBcosmicdreams
#27 user_entity_type_hinting_27.patch34.23 KBgarphy
#24 user_entity_type_hinting_24.patch34.21 KBsocketwench
#19 user_entity_type_hinting_19.patch56.29 KBcosmicdreams
#17 user_entity_type_hinting.patch33.69 KBcosmicdreams
#14 user_entity_type_hinting-1537434-14.patch46.59 KBalbert volkman
#14 interdiff.txt390 bytesalbert volkman
#12 user_entity_type_hinting-1537434-12.patch46.54 KBalbert volkman
#6 1537434-user-entity-type-hinting-6.patch46.87 KBaspilicious
#3 1537434-user-entity-type-hinting-3.patch50.47 KBaspilicious
#1 1537434-user-entity-type-hinting-1.patch50.78 KBaspilicious

Comments

aspilicious’s picture

aspilicious commented
Status: Active » Needs review
StatusFileSize
new 1537434-user-entity-type-hinting-1.patch50.78 KB

Experimental patch... Wonna see what happens with the bot :)

Status: Needs review » Needs work

The last submitted patch, 1537434-user-entity-type-hinting-1.patch, failed testing.

aspilicious’s picture

aspilicious commented
Status: Needs work » Needs review
StatusFileSize
new 1537434-user-entity-type-hinting-3.patch50.47 KB

Reroll, will probably fail again. Have no idea why...

Status: Needs review » Needs work

The last submitted patch, 1537434-user-entity-type-hinting-3.patch, failed testing.

aspilicious’s picture

aspilicious commented

Ok after installing I found the nastyness...
global $user is still a std object so most functions get both instances of User and std object. Resulting into failures...

aspilicious’s picture

aspilicious commented
Status: Needs work » Needs review
StatusFileSize
new 1537434-user-entity-type-hinting-6.patch46.87 KB

Ok this one should install...

Status: Needs review » Needs work

The last submitted patch, 1537434-user-entity-type-hinting-6.patch, failed testing.

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
commented
Issue tags: +PSR-0

Tagging.

cosmicdreams’s picture

cosmicdreams commented
Status: Needs work » Needs review
Issue tags: -PSR-0

#6: 1537434-user-entity-type-hinting-6.patch queued for re-testing.

Status: Needs review » Needs work
Issue tags: +PSR-0

The last submitted patch, 1537434-user-entity-type-hinting-6.patch, failed testing.

cosmicdreams’s picture

cosmicdreams commented
Issue tags: +Needs reroll

needs reroll

albert volkman’s picture

albert volkman commented
Status: Needs work » Needs review
StatusFileSize
new user_entity_type_hinting-1537434-12.patch46.54 KB

Reroll.

Status: Needs review » Needs work

The last submitted patch, user_entity_type_hinting-1537434-12.patch, failed testing.

albert volkman’s picture

albert volkman commented
Status: Needs work » Needs review
StatusFileSize
new interdiff.txt390 bytes
new user_entity_type_hinting-1537434-14.patch46.59 KB

Forgot 'use' statement in user.module.

Status: Needs review » Needs work

The last submitted patch, user_entity_type_hinting-1537434-14.patch, failed testing.

albert volkman’s picture

albert volkman commented
Status: Needs work » Postponed

Postponing.

cosmicdreams’s picture

cosmicdreams commented
Status: Postponed » Needs review
StatusFileSize
new user_entity_type_hinting.patch33.69 KB

Rerolled, provided type hinting in user.module. Checking to see how much this breaks.

Status: Needs review » Needs work

The last submitted patch, user_entity_type_hinting.patch, failed testing.

cosmicdreams’s picture

cosmicdreams commented
Status: Needs work » Needs review
StatusFileSize
new user_entity_type_hinting_19.patch56.29 KB

After reviewing the patch I found many more places where type hinting could be used.

Status: Needs review » Needs work

The last submitted patch, user_entity_type_hinting_19.patch, failed testing.

aspilicious’s picture

aspilicious commented
Status: Needs work » Postponed

This was postponed because session handling isn't a true entity. This will fail all the time. Leave it postponed untill we figured out the session handling.

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
commented
Status: Postponed » Needs work
socketwench’s picture

socketwench commented
Status: Needs work » Needs review
StatusFileSize
new user_entity_type_hinting_24.patch34.21 KB

Rerolled. I hope.

I wonder if I can beg someone to show me how to do this in PHPstorm. Doing manually is just too error prone.

socketwench’s picture

socketwench commented

Might need to redo the patch because of https://drupal.org/node/2053489

Status: Needs review » Needs work

The last submitted patch, user_entity_type_hinting_24.patch, failed testing.

garphy’s picture

garphy
Primary language French
Location Montreal, Canada
commented
Issue tags: -Needs reroll
StatusFileSize
new user_entity_type_hinting_27.patch34.23 KB

Rerolled

garphy’s picture

garphy
Primary language French
Location Montreal, Canada
commented
Status: Needs work » Needs review

changing status to trigger the bot

Status: Needs review » Needs work

The last submitted patch, user_entity_type_hinting_27.patch, failed testing.

cosmicdreams’s picture

cosmicdreams commented 
+++ b/core/includes/bootstrap.inc
@@ -2848,7 +2848,7 @@ function drupal_classloader_register($name, $path) {
+ * function user_access($string, User $account = NULL) {

In general, why are we using User and not UserInterface to type these parameters?

So, I think we need to document the parameters in the function's docblock.

cosmicdreams’s picture

cosmicdreams commented
Status: Needs work » Needs review
StatusFileSize
new 1537434_31.patch36.12 KB

Here's a patch that explains what I'm saying.

Status: Needs review » Needs work

The last submitted patch, 1537434_31.patch, failed testing.

berdir’s picture

berdir
Primary language German
Location Switzerland
commented 
  1. +++ b/core/modules/comment/comment.module
@@ -1211,8 +1212,65 @@ function comment_user_cancel($edit, $account, $method) {
-function comment_user_predelete($account) {
+function comment_user_predelete(User $account) {

    Yes, UserInterface should be used, for example here too :)

    Lots of those below, always UserInterface, in @params and hook_*() documentations always fully qualified with a leading \, in actual implementations only UserInterface + a use. 

  2. +++ b/core/modules/comment/comment.module
@@ -1211,8 +1212,65 @@ function comment_user_cancel($edit, $account, $method) {
+
+/**
+ * Accepts a submission of new or changed comment content.
+ *
+ * @param Drupal\comment\Comment $comment
+ *   A comment object.
+ */
+function comment_save(Comment $comment) {
+  $comment->save();
+}

    I have absolutely no clue where all this is coming from (not just this, all the added functions around this), but it shouldn't be here :)

cosmicdreams’s picture

cosmicdreams commented

Probably just how I created the patch. Clearly, there's more work to be done.

Version: 8.0.x-dev » 8.1.x-dev

Drupal 8.0.6 was released on April 6 and is the final bugfix release for the Drupal 8.0.x series. Drupal 8.0.x will not receive any further development aside from security fixes. Drupal 8.1.0-rc1 is now available and sites should prepare to update to 8.1.0.

Bug reports should be targeted against the 8.1.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.2.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.1.x-dev » 8.2.x-dev

Drupal 8.1.9 was released on September 7 and is the final bugfix release for the Drupal 8.1.x series. Drupal 8.1.x will not receive any further development aside from security fixes. Drupal 8.2.0-rc1 is now available and sites should prepare to upgrade to 8.2.0.

Bug reports should be targeted against the 8.2.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.3.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

dpi’s picture

dpi
Location Perth, Australia
commented
Issue summary: View changes
Status: Needs work » Closed (outdated)

Safe to say this is outdated.